From 415e98e843f23a1d1d6d52a60ea890d3fd19ace4 Mon Sep 17 00:00:00 2001 From: Eugene Rozenfeld Date: Tue, 11 Aug 2020 23:12:28 -0700 Subject: [PATCH] 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 #39398. Fixes #39309. Fixes #38892. Fixes #38889. Fixes #38887. Fixes #37117. Fixes #8017. --- eng/pipelines/libraries/run-test-job.yml | 5 +--- src/coreclr/src/jit/gentree.h | 8 +++++ src/coreclr/src/jit/importer.cpp | 20 +++++++++---- src/coreclr/src/jit/morph.cpp | 29 +++++++------------ src/coreclr/tests/issues.targets | 3 -- .../tests/ProducerConsumerCollectionTests.cs | 1 - .../System.Drawing.Common/tests/FontTests.cs | 2 -- .../tests/AssemblyInfo.cs | 6 ---- .../System.Net.HttpListener.Tests.csproj | 1 - .../tests/RevocationTests/AiaTests.cs | 1 - src/tests/JIT/opt/OSR/tailrecursetry.csproj | 4 +++ .../unittest/getappdomainstaticaddress.csproj | 4 --- 12 files changed, 38 insertions(+), 46 deletions(-) delete mode 100644 src/libraries/System.Net.HttpListener/tests/AssemblyInfo.cs diff --git a/eng/pipelines/libraries/run-test-job.yml b/eng/pipelines/libraries/run-test-job.yml index faa5ab29e30029..58bcd933f3daff 100644 --- 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 index 7e25365730720c..5c3a94395db6fc 100644 --- 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 index 2823851cd094c6..f03f639431208e 100644 --- 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 index 6ec078e450fc55..2c71bb0b095999 100644 --- 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 index e6c8104ed55712..a2f45d4d140172 100644 --- a/src/coreclr/tests/issues.targets +++ b/src/coreclr/tests/issues.targets @@ -67,9 +67,6 @@ https://github.com/dotnet/runtime/issues/5933 - - https://github.com/dotnet/runtime/issues/8017 - https://github.com/dotnet/runtime/issues/11213 diff --git a/src/libraries/System.Collections.Concurrent/tests/ProducerConsumerCollectionTests.cs b/src/libraries/System.Collections.Concurrent/tests/ProducerConsumerCollectionTests.cs index c6393faa9359b5..ef699764e4214a 100644 --- 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 c = CreateProducerConsumerCollection(); diff --git a/src/libraries/System.Drawing.Common/tests/FontTests.cs b/src/libraries/System.Drawing.Common/tests/FontTests.cs index 067ff89a7be6e8..f731cdb9ddada5 100644 --- 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 deleted file mode 100644 index 5bfc6fc542f551..00000000000000 --- a/src/libraries/System.Net.HttpListener/tests/AssemblyInfo.cs +++ /dev/null @@ -1,6 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using Xunit; - -[assembly: SkipOnCoreClr("https://github.com/dotnet/runtime/issues/39309", RuntimeTestModes.TailcallStress)] 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 index 9da20596f8144d..3d8ebf1e50be37 100644 --- 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 @@ $(NetCoreAppCurrent)-Windows_NT;$(NetCoreAppCurrent)-Unix;$(NetCoreAppCurrent)-Browser;$(NetCoreAppCurrent)-OSX - diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/RevocationTests/AiaTests.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/RevocationTests/AiaTests.cs index d05290b14d4172..bfdb42328a730d 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/RevocationTests/AiaTests.cs +++ b/src/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 index 9620f75474a935..c79a0af1d255af 100644 --- a/src/tests/JIT/opt/OSR/tailrecursetry.csproj +++ b/src/tests/JIT/opt/OSR/tailrecursetry.csproj @@ -3,6 +3,10 @@ Exe True + + true diff --git a/src/tests/profiler/unittest/getappdomainstaticaddress.csproj b/src/tests/profiler/unittest/getappdomainstaticaddress.csproj index c5769302e85908..09f11fb4e52799 100644 --- a/src/tests/profiler/unittest/getappdomainstaticaddress.csproj +++ b/src/tests/profiler/unittest/getappdomainstaticaddress.csproj @@ -6,10 +6,6 @@ true 0 true - - true true