Fix x86 stack probing

On x86, structs are passed by value on the stack. We copy structs to the stack in various ways, but one way is to first subtract the size of the struct and then use a "rep movsb" instruction. If the struct we are passing is sufficiently large, this can cause us to miss the stack guard page. So, introduce stack probes for these struct copies. It turns out the stack pointer after prolog probing can be sitting near the very end of the guard page (one `STACK_ALIGN` slot before the end, which allows a "call" instruction which pushes its return address to touch the guard page with the return address push). We don't want to probe with every argument push, though. So change the prolog probing to insert an "extra" touch at the final SP location if the previous touch was "too far" away, leaving at least some buffer zone for un-probed SP adjustments. I chose this to be the size of the largest SIMD register, which also can get copied to the argument stack with a "SUB;MOV" sequence. Added several test case variations showing different large stack probe situations. Fixes #23796
BruceForstall · Apr 10, 2019 · 98b6a5b · 98b6a5b
1 parent a6b1e97
commit 98b6a5b
Show file tree

Hide file tree

Showing 11 changed files with 594 additions and 78 deletions.
diff --git a/src/jit/codegen.h b/src/jit/codegen.h
@@ -1211,6 +1211,14 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
 
     void genReturn(GenTree* treeNode);
 
+#if defined(_TARGET_XARCH_)
+    void genStackPointerConstantAdjustmentWithProbe(ssize_t spDelta, bool hideSpChangeFromEmitter, regNumber regTmp);
+    void genStackPointerConstantAdjustmentLoopWithProbe(ssize_t   spDelta,
+                                                        bool      hideSpChangeFromEmitter,
+                                                        regNumber regTmp);
+    void genStackPointerDynamicAdjustmentWithProbe(regNumber regSpDelta, regNumber regTmp);
+#endif // defined(_TARGET_XARCH_)
+
     void genLclHeap(GenTree* tree);
 
     bool genIsRegCandidateLocal(GenTree* tree)

diff --git a/src/jit/codegenlinear.cpp b/src/jit/codegenlinear.cpp
@@ -1646,7 +1646,7 @@ void CodeGen::genPutArgStkFieldList(GenTreePutArgStk* putArgStk, unsigned outArg
         unsigned thisFieldOffset = argOffset + fieldListPtr->gtFieldOffset;
         getEmitter()->emitIns_S_R(ins_Store(type), attr, reg, outArgVarNum, thisFieldOffset);
 
-        // We can't write beyound the arg area
+        // We can't write beyond the arg area
         assert((thisFieldOffset + EA_SIZE_IN_BYTES(attr)) <= compiler->lvaLclSize(outArgVarNum));
     }
 }

diff --git a/src/jit/codegenxarch.cpp b/src/jit/codegenxarch.cpp
diff --git a/src/jit/target.h b/src/jit/target.h
@@ -488,6 +488,15 @@ typedef unsigned char   regNumberSmall;
   #define INS_stosp                INS_stosd
   #define INS_r_stosp              INS_r_stosd
 
+  // Any stack pointer adjustments larger than this (in bytes) when setting up outgoing call arguments
+  // requires a stack probe. Currently set to the largest supported SIMD register.
+  #define ARG_STACK_PROBE_THRESHOLD_BYTES 32
+
+  // The number of bytes from the end the last probed page that must also be probed, to allow for some
+  // small SP adjustments without probes. If zero, then the stack pointer can point to the last byte/word
+  // on the stack guard page, and must be touched before any further "SUB SP".
+  #define STACK_PROBE_BOUNDARY_THRESHOLD_BYTES ARG_STACK_PROBE_THRESHOLD_BYTES
+
 #elif defined(_TARGET_AMD64_)
   // TODO-AMD64-CQ: Fine tune the following xxBlk threshold values:
 
@@ -889,6 +898,9 @@ typedef unsigned char   regNumberSmall;
   #define INS_stosp                INS_stosq
   #define INS_r_stosp              INS_r_stosq
 
+  // AMD64 uses FEATURE_FIXED_OUT_ARGS so this can be zero.
+  #define STACK_PROBE_BOUNDARY_THRESHOLD_BYTES 0
+
 #elif defined(_TARGET_ARM_)
 
   // TODO-ARM-CQ: Use shift for division by power of 2

