From 395850648ec4d5f7d2f9e172bf19f861181d1a77 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Tue, 5 Aug 2025 11:46:36 -0700 Subject: [PATCH 01/12] [Interpreter] Intrinic handling for Delegate Invoke methods - Add a new opcode - Add a new invocation method. Note that this needs to have the same copy the CallStubHeader logic as calli, as the target is not consistent from use to use --- src/coreclr/interpreter/compiler.cpp | 16 ++++++- src/coreclr/interpreter/intops.def | 1 + src/coreclr/vm/interpexec.cpp | 67 ++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 1 deletion(-) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index 2363738a3f725f..5818bd6f9f1134 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -2707,6 +2707,7 @@ void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool re { uint32_t token = getU4LittleEndian(m_ip + 1); bool isVirtual = (*m_ip == CEE_CALLVIRT); + bool isDelegateInvoke = false; CORINFO_RESOLVED_TOKEN resolvedCallToken; CORINFO_CALL_INFO callInfo; @@ -2758,6 +2759,11 @@ void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool re } } + if (callInfo.methodFlags & CORINFO_FLG_DELEGATE_INVOKE) + { + isDelegateInvoke = true; + } + if (callInfo.thisTransform != CORINFO_NO_THIS_TRANSFORM) { assert(pConstrainedToken != NULL); @@ -3031,7 +3037,15 @@ void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool re // before the call. // TODO: Add null checking behavior somewhere here! } - AddIns((isPInvoke && !isMarshaledPInvoke) ? INTOP_CALL_PINVOKE : INTOP_CALL); + if (isDelegateInvoke) + { + assert(!isPInvoke && !isMarshaledPInvoke); + AddIns(INTOP_CALLDELEGATE); + } + else + { + AddIns((isPInvoke && !isMarshaledPInvoke) ? INTOP_CALL_PINVOKE : INTOP_CALL); + } m_pLastNewIns->data[0] = GetMethodDataItemIndex(callInfo.hMethod); if (isPInvoke && !isMarshaledPInvoke) { diff --git a/src/coreclr/interpreter/intops.def b/src/coreclr/interpreter/intops.def index 0e1d73769a44fa..90083caa3cc7b8 100644 --- a/src/coreclr/interpreter/intops.def +++ b/src/coreclr/interpreter/intops.def @@ -356,6 +356,7 @@ OPDEF(INTOP_LDFLDA, "ldflda", 4, 1, 1, InterpOpInt) // Calls OPDEF(INTOP_CALL, "call", 4, 1, 1, InterpOpMethodHandle) +OPDEF(INTOP_CALLDELEGATE, "call.delegate", 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 diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index 2fb4f73e4f2a69..aa1524c3ce1a32 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -56,6 +56,49 @@ void InvokeCompiledMethod(MethodDesc *pMD, int8_t *pArgs, int8_t *pRet, PCODE ta pHeader->Invoke(pHeader->Routines, pArgs, pRet, pHeader->TotalStackSize); } +void InvokeDelegateInvokeMethod(MethodDesc *pMDDelegateInvoke, int8_t *pArgs, int8_t *pRet, PCODE target) +{ + CONTRACTL + { + THROWS; + MODE_ANY; + PRECONDITION(CheckPointer(pMDDelegateInvoke)); + PRECONDITION(CheckPointer(pArgs)); + PRECONDITION(CheckPointer(pRet)); + } + CONTRACTL_END + + CallStubHeader *stubHeaderTemplate = pMDDelegateInvoke->GetCallStub(); + if (stubHeaderTemplate == NULL) + { + CallStubGenerator callStubGenerator; + GCX_PREEMP(); + + AllocMemTracker amTracker; + stubHeaderTemplate = callStubGenerator.GenerateCallStub(pMDDelegateInvoke, &amTracker, true /* interpreterToNative */); + + if (pMDDelegateInvoke->SetCallStub(stubHeaderTemplate)) + { + amTracker.SuppressRelease(); + } + else + { + // We have lost the race for generating the header, use the one that was generated by another thread + // and let the amTracker release the memory of the one we generated. + stubHeaderTemplate = pMDDelegateInvoke->GetCallStub(); + } + } + + // CallStubHeaders encode their destination addresses in the Routines array, so they need to be + // copied to a local buffer before we can actually set their target address. + size_t templateSize = stubHeaderTemplate->GetSize(); + uint8_t* actualCallStub = (uint8_t*)alloca(templateSize); + memcpy(actualCallStub, stubHeaderTemplate, templateSize); + CallStubHeader *pHeader = (CallStubHeader*)actualCallStub; + pHeader->SetTarget(target); // The method to call + pHeader->Invoke(pHeader->Routines, pArgs, pRet, pHeader->TotalStackSize); +} + void InvokeCalliStub(PCODE ftn, CallStubHeader *stubHeaderTemplate, int8_t *pArgs, int8_t *pRet) { CONTRACTL @@ -1881,6 +1924,30 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr break; } + case INTOP_CALLDELEGATE: + { + returnOffset = ip[1]; + callArgsOffset = ip[2]; + methodSlot = ip[3]; + + // targetMethod holds a pointer to the Invoke method of the delegate, not the final actual target. + targetMethod = (MethodDesc*)pMethod->pDataItems[methodSlot]; + + ip += 4; + + DELEGATEREF delegateObj = LOCAL_VAR(callArgsOffset, DELEGATEREF); + NULL_CHECK(delegateObj); + PCODE targetAddress = delegateObj->GetMethodPtr(); + OBJECTREF targetMethodObj = delegateObj->GetTarget(); + LOCAL_VAR(callArgsOffset, OBJECTREF) = targetMethodObj; + + // TODO! Once we are investigatin performance here, we may way to optimize this so that + // delegate calls to interpeted methods don't have to go through the native invoke here, but for + // now this should work well. + InvokeDelegateInvokeMethod(targetMethod, stack + callArgsOffset, stack + returnOffset, targetAddress); + break; + } + case INTOP_CALL: { returnOffset = ip[1]; From 0b110cffc1f4dea1408a0aa7c37c253274b1a7f7 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Tue, 5 Aug 2025 11:47:37 -0700 Subject: [PATCH 02/12] Update src/coreclr/vm/interpexec.cpp Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/coreclr/vm/interpexec.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index aa1524c3ce1a32..644c593c61b124 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -1941,7 +1941,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr OBJECTREF targetMethodObj = delegateObj->GetTarget(); LOCAL_VAR(callArgsOffset, OBJECTREF) = targetMethodObj; - // TODO! Once we are investigatin performance here, we may way to optimize this so that + // TODO! Once we are investigating performance here, we may want to optimize this so that // delegate calls to interpeted methods don't have to go through the native invoke here, but for // now this should work well. InvokeDelegateInvokeMethod(targetMethod, stack + callArgsOffset, stack + returnOffset, targetAddress); From 232dee5bdc20278b5cbf8c708b6b3f0e0432cf13 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Tue, 5 Aug 2025 13:25:51 -0700 Subject: [PATCH 03/12] Refactor slow path out of function, and make wasm build --- src/coreclr/vm/interpexec.cpp | 66 ++++++++++++++++----------------- src/coreclr/vm/wasm/helpers.cpp | 5 +++ 2 files changed, 38 insertions(+), 33 deletions(-) diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index aa1524c3ce1a32..1770b7e5221065 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -14,9 +14,40 @@ #ifdef TARGET_WASM void InvokeCalliStub(PCODE ftn, CallStubHeader *stubHeaderTemplate, int8_t *pArgs, int8_t *pRet); void InvokeCompiledMethod(MethodDesc *pMD, int8_t *pArgs, int8_t *pRet, PCODE target); +void InvokeDelegateInvokeMethod(MethodDesc *pMDDelegateInvoke, int8_t *pArgs, int8_t *pRet, PCODE target); #else #include "callstubgenerator.h" +CallStubHeader *UpdateCallStubForMethod(MethodDesc *pMD) +{ + CONTRACTL + { + THROWS; + MODE_ANY; + PRECONDITION(CheckPointer(pMD)); + } + CONTRACTL_END + + CallStubGenerator callStubGenerator; + GCX_PREEMP(); + + AllocMemTracker amTracker; + CallStubHeader *header = callStubGenerator.GenerateCallStub(pMD, &amTracker, true /* interpreterToNative */); + + if (pMD->SetCallStub(header)) + { + amTracker.SuppressRelease(); + } + else + { + // We have lost the race for generating the header, use the one that was generated by another thread + // and let the amTracker release the memory of the one we generated. + header = pMD->GetCallStub(); + } + + return header; +} + void InvokeCompiledMethod(MethodDesc *pMD, int8_t *pArgs, int8_t *pRet, PCODE target) { CONTRACTL @@ -32,22 +63,7 @@ void InvokeCompiledMethod(MethodDesc *pMD, int8_t *pArgs, int8_t *pRet, PCODE ta CallStubHeader *pHeader = pMD->GetCallStub(); if (pHeader == NULL) { - CallStubGenerator callStubGenerator; - GCX_PREEMP(); - - AllocMemTracker amTracker; - pHeader = callStubGenerator.GenerateCallStub(pMD, &amTracker, true /* interpreterToNative */); - - if (pMD->SetCallStub(pHeader)) - { - amTracker.SuppressRelease(); - } - else - { - // We have lost the race for generating the header, use the one that was generated by another thread - // and let the amTracker release the memory of the one we generated. - pHeader = pMD->GetCallStub(); - } + pHeader = UpdateCallStubForMethod(pMD); } // Interpreter-FIXME: Potential race condition if a single CallStubHeader is reused for multiple targets. @@ -71,22 +87,7 @@ void InvokeDelegateInvokeMethod(MethodDesc *pMDDelegateInvoke, int8_t *pArgs, in CallStubHeader *stubHeaderTemplate = pMDDelegateInvoke->GetCallStub(); if (stubHeaderTemplate == NULL) { - CallStubGenerator callStubGenerator; - GCX_PREEMP(); - - AllocMemTracker amTracker; - stubHeaderTemplate = callStubGenerator.GenerateCallStub(pMDDelegateInvoke, &amTracker, true /* interpreterToNative */); - - if (pMDDelegateInvoke->SetCallStub(stubHeaderTemplate)) - { - amTracker.SuppressRelease(); - } - else - { - // We have lost the race for generating the header, use the one that was generated by another thread - // and let the amTracker release the memory of the one we generated. - stubHeaderTemplate = pMDDelegateInvoke->GetCallStub(); - } + stubHeaderTemplate = UpdateCallStubForMethod(pMDDelegateInvoke); } // CallStubHeaders encode their destination addresses in the Routines array, so they need to be @@ -127,7 +128,6 @@ CallStubHeader *CreateNativeToInterpreterCallStub(InterpMethod* pInterpMethod) CallStubHeader *pHeader = VolatileLoadWithoutBarrier(&pInterpMethod->pCallStub); GCX_PREEMP(); - AllocMemTracker amTracker; if (pHeader == NULL) { // Ensure that there is an interpreter thread context instance and thus an interpreter stack diff --git a/src/coreclr/vm/wasm/helpers.cpp b/src/coreclr/vm/wasm/helpers.cpp index 7f1fde32c918fc..ac2298f34837a1 100644 --- a/src/coreclr/vm/wasm/helpers.cpp +++ b/src/coreclr/vm/wasm/helpers.cpp @@ -487,3 +487,8 @@ void InvokeCompiledMethod(MethodDesc *pMD, int8_t *pArgs, int8_t *pRet, PCODE ta { PORTABILITY_ASSERT("Attempted to execute non-interpreter code from interpreter on wasm, this is not yet implemented"); } + +void InvokeDelegateInvokeMethod(MethodDesc *pMDDelegateInvoke, int8_t *pArgs, int8_t *pRet, PCODE target) +{ + PORTABILITY_ASSERT("Attempted to execute non-interpreter code from interpreter on wasm, this is not yet implemented"); +} \ No newline at end of file From e04e9788e34d3304cf402a8c93fc2567ebc480cf Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Tue, 5 Aug 2025 15:12:57 -0700 Subject: [PATCH 04/12] Update src/coreclr/vm/interpexec.cpp Co-authored-by: Jan Kotas --- src/coreclr/vm/interpexec.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index 3f707e3b617160..3a40f605e08c8f 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -28,9 +28,10 @@ CallStubHeader *UpdateCallStubForMethod(MethodDesc *pMD) } CONTRACTL_END - CallStubGenerator callStubGenerator; GCX_PREEMP(); + CallStubGenerator callStubGenerator; + AllocMemTracker amTracker; CallStubHeader *header = callStubGenerator.GenerateCallStub(pMD, &amTracker, true /* interpreterToNative */); From 69bc845a227a5e72773294ecd55871dd38538745 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Wed, 6 Aug 2025 16:20:15 +0200 Subject: [PATCH 05/12] 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 06/12] 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 07/12] 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 08/12] 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 09/12] 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 10/12] 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 11/12] 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 From 183e68fdd9ba415152dd9b64e86e7e05d0264984 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Thu, 7 Aug 2025 12:59:23 -0700 Subject: [PATCH 12/12] [clr-interp] Fix issues found in the startup path with the interpreter - Fix virtual function resolution might dispatch to the wrong location - Fix ldelema for an array of reference types doesn't handle shared generics properly - Fix the readonly prefix for ldelema is ignored. --- src/coreclr/interpreter/compiler.cpp | 33 +++++++++++++++++++++------- src/coreclr/interpreter/intops.def | 3 ++- src/coreclr/vm/interpexec.cpp | 33 +++++++++++++++++++++++++--- 3 files changed, 57 insertions(+), 12 deletions(-) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index 3b090273c6a68a..db45085e6f188f 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -3095,7 +3095,7 @@ void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool re break; case CORINFO_VIRTUALCALL_LDVIRTFTN: - if ((callInfo.sig.sigInst.methInstCount != 0) || (m_compHnd->getClassAttribs(resolvedCallToken.hClass) & CORINFO_FLG_SHAREDINST)) + if ((callInfo.sig.sigInst.methInstCount != 0) || (m_compHnd->getMethodAttribs(callInfo.hMethod) & CORINFO_FLG_SHAREDINST)) { assert(extraParamArgLocation == INT_MAX); // We should not have a type argument for the ldvirtftn path since we don't know @@ -5805,17 +5805,34 @@ void InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo) CorInfoType elemCorType = m_compHnd->asCorInfoType(elemClsHnd); m_pStackPointer -= 2; - if (elemCorType == CORINFO_TYPE_CLASS) + if ((elemCorType == CORINFO_TYPE_CLASS) && !readonly) { - AddIns(INTOP_LDELEMA_REF); - m_pLastNewIns->SetSVars2(m_pStackPointer[0].var, m_pStackPointer[1].var); - PushInterpType(InterpTypeByRef, elemClsHnd); - m_pLastNewIns->SetDVar(m_pStackPointer[-1].var); - m_pLastNewIns->data[0] = m_compHnd->getClassSize(elemClsHnd); - m_pLastNewIns->data[1] = GetDataItemIndex(elemClsHnd); + CORINFO_GENERICHANDLE_RESULT embedInfo; + m_compHnd->embedGenericHandle(&resolvedToken, false, m_methodInfo->ftn, &embedInfo); + DeclarePointerIsClass((CORINFO_CLASS_HANDLE)embedInfo.compileTimeHandle); + + if (embedInfo.lookup.lookupKind.needsRuntimeLookup) + { + GenericHandleData handleData = GenericHandleToGenericHandleData(embedInfo); + + AddIns(INTOP_LDELEMA_REF_GENERIC); + m_pLastNewIns->SetSVars3(m_pStackPointer[0].var, m_pStackPointer[1].var, handleData.genericVar); + PushInterpType(InterpTypeByRef, elemClsHnd); + m_pLastNewIns->SetDVar(m_pStackPointer[-1].var); + m_pLastNewIns->data[0] = handleData.dataItemIndex; + } + else + { + AddIns(INTOP_LDELEMA_REF); + m_pLastNewIns->SetSVars2(m_pStackPointer[0].var, m_pStackPointer[1].var); + PushInterpType(InterpTypeByRef, elemClsHnd); + m_pLastNewIns->SetDVar(m_pStackPointer[-1].var); + m_pLastNewIns->data[0] = GetDataItemIndex(elemClsHnd); + } } else { + readonly = false; // If readonly was set, it is no longer needed as its been handled. AddIns(INTOP_LDELEMA); m_pLastNewIns->SetSVars2(m_pStackPointer[0].var, m_pStackPointer[1].var); PushInterpType(InterpTypeByRef, elemClsHnd); diff --git a/src/coreclr/interpreter/intops.def b/src/coreclr/interpreter/intops.def index b88b71129a28c2..95ea7a745e66f6 100644 --- a/src/coreclr/interpreter/intops.def +++ b/src/coreclr/interpreter/intops.def @@ -53,7 +53,8 @@ OPDEF(INTOP_STELEM_VT, "stelem.vt", 6, 0, 3, InterpOpTwoInts) OPDEF(INTOP_STELEM_VT_NOREF, "stelem.vt.noref", 5, 0, 3, InterpOpInt) OPDEF(INTOP_LDELEMA, "ldelema", 5, 1, 2, InterpOpInt) -OPDEF(INTOP_LDELEMA_REF, "ldelema.ref", 6, 1, 2, InterpOpTwoInts) +OPDEF(INTOP_LDELEMA_REF, "ldelema.ref", 5, 1, 2, InterpOpClassHandle) +OPDEF(INTOP_LDELEMA_REF_GENERIC, "ldelema.ref.generic", 6, 1, 3, InterpOpGenericLookup) OPDEF(INTOP_MOV_I4_I1, "mov.i4.i1", 3, 1, 1, InterpOpNoArgs) OPDEF(INTOP_MOV_I4_U1, "mov.i4.u1", 3, 1, 1, InterpOpNoArgs) diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index cbf0d235beff10..1ba9070d5c5858 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -2458,11 +2458,38 @@ do { \ COMPlusThrow(kIndexOutOfRangeException); uint8_t* pData = arr->GetDataPtr(); - size_t elemSize = ip[4]; - void* elemAddr = pData + idx * elemSize; + void* elemAddr = pData + idx * sizeof(void*); MethodTable* arrayElemMT = arr->GetArrayElementTypeHandle().AsMethodTable(); - MethodTable* expectedMT = (MethodTable*)pMethod->pDataItems[ip[5]]; + MethodTable* expectedMT = (MethodTable*)pMethod->pDataItems[ip[4]]; + if (arrayElemMT != expectedMT) + { + COMPlusThrow(kArrayTypeMismatchException); + } + + LOCAL_VAR(ip[1], void*) = elemAddr; + ip += 5; + break; + } + + case INTOP_LDELEMA_REF_GENERIC: + { + BASEARRAYREF arrayRef = LOCAL_VAR(ip[2], BASEARRAYREF); + if (arrayRef == NULL) + COMPlusThrow(kNullReferenceException); + + ArrayBase* arr = (ArrayBase*)OBJECTREFToObject(arrayRef); + uint32_t len = arr->GetNumComponents(); + uint32_t idx = (uint32_t)LOCAL_VAR(ip[3], int32_t); + if (idx >= len) + COMPlusThrow(kIndexOutOfRangeException); + + uint8_t* pData = arr->GetDataPtr(); + void* elemAddr = pData + idx * sizeof(void*); + + MethodTable* arrayElemMT = arr->GetArrayElementTypeHandle().AsMethodTable(); + InterpGenericLookup *pLookup = (InterpGenericLookup*)&pMethod->pDataItems[ip[5]]; + MethodTable* expectedMT = (MethodTable*)DoGenericLookup(LOCAL_VAR(ip[4], void*), pLookup); if (arrayElemMT != expectedMT) { COMPlusThrow(kArrayTypeMismatchException);