Fix TailCallStress mode.

Improve validation of tail calls that are not tail-prefixed in the IL but are marked as such because of TailCallStress. We now do the same correctness validation in morph for such tail calls as we do for implicit tail calls. That blocks tail calls when we have address-taken locals, struct promoted params, and pinned vars. Fixes dotnet#39398. Fixes dotnet#39309. Fixes dotnet#38892. Fixes dotnet#38889. Fixes dotnet#38887. Fixes dotnet#37117. Fixes dotnet#8017.
erozenfeld · Aug 14, 2020 · 415e98e · 415e98e
1 parent 4a436e3
commit 415e98e
Show file tree

Hide file tree

Showing 12 changed files with 38 additions and 46 deletions.
diff --git a/eng/pipelines/libraries/run-test-job.yml b/eng/pipelines/libraries/run-test-job.yml
@@ -143,10 +143,7 @@ jobs:
               - jitstress2
               - jitstress2_tiered
               - zapdisable
-              # tailcallstress currently has hundreds of failures on Linux/arm32, so disable it.
-              # Tracked by https://github.com/dotnet/runtime/issues/38892.
-              - ${{ if or(eq(parameters.osGroup, 'Windows_NT'), ne(parameters.archType, 'arm')) }}:
-                - tailcallstress
+              - tailcallstress
             ${{ if in(parameters.coreclrTestGroup, 'jitstressregs' ) }}:
               scenarios:
               - jitstressregs1

diff --git a/src/coreclr/src/jit/gentree.h b/src/coreclr/src/jit/gentree.h
@@ -4201,6 +4201,7 @@ struct GenTreeCall final : public GenTree
 #define GTF_CALL_M_ALLOC_SIDE_EFFECTS      0x00400000 // GT_CALL -- this is a call to an allocator with side effects
 #define GTF_CALL_M_SUPPRESS_GC_TRANSITION  0x00800000 // GT_CALL -- suppress the GC transition (i.e. during a pinvoke) but a separate GC safe point is required.
 #define GTF_CALL_M_EXP_RUNTIME_LOOKUP      0x01000000 // GT_CALL -- this call needs to be tranformed into CFG for the dynamic dictionary expansion feature.
+#define GTF_CALL_M_STRESS_TAILCALL         0x02000000 // GT_CALL -- the call is NOT "tail" prefixed but GTF_CALL_M_EXPLICIT_TAILCALL was added because of tail call stress mode
 
     // clang-format on
 
@@ -4315,6 +4316,13 @@ struct GenTreeCall final : public GenTree
         return (gtCallMoreFlags & GTF_CALL_M_EXPLICIT_TAILCALL) != 0;
     }
 
+    // Returns true if this call didn't have an explicit tail. prefix in the IL
+    // but was marked as an explicit tail call because of tail call stress mode.
+    bool IsStressTailCall() const
+    {
+        return (gtCallMoreFlags & GTF_CALL_M_STRESS_TAILCALL) != 0;
+    }
+
     // This method returning "true" implies that tail call flowgraph morhphing has
     // performed final checks and committed to making a tail call.
     bool IsTailCall() const

diff --git a/src/coreclr/src/jit/importer.cpp b/src/coreclr/src/jit/importer.cpp
@@ -7526,11 +7526,13 @@ enum
     PREFIX_TAILCALL_EXPLICIT = 0x00000001, // call has "tail" IL prefix
     PREFIX_TAILCALL_IMPLICIT =
         0x00000010, // call is treated as having "tail" prefix even though there is no "tail" IL prefix
