From 8f7388a2deaf74c645c0bf71fe61330cda1d49c3 Mon Sep 17 00:00:00 2001 From: Bruce Forstall Date: Sun, 7 Mar 2021 16:23:54 -0800 Subject: [PATCH] Fix silent bad codegen VSD possible tailcall converted to normal call The problem was when a VSD interface call returning a multi-byte struct in registers was initially considered to be a tailcall, but the tailcall was abandoned in morph due to not enough stack space to store outgoing arguments, in which case we create a new call return local to store the return struct, and re-morph the call. In doing so, we forget that we had already added VSD non-standard args, and re-add them, leaving the originally added arg as a "normal" arg that shouldn't be there. So, in summary, for a call A->B, to see this failure, we need: 1. The call is considered a potential tailcall (by the importer) 2. The call requires non-standard arguments that add call argument IR in fgInitArgInfo() (e.g., VSD call -- in this case, a generic interface call) 3. We reject the tailcall in fgMorphPotentialTailCall() (e.g., not enough incoming arg stack space in A to store B's outgoing args), in this case because the first arg is a large struct. We can't reject it earlier, due to things like address exposed locals -- we must get far enough through the checks to have called fgInitArgInfo() to add the extra non-standard arg. 4. B returns a struct in multiple registers (e.g., a 16-byte struct in Linux x64 ABI) The fix is to remove the previously-added non-standard VSD argument IR when we are preparing to re-morph a call. There was one other location that was already doing this. I'm a little worried that there are other places where the non-standard added IR is not being considered when we clear out the arg info before remorphing. It seems like there is some risk here. Probably, we should consider a better way of handling the non-standard arg IR given the need in some cases to re-morph calls. I commented out a few cases of: ``` assert(!fgCanFastTailCall(call, nullptr)); ``` because this function can call `fgInitArgInfo` which can alter the IR. That seems dangerous in an assert, which should have any side-effects like modifying the IR. Fixes #49078 No SPMI asm diffs. --- src/coreclr/jit/gentree.cpp | 32 ++++++ src/coreclr/jit/gentree.h | 2 + src/coreclr/jit/morph.cpp | 24 ++-- .../JitBlue/Runtime_49078/Runtime_49078.cs | 105 ++++++++++++++++++ .../Runtime_49078/Runtime_49078.csproj | 14 +++ 5 files changed, 169 insertions(+), 8 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_49078/Runtime_49078.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_49078/Runtime_49078.csproj diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 86303b7c0852b3..ec5352b1338f16 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -1205,6 +1205,38 @@ bool GenTreeCall::Equals(GenTreeCall* c1, GenTreeCall* c2) return true; } +//-------------------------------------------------------------------------- +// ResetArgInfo: The argument info needs to be reset so it can be recomputed based on some change +// in conditions, such as changing the return type of a call due to giving up on doing a tailcall. +// If there is no fgArgInfo computed yet for this call, then there is nothing to reset. +// +void GenTreeCall::ResetArgInfo() +{ + if (fgArgInfo == nullptr) + { + return; + } + + // We would like to just set `fgArgInfo = nullptr`. But `fgInitArgInfo()` not + // only sets up fgArgInfo, it also adds non-standard args to the IR, and we need + // to remove that extra IR so it doesn't get added again. + // + // NOTE: this doesn't handle all possible cases. There might be cases where we + // should be removing non-standard arg IR but currently aren't. + CLANG_FORMAT_COMMENT_ANCHOR; + +#if !defined(TARGET_X86) + if (IsVirtualStub()) + { + JITDUMP("Removing VSD non-standard arg [%06u] to prepare for re-morphing call [%06u]\n", + Compiler::dspTreeID(gtCallArgs->GetNode()), gtTreeID); + gtCallArgs = gtCallArgs->GetNext(); + } +#endif // !defined(TARGET_X86) + + fgArgInfo = nullptr; +} + #if !defined(FEATURE_PUT_STRUCT_ARG_STK) unsigned GenTreePutArgStk::GetStackByteSize() const { diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index b4755049fdfcf5..e4ed857e1322eb 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -4548,6 +4548,8 @@ struct GenTreeCall final : public GenTree return (gtCallMoreFlags & GTF_CALL_M_EXPANDED_EARLY) != 0; } + void ResetArgInfo(); + unsigned gtCallMoreFlags; // in addition to gtFlags unsigned char gtCallType : 3; // value from the gtCallTypes enumeration diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 9122dbf0756d82..c2ee204766075e 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -2456,6 +2456,9 @@ GenTree* Compiler::fgInsertCommaFormTemp(GenTree** ppTree, CORINFO_CLASS_HANDLE // This method only computes the arg table and arg entries for the call (the fgArgInfo), // and makes no modification of the args themselves. // +// The IR for the call args can change for calls with non-standard arguments: some non-standard +// arguments add new call argument IR nodes. +// void Compiler::fgInitArgInfo(GenTreeCall* call) { GenTreeCall::Use* args; @@ -6777,6 +6780,9 @@ void Compiler::fgMorphCallInlineHelper(GenTreeCall* call, InlineResult* result) // This function is target specific and each target will make the fastTailCall // decision differently. See the notes below. // +// This function calls fgInitArgInfo() to initialize the arg info table, which +// is used to analyze the argument. This function can alter the call arguments +// by adding argument IR nodes for non-standard arguments. // // Windows Amd64: // A fast tail call can be made whenever the number of callee arguments @@ -8004,7 +8010,10 @@ GenTree* Compiler::fgMorphTailCallViaHelpers(GenTreeCall* call, CORINFO_TAILCALL // We come this route only for tail prefixed calls that cannot be dispatched as // fast tail calls assert(!call->IsImplicitTailCall()); - assert(!fgCanFastTailCall(call, nullptr)); + + // We want to use the following assert, but it can modify the IR in some cases, so we + // can't do that in an assert. + // assert(!fgCanFastTailCall(call, nullptr)); bool virtualCall = call->IsVirtual(); @@ -8015,11 +8024,7 @@ GenTree* Compiler::fgMorphTailCallViaHelpers(GenTreeCall* call, CORINFO_TAILCALL { JITDUMP("This is a VSD\n"); #if FEATURE_FASTTAILCALL - // fgInitArgInfo has been called from fgCanFastTailCall and it added the stub address - // to the arg list. Remove it now. - call->gtCallArgs = call->gtCallArgs->GetNext(); - // We changed args so recompute info. - call->fgArgInfo = nullptr; + call->ResetArgInfo(); #endif call->gtFlags &= ~GTF_CALL_VIRT_STUB; @@ -8634,7 +8639,10 @@ void Compiler::fgMorphTailCallViaJitHelper(GenTreeCall* call) // We come this route only for tail prefixed calls that cannot be dispatched as // fast tail calls assert(!call->IsImplicitTailCall()); - assert(!fgCanFastTailCall(call, nullptr)); + + // We want to use the following assert, but it can modify the IR in some cases, so we + // can't do that in an assert. + // assert(!fgCanFastTailCall(call, nullptr)); // First move the 'this' pointer (if any) onto the regular arg list. We do this because // we are going to prepend special arguments onto the argument list (for non-x86 platforms), @@ -9136,7 +9144,7 @@ GenTree* Compiler::fgMorphCall(GenTreeCall* call) // ret temp // Force re-evaluating the argInfo as the return argument has changed. - call->fgArgInfo = nullptr; + call->ResetArgInfo(); // Create a new temp. unsigned tmpNum = diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_49078/Runtime_49078.cs b/src/tests/JIT/Regression/JitBlue/Runtime_49078/Runtime_49078.cs new file mode 100644 index 00000000000000..1bbbc9cdb903a5 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_49078/Runtime_49078.cs @@ -0,0 +1,105 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; + +// Regression test for GitHub issue 49078: https://github.com/dotnet/runtime/issues/49078 +// +// The problem was when a VSD interface call returning a multi-byte struct in registers was initially considered +// to be a tailcall, but the tailcall was abandoned in morph due to not enough stack space to store outgoing +// arguments, in which case we create a new call return local to store the return struct, and re-morph the +// call. In doing so, we forget that we had already added VSD non-standard args, and re-add them, leaving +// the originally added arg as a "normal" arg that shouldn't be there. +// +// So, in summary, for a call A->B, to see this failure, we need: +// +// 1. The call is considered a potential tailcall (by the importer) +// 2. The call requires non-standard arguments that add call argument IR in fgInitArgInfo() +// (e.g., VSD call -- in this case, a generic interface call) +// 3. We reject the tailcall in fgMorphPotentialTailCall() (e.g., not enough incoming arg stack space in A +// to store B's outgoing args), in this case because the first arg is a large struct. We can't reject +// it earlier, due to things like address exposed locals -- we must get far enough through the checks +// to have called fgInitArgInfo() to add the extra non-standard arg. +// 4. B returns a struct in multiple registers (e.g., a 16-byte struct in Linux x64 ABI) + +namespace GitHub_49078 +{ + public struct S16 + { + public IntPtr a; + public uint b; + } + + public struct BigStruct + { + public IntPtr a, b, c, d, e, f, g, h, j, k, l, m; + + public BigStruct(IntPtr a1) + { + a = b = c = d = e = f = g = h = j = k = l = m = a1; + } + } + + public interface IFoo + { + public S16 Foo(BigStruct b, int i, int j); + } + + public class CFoo : IFoo + { + public S16 Foo(BigStruct b, int i, int j) + { + S16 s16; + s16.a = (IntPtr)i; + s16.b = (uint)j; + return s16; + } + } + + class Test + { + IFoo m_if = new CFoo(); + BigStruct m_bs = new BigStruct((IntPtr)1); + + public S16 Caller(int a) + { + // Add some computation so this is not inlineable (but don't mark it noinline, + // which would prevent the tailcall consideration). + int i = 7; + try + { + for (int j = 0; j < a; j++) + { + i += j; + } + } + finally + { + i += 2; + } + + return m_if.Foo(m_bs, i, a); + } + } + + class Program + { + static int Main(string[] args) + { + Test t = new Test(); + S16 s = t.Caller(4); + long l = (long)s.a + s.b; + if (l == 19) + { + Console.WriteLine("Passed"); + return 100; + } + else + { + Console.WriteLine($"{s.a}, {s.b}, {l}"); + Console.WriteLine("Failed"); + return 101; + } + } + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_49078/Runtime_49078.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_49078/Runtime_49078.csproj new file mode 100644 index 00000000000000..d6abcf55adaad5 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_49078/Runtime_49078.csproj @@ -0,0 +1,14 @@ + + + Exe + 1 + + + true + None + True + + + + +