From 7742b574c02f4e68493aff217fd1c3a957c4ae46 Mon Sep 17 00:00:00 2001 From: Eugene Rozenfeld Date: Fri, 14 Aug 2020 19:33:05 -0700 Subject: [PATCH] Fix TailCallStress mode. (#40698) 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 faa5ab29e3002..58bcd933f3daf 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 7e25365730720..5c3a94395db6f 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 2823851cd094c..f03f639431208 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 ea30779946ef2..b173993d6bb55 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 e6c8104ed5571..a2f45d4d14017 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 c6393faa9359b..ef699764e4214 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 067ff89a7be6e..f731cdb9ddada 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 5bfc6fc542f55..0000000000000 --- 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 9da20596f8144..3d8ebf1e50be3 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 d05290b14d417..bfdb42328a730 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 9620f75474a93..c79a0af1d255a 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 c5769302e8590..09f11fb4e5279 100644 --- a/src/tests/profiler/unittest/getappdomainstaticaddress.csproj +++ b/src/tests/profiler/unittest/getappdomainstaticaddress.csproj @@ -6,10 +6,6 @@ true 0 true - - true true