From 69bc845a227a5e72773294ecd55871dd38538745 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Wed, 6 Aug 2025 16:20:15 +0200 Subject: [PATCH 1/7] Fix few startup path issues with interpreter This change fixes the following issues: * Reverse pinvoke to method marked by UnmanagedCallersOnly attribute. The InterpExecMethod is entered in GC preemptive mode in that case, but it is expected to run in GC cooperative mode. * Pinvoke to method marked by SuppressGCTransition attribute . The pinvoke is entered in GC preemtive mode, but it needs to be called in GC cooperative mode. * Calling a delegate or a similar case when IL stub is used. The interpreter code is set only on the IL stub MethodDesc, but it needs to be set on both the IL stub and the original MethodDesc. --- src/coreclr/vm/interpexec.cpp | 6 +++++- src/coreclr/vm/prestub.cpp | 7 +++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index 3419a19710d3b3..cc8ba3bbc31f98 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -1873,7 +1873,11 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr { GCX_PREEMP(); - InvokeCompiledMethod(targetMethod, stack + callArgsOffset, stack + returnOffset, callTarget); + bool shouldSuppressGCTransition = targetMethod->ShouldSuppressGCTransition(); + { + GCX_MAYBE_COOP(shouldSuppressGCTransition); + InvokeCompiledMethod(targetMethod, stack + callArgsOffset, stack + returnOffset, callTarget); + } } inlinedCallFrame.Pop(); diff --git a/src/coreclr/vm/prestub.cpp b/src/coreclr/vm/prestub.cpp index f9da780c74c417..45c910cdf69ca0 100644 --- a/src/coreclr/vm/prestub.cpp +++ b/src/coreclr/vm/prestub.cpp @@ -1004,6 +1004,9 @@ PCODE MethodDesc::JitCompileCodeLocked(PrepareCodeConfig* pConfig, COR_ILMETHOD_ InterpreterPrecode* pPrecode = InterpreterPrecode::FromEntryPoint(pCode); InterpByteCodeStart* interpreterCode = dac_cast(pPrecode->GetData()->ByteCodeAddr); pConfig->GetMethodDesc()->SetInterpreterCode(interpreterCode); + // The current MethodDesc is different from the pConfig->GetMethodDesc() in case of a call that goes through + // an IL stub. Make sure we set the same interpreter code on both. + SetInterpreterCode(interpreterCode); } #endif // FEATURE_INTERPRETER @@ -2008,6 +2011,10 @@ extern "C" void* STDCALL ExecuteInterpretedMethod(TransitionBlock* pTransitionBl InterpThreadContext *threadContext = GetInterpThreadContext(); int8_t *sp = threadContext->pStackPointer; + InterpByteCodeStart* pInterpreterCode = dac_cast(byteCodeAddr); + + GCX_MAYBE_COOP(((MethodDesc*)pInterpreterCode->Method->methodHnd)->HasUnmanagedCallersOnlyAttribute()); + // This construct ensures that the InterpreterFrame is always stored at a higher address than the // InterpMethodContextFrame. This is important for the stack walking code. struct Frames From 1da66c8f96495c34b725bb8f94a133264027d51e Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Wed, 6 Aug 2025 17:37:20 +0200 Subject: [PATCH 2/7] Check for the SuppressGCTransition in interpreter at compile time --- src/coreclr/interpreter/compiler.cpp | 3 +++ src/coreclr/interpreter/intops.def | 2 +- src/coreclr/vm/interpexec.cpp | 11 ++++------- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index c184a263bf1bf5..58056035782921 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -3041,6 +3041,9 @@ void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool re m_pLastNewIns->data[2] = lookup.accessType == IAT_PVALUE; if (lookup.accessType == IAT_PPVALUE) NO_WAY("IAT_PPVALUE pinvokes not implemented in interpreter"); + bool suppressGCTransition = false; + m_compHnd->getUnmanagedCallConv(callInfo.hMethod, nullptr, &suppressGCTransition); + m_pLastNewIns->data[3] = suppressGCTransition ? 1 : 0; } } break; diff --git a/src/coreclr/interpreter/intops.def b/src/coreclr/interpreter/intops.def index 0861d0f21fbfa7..929312a135a700 100644 --- a/src/coreclr/interpreter/intops.def +++ b/src/coreclr/interpreter/intops.def @@ -358,7 +358,7 @@ OPDEF(INTOP_LDFLDA, "ldflda", 4, 1, 1, InterpOpInt) OPDEF(INTOP_CALL, "call", 4, 1, 1, InterpOpMethodHandle) OPDEF(INTOP_CALLI, "calli", 5, 1, 2, InterpOpLdPtr) OPDEF(INTOP_CALLVIRT, "callvirt", 4, 1, 1, InterpOpMethodHandle) -OPDEF(INTOP_CALL_PINVOKE, "call.pinvoke", 6, 1, 1, InterpOpMethodHandle) // inlined (no marshaling wrapper) pinvokes only +OPDEF(INTOP_CALL_PINVOKE, "call.pinvoke", 7, 1, 1, InterpOpMethodHandle) // inlined (no marshaling wrapper) pinvokes only OPDEF(INTOP_NEWOBJ, "newobj", 5, 1, 1, InterpOpMethodHandle) OPDEF(INTOP_NEWOBJ_GENERIC, "newobj.generic", 6, 1, 2, InterpOpMethodHandle) OPDEF(INTOP_NEWOBJ_VT, "newobj.vt", 5, 1, 1, InterpOpMethodHandle) diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index cc8ba3bbc31f98..9607cb80c32bf3 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -1856,8 +1856,9 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr methodSlot = ip[3]; int32_t targetAddrSlot = ip[4]; int32_t indirectFlag = ip[5]; + bool suppressGCTransition = (ip[6] != 0); - ip += 6; + ip += 7; targetMethod = (MethodDesc*)pMethod->pDataItems[methodSlot]; PCODE callTarget = indirectFlag ? *(PCODE *)pMethod->pDataItems[targetAddrSlot] @@ -1872,12 +1873,8 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr inlinedCallFrame.Push(); { - GCX_PREEMP(); - bool shouldSuppressGCTransition = targetMethod->ShouldSuppressGCTransition(); - { - GCX_MAYBE_COOP(shouldSuppressGCTransition); - InvokeCompiledMethod(targetMethod, stack + callArgsOffset, stack + returnOffset, callTarget); - } + GCX_MAYBE_PREEMP(!suppressGCTransition); + InvokeCompiledMethod(targetMethod, stack + callArgsOffset, stack + returnOffset, callTarget); } inlinedCallFrame.Pop(); From ef5e20b7a8229ea62bd433ba75bc244fa7095e93 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 7 Aug 2025 02:52:26 +0200 Subject: [PATCH 3/7] PR feedback --- src/coreclr/interpreter/compiler.cpp | 12 +++++++++--- src/coreclr/interpreter/interpretershared.h | 11 ++++++++++- src/coreclr/vm/interpexec.cpp | 8 ++++---- src/coreclr/vm/prestub.cpp | 5 +---- 4 files changed, 24 insertions(+), 12 deletions(-) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index 58056035782921..9c2dcaf0194dd8 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -1215,8 +1215,13 @@ InterpMethod* InterpCompiler::CreateInterpMethod() pDataItems[i] = m_dataItems.Get(i); bool initLocals = (m_methodInfo->options & CORINFO_OPT_INIT_LOCALS) != 0; + CORJIT_FLAGS corJitFlags; + DWORD jitFlagsSize = m_compHnd->getJitFlags(&corJitFlags, sizeof(corJitFlags)); + assert(jitFlagsSize == sizeof(corJitFlags)); - InterpMethod *pMethod = new InterpMethod(m_methodHnd, m_totalVarsStackSize, pDataItems, initLocals); + bool unmanagedCallersOnly = corJitFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_REVERSE_PINVOKE); + + InterpMethod *pMethod = new InterpMethod(m_methodHnd, m_totalVarsStackSize, pDataItems, initLocals, unmanagedCallersOnly); return pMethod; } @@ -3038,12 +3043,13 @@ void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool re CORINFO_CONST_LOOKUP lookup; m_compHnd->getAddressOfPInvokeTarget(callInfo.hMethod, &lookup); m_pLastNewIns->data[1] = GetDataItemIndex(lookup.addr); - m_pLastNewIns->data[2] = lookup.accessType == IAT_PVALUE; if (lookup.accessType == IAT_PPVALUE) NO_WAY("IAT_PPVALUE pinvokes not implemented in interpreter"); bool suppressGCTransition = false; m_compHnd->getUnmanagedCallConv(callInfo.hMethod, nullptr, &suppressGCTransition); - m_pLastNewIns->data[3] = suppressGCTransition ? 1 : 0; + m_pLastNewIns->data[2] = + ((lookup.accessType == IAT_PVALUE) ? (int32_t)PInvokeCallFlags::Indirect : 0) | + (suppressGCTransition ? (int32_t)PInvokeCallFlags::SuppressGCTransition : 0); } } break; diff --git a/src/coreclr/interpreter/interpretershared.h b/src/coreclr/interpreter/interpretershared.h index b4bb490237ed6d..d9338543d8ea24 100644 --- a/src/coreclr/interpreter/interpretershared.h +++ b/src/coreclr/interpreter/interpretershared.h @@ -35,8 +35,9 @@ struct InterpMethod // This stub is used for calling the interpreted method from JITted/AOTed code CallStubHeader *pCallStub; bool initLocals; + bool unmanagedCallersOnly; - InterpMethod(CORINFO_METHOD_HANDLE methodHnd, int32_t allocaSize, void** pDataItems, bool initLocals) + InterpMethod(CORINFO_METHOD_HANDLE methodHnd, int32_t allocaSize, void** pDataItems, bool initLocals, bool unmanagedCallersOnly) { #if DEBUG this->self = this; @@ -45,6 +46,7 @@ struct InterpMethod this->allocaSize = allocaSize; this->pDataItems = pDataItems; this->initLocals = initLocals; + this->unmanagedCallersOnly = unmanagedCallersOnly; pCallStub = NULL; } @@ -157,4 +159,11 @@ struct InterpGenericLookup uint16_t offsets[InterpGenericLookup_MaxIndirections]; }; +enum class PInvokeCallFlags : int32_t +{ + None = 0, + Indirect = 1 << 0, // The call target address is indirect + SuppressGCTransition = 1 << 1, // The pinvoke is marked by the SuppressGCTransition attribute +}; + #endif diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index 9607cb80c32bf3..96842ee76587be 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -1855,12 +1855,11 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr callArgsOffset = ip[2]; methodSlot = ip[3]; int32_t targetAddrSlot = ip[4]; - int32_t indirectFlag = ip[5]; - bool suppressGCTransition = (ip[6] != 0); + int32_t flags = ip[5]; ip += 7; targetMethod = (MethodDesc*)pMethod->pDataItems[methodSlot]; - PCODE callTarget = indirectFlag + PCODE callTarget = (flags & (int32_t)PInvokeCallFlags::Indirect) ? *(PCODE *)pMethod->pDataItems[targetAddrSlot] : (PCODE)pMethod->pDataItems[targetAddrSlot]; @@ -1873,7 +1872,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr inlinedCallFrame.Push(); { - GCX_MAYBE_PREEMP(!suppressGCTransition); + GCX_MAYBE_PREEMP(!(flags & (int32_t)PInvokeCallFlags::SuppressGCTransition)); InvokeCompiledMethod(targetMethod, stack + callArgsOffset, stack + returnOffset, callTarget); } @@ -1906,6 +1905,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr pInterpreterFrame->SetTopInterpMethodContextFrame(pFrame); GCX_PREEMP(); // Attempt to setup the interpreter code for the target method. + if (targetMethod->IsIL() || targetMethod->IsNoMetadata()) { targetMethod->PrepareInitialCode(CallerGCMode::Coop); diff --git a/src/coreclr/vm/prestub.cpp b/src/coreclr/vm/prestub.cpp index 45c910cdf69ca0..f02d22ca9cd77b 100644 --- a/src/coreclr/vm/prestub.cpp +++ b/src/coreclr/vm/prestub.cpp @@ -1004,9 +1004,6 @@ PCODE MethodDesc::JitCompileCodeLocked(PrepareCodeConfig* pConfig, COR_ILMETHOD_ InterpreterPrecode* pPrecode = InterpreterPrecode::FromEntryPoint(pCode); InterpByteCodeStart* interpreterCode = dac_cast(pPrecode->GetData()->ByteCodeAddr); pConfig->GetMethodDesc()->SetInterpreterCode(interpreterCode); - // The current MethodDesc is different from the pConfig->GetMethodDesc() in case of a call that goes through - // an IL stub. Make sure we set the same interpreter code on both. - SetInterpreterCode(interpreterCode); } #endif // FEATURE_INTERPRETER @@ -2013,7 +2010,7 @@ extern "C" void* STDCALL ExecuteInterpretedMethod(TransitionBlock* pTransitionBl InterpByteCodeStart* pInterpreterCode = dac_cast(byteCodeAddr); - GCX_MAYBE_COOP(((MethodDesc*)pInterpreterCode->Method->methodHnd)->HasUnmanagedCallersOnlyAttribute()); + GCX_MAYBE_COOP(pInterpreterCode->Method->unmanagedCallersOnly); // This construct ensures that the InterpreterFrame is always stored at a higher address than the // InterpMethodContextFrame. This is important for the stack walking code. From 470f9586997551c1dc4dd702946dad3c0d07b0f6 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 7 Aug 2025 09:49:47 +0200 Subject: [PATCH 4/7] Update src/coreclr/vm/prestub.cpp Co-authored-by: Jan Kotas --- src/coreclr/vm/prestub.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/coreclr/vm/prestub.cpp b/src/coreclr/vm/prestub.cpp index f02d22ca9cd77b..278f1c69ba39f2 100644 --- a/src/coreclr/vm/prestub.cpp +++ b/src/coreclr/vm/prestub.cpp @@ -2010,6 +2010,17 @@ extern "C" void* STDCALL ExecuteInterpretedMethod(TransitionBlock* pTransitionBl InterpByteCodeStart* pInterpreterCode = dac_cast(byteCodeAddr); + if (pInterpreterCode->Method->unmanagedCallersOnly) + { + Thread* thread = GetThreadNULLOk(); + if (thread == NULL) + CREATETHREAD_IF_NULL_FAILFAST(thread, W("Failed to setup new thread during reverse P/Invoke")); + + // Verify the current thread isn't in COOP mode. + if (thread->PreemptiveGCDisabled()) + ReversePInvokeBadTransition(); + } + GCX_MAYBE_COOP(pInterpreterCode->Method->unmanagedCallersOnly); // This construct ensures that the InterpreterFrame is always stored at a higher address than the From fe75f32fdf9f74fc6967399cbd087ab46363a9f0 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 7 Aug 2025 09:49:55 +0200 Subject: [PATCH 5/7] Update src/coreclr/vm/interpexec.cpp Co-authored-by: Jan Kotas --- src/coreclr/vm/interpexec.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index 96842ee76587be..1ca624d119d4d9 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -1905,7 +1905,6 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr pInterpreterFrame->SetTopInterpMethodContextFrame(pFrame); GCX_PREEMP(); // Attempt to setup the interpreter code for the target method. - if (targetMethod->IsIL() || targetMethod->IsNoMetadata()) { targetMethod->PrepareInitialCode(CallerGCMode::Coop); From 1ff7bb3754a828d1d5ce6ab9b7dda9ba1cf98644 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 7 Aug 2025 13:54:40 +0200 Subject: [PATCH 6/7] Undo INTOP_CALL_PINVOKE opcode size bump No longer needed after the previous commit --- src/coreclr/interpreter/intops.def | 2 +- src/coreclr/vm/interpexec.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/interpreter/intops.def b/src/coreclr/interpreter/intops.def index 929312a135a700..0861d0f21fbfa7 100644 --- a/src/coreclr/interpreter/intops.def +++ b/src/coreclr/interpreter/intops.def @@ -358,7 +358,7 @@ OPDEF(INTOP_LDFLDA, "ldflda", 4, 1, 1, InterpOpInt) OPDEF(INTOP_CALL, "call", 4, 1, 1, InterpOpMethodHandle) OPDEF(INTOP_CALLI, "calli", 5, 1, 2, InterpOpLdPtr) OPDEF(INTOP_CALLVIRT, "callvirt", 4, 1, 1, InterpOpMethodHandle) -OPDEF(INTOP_CALL_PINVOKE, "call.pinvoke", 7, 1, 1, InterpOpMethodHandle) // inlined (no marshaling wrapper) pinvokes only +OPDEF(INTOP_CALL_PINVOKE, "call.pinvoke", 6, 1, 1, InterpOpMethodHandle) // inlined (no marshaling wrapper) pinvokes only OPDEF(INTOP_NEWOBJ, "newobj", 5, 1, 1, InterpOpMethodHandle) OPDEF(INTOP_NEWOBJ_GENERIC, "newobj.generic", 6, 1, 2, InterpOpMethodHandle) OPDEF(INTOP_NEWOBJ_VT, "newobj.vt", 5, 1, 1, InterpOpMethodHandle) diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index 1ca624d119d4d9..d81c2a2d0a0b60 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -1857,7 +1857,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr int32_t targetAddrSlot = ip[4]; int32_t flags = ip[5]; - ip += 7; + ip += 6; targetMethod = (MethodDesc*)pMethod->pDataItems[methodSlot]; PCODE callTarget = (flags & (int32_t)PInvokeCallFlags::Indirect) ? *(PCODE *)pMethod->pDataItems[targetAddrSlot] From cd001978695cc437ed36fff2bb69e53266a7944c Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 7 Aug 2025 17:34:37 +0200 Subject: [PATCH 7/7] Fix build break --- src/coreclr/vm/prestub.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/vm/prestub.cpp b/src/coreclr/vm/prestub.cpp index 278f1c69ba39f2..3ea82cfe8ae4d2 100644 --- a/src/coreclr/vm/prestub.cpp +++ b/src/coreclr/vm/prestub.cpp @@ -2001,6 +2001,8 @@ static InterpThreadContext* GetInterpThreadContext() return threadContext; } +EXTERN_C void STDCALL ReversePInvokeBadTransition(); + extern "C" void* STDCALL ExecuteInterpretedMethod(TransitionBlock* pTransitionBlock, TADDR byteCodeAddr, void* retBuff) { // Argument registers are in the TransitionBlock