-    PREFIX_TAILCALL    = (PREFIX_TAILCALL_EXPLICIT | PREFIX_TAILCALL_IMPLICIT),
-    PREFIX_VOLATILE    = 0x00000100,
-    PREFIX_UNALIGNED   = 0x00001000,
-    PREFIX_CONSTRAINED = 0x00010000,
-    PREFIX_READONLY    = 0x00100000
+    PREFIX_TAILCALL_STRESS =
+        0x00000100, // call doesn't "tail" IL prefix but is treated as explicit because of tail call stress
+    PREFIX_TAILCALL    = (PREFIX_TAILCALL_EXPLICIT | PREFIX_TAILCALL_IMPLICIT | PREFIX_TAILCALL_STRESS),
+    PREFIX_VOLATILE    = 0x00001000,
+    PREFIX_UNALIGNED   = 0x00010000,
+    PREFIX_CONSTRAINED = 0x00100000,
+    PREFIX_READONLY    = 0x01000000
 };
 
 /********************************************************************************
@@ -8674,6 +8676,7 @@ var_types Compiler::impImportCall(OPCODE                  opcode,
     {
         const bool isExplicitTailCall = (tailCallFlags & PREFIX_TAILCALL_EXPLICIT) != 0;
         const bool isImplicitTailCall = (tailCallFlags & PREFIX_TAILCALL_IMPLICIT) != 0;
+        const bool isStressTailCall   = (tailCallFlags & PREFIX_TAILCALL_STRESS) != 0;
 
         // Exactly one of these should be true.
         assert(isExplicitTailCall != isImplicitTailCall);
@@ -8740,6 +8743,12 @@ var_types Compiler::impImportCall(OPCODE                  opcode,
                     // for in-lining.
                     call->AsCall()->gtCallMoreFlags |= GTF_CALL_M_EXPLICIT_TAILCALL;
                     JITDUMP("\nGTF_CALL_M_EXPLICIT_TAILCALL set for call [%06u]\n", dspTreeID(call));
+
+                    if (isStressTailCall)
+                    {
+                        call->AsCall()->gtCallMoreFlags |= GTF_CALL_M_STRESS_TAILCALL;
+                        JITDUMP("\nGTF_CALL_M_STRESS_TAILCALL set for call [%06u]\n", dspTreeID(call));
+                    }
                 }
                 else
                 {
@@ -14209,6 +14218,7 @@ void Compiler::impImportBlockCode(BasicBlock* block)
                                     // Stress the tailcall.
                                     JITDUMP(" (Tailcall stress: prefixFlags |= PREFIX_TAILCALL_EXPLICIT)");
                                     prefixFlags |= PREFIX_TAILCALL_EXPLICIT;
+                                    prefixFlags |= PREFIX_TAILCALL_STRESS;
                                 }
                                 else
                                 {

diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp
@@ -7235,7 +7235,7 @@ GenTree* Compiler::fgMorphPotentialTailCall(GenTreeCall* call)
         // We still must check for any struct parameters and set 'hasStructParam'
         // so that we won't transform the recursive tail call into a loop.
         //
-        if (call->IsImplicitTailCall())
+        if (call->IsImplicitTailCall() || call->IsStressTailCall())
         {
             if (varDsc->lvHasLdAddrOp && !lvaIsImplicitByRefLocal(varNum))
             {
@@ -8793,7 +8793,7 @@ GenTree* Compiler::fgMorphCall(GenTreeCall* call)
         assert(!call->CanTailCall());
 
 #if FEATURE_MULTIREG_RET
-        if (fgGlobalMorph && call->HasMultiRegRetVal())
+        if (fgGlobalMorph && call->HasMultiRegRetVal() && varTypeIsStruct(call->TypeGet()))
         {
             // The tail call has been rejected so we must finish the work deferred
             // by impFixupCallStructReturn for multi-reg-returning calls and transform
@@ -8810,23 +8810,14 @@ GenTree* Compiler::fgMorphCall(GenTreeCall* call)
                 lvaGrabTemp(false DEBUGARG("Return value temp for multi-reg return (rejected tail call)."));
             lvaTable[tmpNum].lvIsMultiRegRet = true;
 
-            GenTree* assg = nullptr;
-            if (varTypeIsStruct(call->TypeGet()))
-            {
-                CORINFO_CLASS_HANDLE structHandle = call->gtRetClsHnd;
-                assert(structHandle != NO_CLASS_HANDLE);
-                const bool unsafeValueClsCheck = false;
-                lvaSetStruct(tmpNum, structHandle, unsafeValueClsCheck);
-                var_types structType = lvaTable[tmpNum].lvType;
-                GenTree*  dst        = gtNewLclvNode(tmpNum, structType);
-                assg                 = gtNewAssignNode(dst, call);
-            }
-            else
-            {
-                assg = gtNewTempAssign(tmpNum, call);
-            }
-
-            assg = fgMorphTree(assg);
+            CORINFO_CLASS_HANDLE structHandle = call->gtRetClsHnd;
+            assert(structHandle != NO_CLASS_HANDLE);
+            const bool unsafeValueClsCheck = false;
+            lvaSetStruct(tmpNum, structHandle, unsafeValueClsCheck);
+            var_types structType = lvaTable[tmpNum].lvType;
+            GenTree*  dst        = gtNewLclvNode(tmpNum, structType);
+            GenTree*  assg       = gtNewAssignNode(dst, call);
+            assg                 = fgMorphTree(assg);
 
             // Create the assignment statement and insert it before the current statement.
             Statement* assgStmt = gtNewStmt(assg, compCurStmt->GetILOffsetX());

diff --git a/src/coreclr/tests/issues.targets b/src/coreclr/tests/issues.targets
@@ -67,9 +67,6 @@
         <ExcludeList Include="$(XunitTestBinBase)/GC/Scenarios/muldimjagary/muldimjagary/*">
             <Issue>https://github.com/dotnet/runtime/issues/5933</Issue>
         </ExcludeList>
-        <ExcludeList Include="$(XunitTestBinBase)/JIT/Regression/JitBlue/GitHub_11408/GitHub_11408/*">
-            <Issue>https://github.com/dotnet/runtime/issues/8017</Issue>
-        </ExcludeList>
         <ExcludeList Include="$(XunitTestBinBase)/baseservices/exceptions/StackTracePreserve/StackTracePreserveTests/*">
             <Issue>https://github.com/dotnet/runtime/issues/11213</Issue>
         </ExcludeList>

diff --git a/src/libraries/System.Collections.Concurrent/tests/ProducerConsumerCollectionTests.cs b/src/libraries/System.Collections.Concurrent/tests/ProducerConsumerCollectionTests.cs
@@ -98,7 +98,6 @@ public void Ctor_InitializeFromCollection_ContainsExpectedItems(int numItems)
         }
 
         [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
-        [SkipOnCoreClr("https://github.com/dotnet/runtime/issues/39398", RuntimeTestModes.TailcallStress)]
         public void Add_TakeFromAnotherThread_ExpectedItemsTaken()
         {
             IProducerConsumerCollection<int> c = CreateProducerConsumerCollection();

diff --git a/src/libraries/System.Drawing.Common/tests/FontTests.cs b/src/libraries/System.Drawing.Common/tests/FontTests.cs
@@ -784,7 +784,6 @@ public void SizeInPoints_Get_ReturnsExpected(GraphicsUnit unit)
         [InlineData(FontStyle.Strikeout | FontStyle.Bold | FontStyle.Italic, 255, true, "@", 700)]
         [InlineData(FontStyle.Regular, 0, false, "", 400)]
         [InlineData(FontStyle.Regular, 10, false, "", 400)]
-        [SkipOnCoreClr("https://github.com/dotnet/runtime/issues/38889", RuntimeTestModes.TailcallStress)]
         public void ToLogFont_Invoke_ReturnsExpected(FontStyle fontStyle, byte gdiCharSet, bool gdiVerticalFont, string expectedNamePrefix, int expectedWeight)
         {
             using (FontFamily family = FontFamily.GenericMonospace)
@@ -818,7 +817,6 @@ public void ToLogFont_Invoke_ReturnsExpected(FontStyle fontStyle, byte gdiCharSe
         [InlineData(TextRenderingHint.SingleBitPerPixel)]
         [InlineData(TextRenderingHint.SingleBitPerPixelGridFit)]
         [InlineData(TextRenderingHint.ClearTypeGridFit)]
-        [SkipOnCoreClr("https://github.com/dotnet/runtime/issues/38889", RuntimeTestModes.TailcallStress)]
         public void ToLogFont_InvokeGraphics_ReturnsExpected(TextRenderingHint textRenderingHint)
         {
             using (FontFamily family = FontFamily.GenericMonospace)

diff --git a/src/libraries/System.Net.HttpListener/tests/AssemblyInfo.cs b/src/libraries/System.Net.HttpListener/tests/AssemblyInfo.cs
diff --git a/src/libraries/System.Net.HttpListener/tests/System.Net.HttpListener.Tests.csproj b/src/libraries/System.Net.HttpListener/tests/System.Net.HttpListener.Tests.csproj
@@ -5,7 +5,6 @@
     <TargetFrameworks>$(NetCoreAppCurrent)-Windows_NT;$(NetCoreAppCurrent)-Unix;$(NetCoreAppCurrent)-Browser;$(NetCoreAppCurrent)-OSX</TargetFrameworks>
   </PropertyGroup>
   <ItemGroup>
-    <Compile Include="AssemblyInfo.cs" />
     <Compile Include="GetContextHelper.cs" />
     <Compile Include="HttpListenerFactory.cs" />
     <Compile Include="HttpListenerAuthenticationTests.cs" />

diff --git a/...libraries/System.Security.Cryptography.X509Certificates/tests/RevocationTests/AiaTests.cs b/...libraries/System.Security.Cryptography.X509Certificates/tests/RevocationTests/AiaTests.cs
@@ -42,7 +42,6 @@ public static void EmptyAiaResponseIsIgnored()
         }
 
         [Fact]
-        [SkipOnCoreClr("https://github.com/dotnet/runtime/issues/38887", RuntimeTestModes.TailcallStress)]
         public static void DisableAiaOptionWorks()
         {
             CertificateAuthority.BuildPrivatePki(

diff --git a/src/tests/JIT/opt/OSR/tailrecursetry.csproj b/src/tests/JIT/opt/OSR/tailrecursetry.csproj
@@ -3,6 +3,10 @@
     <OutputType>Exe</OutputType>
     <DebugType />
     <Optimize>True</Optimize>
+    <!-- This test currently fails for TailcallStress. Disable for JIT stress modes until this is fixed.
+         Issue: https://github.com/dotnet/runtime/issues/35687
+    -->
+    <JitOptimizationSensitive>true</JitOptimizationSensitive>
   </PropertyGroup>
   <ItemGroup>
     <Compile Include="$(MSBuildProjectName).cs" />

diff --git a/src/tests/profiler/unittest/getappdomainstaticaddress.csproj b/src/tests/profiler/unittest/getappdomainstaticaddress.csproj
@@ -6,10 +6,6 @@
     <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
     <CLRTestPriority>0</CLRTestPriority>
     <Optimize>true</Optimize>
-    <!-- This test currently fails for TailcallStress. Disable for JIT stress modes until this is fixed.
-         Issue: https://github.com/dotnet/runtime/issues/37117
-    -->
-    <JitOptimizationSensitive>true</JitOptimizationSensitive>
     <!-- This test provides no interesting scenarios for GCStress -->
     <GCStressIncompatible>true</GCStressIncompatible>
   </PropertyGroup>