diff --git a/tests/src/JIT/Methodical/largeframes/skip3/skippage3.cs b/tests/src/JIT/Methodical/largeframes/skip3/skippage3.cs
@@ -0,0 +1,88 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+// Passing a very large struct by value on the stack, on arm32 and x86,
+// can cause it to be copied from a temp to the outgoing space without
+// probing the stack.
+
+using System;
+using System.Runtime.InteropServices;
+using System.Runtime.CompilerServices;
+
+namespace BigFrames
+{
+
+    [StructLayout(LayoutKind.Explicit)]
+    public struct LargeStruct
+    {
+        [FieldOffset(0)]
+        public int i1;
+        [FieldOffset(65512)]
+        public int i2;
+    }
+
+    public class Test
+    {
+        public static int iret = 1;
+
+        [MethodImplAttribute(MethodImplOptions.NoInlining)]
+        public static void TestWrite(int i1, int i2, int i3, int i4, LargeStruct s)
+        {
+            Console.Write("Enter TestWrite: ");
+            Console.WriteLine(i1 + i2 + i3 + i4 + s.i2);
+            iret = 100;
+        }
+
+        [MethodImplAttribute(MethodImplOptions.NoInlining)]
+        public static void Test1()
+        {
+            Console.WriteLine("Enter Test1");
+            LargeStruct s = new LargeStruct();
+            s.i2 = 5;
+            TestWrite(1, 2, 3, 4, s); // 4 int reg args, then struct stack arg
+        }
+
+        [MethodImplAttribute(MethodImplOptions.NoInlining)]
+        public static void Escape(ref LargeStruct s)
+        {
+        }
+
+        // A lot of time the stack when we are called has a bunch of committed pages
+        // before the guard page. So eat up a bunch of stack before doing our test,
+        // where we want to be near the guard page.
+        [MethodImplAttribute(MethodImplOptions.NoInlining)]
+        public static void EatStackThenTest1(int level = 0)
+        {
+            LargeStruct s = new LargeStruct();
+            s.i2 = level;
+            Escape(ref s);
+
+            if (level < 20)
+            {
+                EatStackThenTest1(level + 1);
+            }
+            else
+            {
+                Test1();
+            }
+        }
+
+        public static int Main()
+        {
+            Test1(); // force JIT of this
+
+            EatStackThenTest1(); // If that didn't fail, eat stack then try again.
+
+            if (iret == 100)
+            {
+                Console.WriteLine("TEST PASSED");
+            }
+            else
+            {
+                Console.WriteLine("TEST FAILED");
+            }
+            return iret;
+        }
+    }
+}
diff --git a/tests/src/JIT/Methodical/largeframes/skip3/skippage3.csproj b/tests/src/JIT/Methodical/largeframes/skip3/skippage3.csproj
@@ -0,0 +1,35 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
+  <PropertyGroup>
+    <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
+    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
+    <AssemblyName>$(MSBuildProjectName)</AssemblyName>
+    <SchemaVersion>2.0</SchemaVersion>
+    <ProjectGuid>{43F24741-6FD9-4593-92FA-D3252B540A92}</ProjectGuid>
+    <OutputType>Exe</OutputType>
+    <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
+    <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
+    <CLRTestPriority>1</CLRTestPriority>
+  </PropertyGroup>
+  <!-- Default configurations to help VS understand the configurations -->
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' "></PropertyGroup>
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' "></PropertyGroup>
+  <ItemGroup>
+    <CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
+      <Visible>False</Visible>
+    </CodeAnalysisDependentAssemblyPaths>
+  </ItemGroup>
+  <PropertyGroup>
+    <DebugType>PdbOnly</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="skippage3.cs" />
+  </ItemGroup>
+  <ItemGroup>
+    <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "></PropertyGroup>
+</Project>
diff --git a/tests/src/JIT/Methodical/largeframes/skip4/skippage4.cs b/tests/src/JIT/Methodical/largeframes/skip4/skippage4.cs
@@ -0,0 +1,61 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+// Passing a very large struct by value on the stack, on arm32 and x86,
+// can cause it to be copied from a temp to the outgoing space without
+// probing the stack.
+
+using System;
+using System.Runtime.InteropServices;
+using System.Runtime.CompilerServices;
+
+namespace BigFrames
+{
+
+    [StructLayout(LayoutKind.Explicit)]
+    public struct LargeStructWithRef
+    {
+        [FieldOffset(0)]
+        public int i1;
+        [FieldOffset(65500)]
+        public Object o1;
+    }
+
+    public class Test
+    {
+        public static int iret = 1;
+
+        [MethodImplAttribute(MethodImplOptions.NoInlining)]
+        public static void TestWrite(LargeStructWithRef s)
+        {
+            Console.Write("Enter TestWrite: ");
+            Console.WriteLine(s.o1.GetHashCode());
+            iret = 100;
+        }
+
+        [MethodImplAttribute(MethodImplOptions.NoInlining)]
+        public static void Test1()
+        {
+            Console.WriteLine("Enter Test1");
+            LargeStructWithRef s = new LargeStructWithRef();
+            s.o1 = new Object();
+            TestWrite(s);
+        }
+
+        public static int Main()
+        {
+            Test1();
+
+            if (iret == 100)
+            {
+                Console.WriteLine("TEST PASSED");
+            }
+            else
+            {
+                Console.WriteLine("TEST FAILED");
+            }
+            return iret;
+        }
+    }
+}
diff --git a/tests/src/JIT/Methodical/largeframes/skip4/skippage4.csproj b/tests/src/JIT/Methodical/largeframes/skip4/skippage4.csproj
@@ -0,0 +1,35 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
+  <PropertyGroup>
+    <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
+    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
+    <AssemblyName>$(MSBuildProjectName)</AssemblyName>
+    <SchemaVersion>2.0</SchemaVersion>
+    <ProjectGuid>{43F24741-6FD9-4593-92FA-D3252B540A92}</ProjectGuid>
+    <OutputType>Exe</OutputType>
+    <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
+    <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
+    <CLRTestPriority>1</CLRTestPriority>
+  </PropertyGroup>
+  <!-- Default configurations to help VS understand the configurations -->
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' "></PropertyGroup>
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' "></PropertyGroup>
+  <ItemGroup>
+    <CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
+      <Visible>False</Visible>
+    </CodeAnalysisDependentAssemblyPaths>
+  </ItemGroup>
+  <PropertyGroup>
+    <DebugType>PdbOnly</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="skippage4.cs" />
+  </ItemGroup>
+  <ItemGroup>
+    <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "></PropertyGroup>
+</Project>
diff --git a/tests/src/JIT/Methodical/largeframes/skip4/skippage4_save.cs b/tests/src/JIT/Methodical/largeframes/skip4/skippage4_save.cs
@@ -0,0 +1,62 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+// Passing a very large struct by value on the stack, on arm32 and x86,
+// can cause it to be copied from a temp to the outgoing space without
+// probing the stack.
+
+using System;
+using System.Runtime.InteropServices;
+using System.Runtime.CompilerServices;
+
+namespace BigFrames
+{
+
+    [StructLayout(LayoutKind.Explicit)]
+    public struct Struct65500ref
+    {
+        [FieldOffset(0)]
+        public int i1;
+        [FieldOffset(65496)]
+        public Object o1;
+    }
+
+    public class Test
+    {
+        public static int iret = 1;
+
+        [MethodImplAttribute(MethodImplOptions.NoInlining)]
+        public static void TestWrite(int i1, int i2, int i3, int i4, Struct65500ref s)
+        {
+            Console.Write("Enter TestWrite: ");
+            Console.WriteLine(i1 + i2 + i3 + i4 + s.o1.GetHashCode());
+            iret = 100;
+            // Test1(); // recurse
+        }
+
+        [MethodImplAttribute(MethodImplOptions.NoInlining)]
+        public static void Test1()
+        {
+            Console.WriteLine("Enter Test1");
+            Struct65500ref s = new Struct65500ref();
+            s.o1 = new Object();
+            TestWrite(1, 2, 3, 4, s); // 4 int reg args, then struct stack arg
+        }
+
+        public static int Main()
+        {
+            Test1();
+
+            if (iret == 100)
+            {
+                Console.WriteLine("TEST PASSED");
+            }
+            else
+            {
+                Console.WriteLine("TEST FAILED");
+            }
+            return iret;
+        }
+    }
+}
diff --git a/tests/src/JIT/Methodical/largeframes/skip5/skippage5.cs b/tests/src/JIT/Methodical/largeframes/skip5/skippage5.cs
@@ -0,0 +1,36 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System;
+using System.Runtime.CompilerServices;
+using System.Runtime.InteropServices;
+
+class Program
+{
+    [StructLayout(LayoutKind.Sequential)]
+    unsafe struct S
+    {
+        fixed byte x[65500];
+    }
+
+    class C
+    {
+        public S s;
+    }
+
+    static int Main() => Test(new C());
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static void Call(int r0, int r1, int r2, int r3, int r4, int r5, int r6, S s)
+    {
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static int Test(C c)
+    {
+        Call(0, 1, 2, 3, 4, 5, 42, c.s);
+        Console.WriteLine("TEST PASSED");
+        return 100; // If we don't crash, we pass
+    }
+}