From ac3ad369910c140d2e219e7d27742d4e7257bc92 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 15 Jul 2021 15:28:48 +0200 Subject: [PATCH 1/3] Widen return values in CallTailCallTarget stubs In the managed calling convention the callee is responsible for widening the return value up to 4 bytes. We need to mimic this in portable tailcall helpers as the JIT cannot know which call eventually produced the value that was stored in the return value buffer passed to the dispatcher. Fix #55253 --- src/coreclr/jit/morph.cpp | 25 ++++++------- src/coreclr/vm/tailcallhelp.cpp | 35 ++++++++++++++++++- src/coreclr/vm/tailcallhelp.h | 1 + .../JitBlue/Runtime_55253/Runtime_55253.cs | 21 +++++++++++ .../Runtime_55253/Runtime_55253.csproj | 22 ++++++++++++ 5 files changed, 91 insertions(+), 13 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_55253/Runtime_55253.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_55253/Runtime_55253.csproj diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 000103b54c5b0a..a6aabde2233b82 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -8272,10 +8272,9 @@ GenTree* Compiler::fgCreateCallDispatcherAndGetResult(GenTreeCall* orig // void DispatchTailCalls(void* callersRetAddrSlot, void* callTarget, void* retValue) // Add return value arg. - GenTree* retValArg; - GenTree* retVal = nullptr; - unsigned int newRetLcl = BAD_VAR_NUM; - GenTree* copyToRetBufNode = nullptr; + GenTree* retValArg; + GenTree* retVal = nullptr; + GenTree* copyToRetBufNode = nullptr; if (origCall->HasRetBufArg()) { @@ -8326,24 +8325,26 @@ GenTree* Compiler::fgCreateCallDispatcherAndGetResult(GenTreeCall* orig else if (origCall->gtType != TYP_VOID) { JITDUMP("Creating a new temp for the return value\n"); - newRetLcl = lvaGrabTemp(false DEBUGARG("Return value for tail call dispatcher")); + unsigned int newRetLcl = lvaGrabTemp(false DEBUGARG("Return value for tail call dispatcher")); if (varTypeIsStruct(origCall->gtType)) { lvaSetStruct(newRetLcl, origCall->gtRetClsHnd, false); } else { - // Since we pass a reference to the return value to the dispatcher - // we need to use the real return type so we can normalize it on - // load when we return it. - lvaTable[newRetLcl].lvType = (var_types)origCall->gtReturnType; + // It is the responsibility of the call-tail-call-target stub to + // normalize the return value in the same way that we would do it + // when it is stored. It is not possible for us to normalize it as + // we don't know which eventual tailcaller may have written it, and + // the type of that tailcaller could potentially be narrower than + // this call. See TailCallHelp::NeedsReturnWidening. + lvaTable[newRetLcl].lvType = (var_types)origCall->gtType; } lvaSetVarAddrExposed(newRetLcl); - retValArg = - gtNewOperNode(GT_ADDR, TYP_I_IMPL, gtNewLclvNode(newRetLcl, genActualType(lvaTable[newRetLcl].lvType))); - retVal = gtNewLclvNode(newRetLcl, genActualType(lvaTable[newRetLcl].lvType)); + retValArg = gtNewOperNode(GT_ADDR, TYP_I_IMPL, gtNewLclvNode(newRetLcl, lvaTable[newRetLcl].lvType)); + retVal = gtNewLclvNode(newRetLcl, lvaTable[newRetLcl].lvType); if (varTypeIsStruct(origCall->gtType)) { diff --git a/src/coreclr/vm/tailcallhelp.cpp b/src/coreclr/vm/tailcallhelp.cpp index 5cb1789aabf3d5..a0ffe85c870a4e 100644 --- a/src/coreclr/vm/tailcallhelp.cpp +++ b/src/coreclr/vm/tailcallhelp.cpp @@ -254,6 +254,26 @@ TypeHandle TailCallHelp::NormalizeSigType(TypeHandle tyHnd) return tyHnd; } +// The managed calling convention requires that return types are widened up to 4 bytes. +// This needs to be mirrored when storing the return value. +bool TailCallHelp::NeedsReturnWidening(TypeHandle tyHnd, bool* isSigned) +{ + CorElementType ety = tyHnd.GetSignatureCorElementType(); + switch (ety) + { + case ELEMENT_TYPE_I1: + case ELEMENT_TYPE_I2: + *isSigned = true; + return true; + case ELEMENT_TYPE_U1: + case ELEMENT_TYPE_U2: + *isSigned = false; + return true; + default: + return false; + } +} + bool TailCallHelp::GenerateGCDescriptor( MethodDesc* pTargetMD, const ArgBufferLayout& layout, GCRefMapBuilder* builder) { @@ -579,7 +599,20 @@ MethodDesc* TailCallHelp::CreateCallTargetStub(const TailCallInfo& info) pCode->EmitLDARG(ARG_RET_VAL); pCode->EmitLDLOC(resultLcl); - EmitStoreTyHnd(pCode, info.RetTyHnd); + bool isSigned; + if (NeedsReturnWidening(info.RetTyHnd, &isSigned)) + { + if (isSigned) + pCode->EmitCONV_I4(); + else + pCode->EmitCONV_U4(); + + pCode->EmitSTIND_I4(); + } + else + { + EmitStoreTyHnd(pCode, info.RetTyHnd); + } } pCode->EmitRET(); diff --git a/src/coreclr/vm/tailcallhelp.h b/src/coreclr/vm/tailcallhelp.h index 32883076a69201..151874ddb33fc1 100644 --- a/src/coreclr/vm/tailcallhelp.h +++ b/src/coreclr/vm/tailcallhelp.h @@ -30,6 +30,7 @@ class TailCallHelp MetaSig& callSiteSig, MethodDesc* calleeMD, bool storeTarget, bool thisArgByRef, bool hasInstArg, ArgBufferLayout* layout); static TypeHandle NormalizeSigType(TypeHandle tyHnd); + static bool NeedsReturnWidening(TypeHandle tyHnd, bool* isSigned); static bool GenerateGCDescriptor(MethodDesc* pTargetMD, const ArgBufferLayout& values, GCRefMapBuilder* builder); static MethodDesc* CreateStoreArgsStub(TailCallInfo& info); diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_55253/Runtime_55253.cs b/src/tests/JIT/Regression/JitBlue/Runtime_55253/Runtime_55253.cs new file mode 100644 index 00000000000000..7432d9a2918f21 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_55253/Runtime_55253.cs @@ -0,0 +1,21 @@ +class Runtime_55253 +{ + static int Main() + { + int errors = 0; + if (AsInt32() != -1) + errors |= 1; + if (AsUInt32() != 255) + errors |= 2; + + return 100 + errors; + } + + static uint AsUInt32() => AsUInt16(); + static uint AsUInt16() => AsUInt8(); + static uint AsUInt8() => 255; + + static int AsInt32() => AsInt16(); + static short AsInt16() => AsInt8(); + static sbyte AsInt8() => -1; +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_55253/Runtime_55253.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_55253/Runtime_55253.csproj new file mode 100644 index 00000000000000..9d41857634d520 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_55253/Runtime_55253.csproj @@ -0,0 +1,22 @@ + + + Exe + + + None + False + + + + + + + + + From d453d5971be2b3fa2f13204101603e54040b3a03 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 15 Jul 2021 15:40:55 +0200 Subject: [PATCH 2/3] Remove unnecessary cast --- src/coreclr/jit/morph.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index a6aabde2233b82..0bc3fe3ead5ce7 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -8338,7 +8338,7 @@ GenTree* Compiler::fgCreateCallDispatcherAndGetResult(GenTreeCall* orig // we don't know which eventual tailcaller may have written it, and // the type of that tailcaller could potentially be narrower than // this call. See TailCallHelp::NeedsReturnWidening. - lvaTable[newRetLcl].lvType = (var_types)origCall->gtType; + lvaTable[newRetLcl].lvType = origCall->gtType; } lvaSetVarAddrExposed(newRetLcl); From 94326f9eb70e62efc28d90ab1f909093fa4abcfd Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 15 Jul 2021 17:47:50 +0200 Subject: [PATCH 3/3] Normalize bool and char as well --- src/coreclr/vm/tailcallhelp.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/vm/tailcallhelp.cpp b/src/coreclr/vm/tailcallhelp.cpp index a0ffe85c870a4e..208c2b922d4b26 100644 --- a/src/coreclr/vm/tailcallhelp.cpp +++ b/src/coreclr/vm/tailcallhelp.cpp @@ -265,6 +265,8 @@ bool TailCallHelp::NeedsReturnWidening(TypeHandle tyHnd, bool* isSigned) case ELEMENT_TYPE_I2: *isSigned = true; return true; + case ELEMENT_TYPE_BOOLEAN: + case ELEMENT_TYPE_CHAR: case ELEMENT_TYPE_U1: case ELEMENT_TYPE_U2: *isSigned = false;