From 4c13e83dc6473270737315dd369d4564c47a9097 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 4 Sep 2025 19:35:36 +0200 Subject: [PATCH 1/6] Implement interpreter PInvoke Calli The Calli implementation was missing support for the case when the targer is a pinvoke. That resulted in errors due to incorrect GC mode. This change fixes that. --- src/coreclr/interpreter/compiler.cpp | 22 +++++++----- src/coreclr/interpreter/compiler.h | 1 + src/coreclr/interpreter/inc/intops.def | 4 +-- src/coreclr/vm/interpexec.cpp | 50 ++++++++++++++++++++++++-- src/coreclr/vm/wasm/helpers.cpp | 5 +++ 5 files changed, 68 insertions(+), 14 deletions(-) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index d61bf5fbe81840..c9104ca927ec31 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -2854,6 +2854,16 @@ void InterpCompiler::EmitPushLdvirtftn(int thisVar, CORINFO_RESOLVED_TOKEN* pRes m_pLastNewIns->info.pCallInfo->pCallArgs = callArgs; } +void InterpCompiler::EmitCalli(bool isTailCall, void* calliCookie, int callIFunctionPointerVar, CorInfoCallConv callConv) +{ + AddIns(isTailCall ? INTOP_CALLI_TAIL : INTOP_CALLI); + m_pLastNewIns->data[0] = GetDataItemIndex(calliCookie); + // data[1] is set to 1 if the calli is calling a pinvoke, 0 otherwise + m_pLastNewIns->data[1] = (callConv == CORINFO_CALLCONV_DEFAULT || callConv == CORINFO_CALLCONV_VARARG) ? 0 : 1; + m_pLastNewIns->SetSVars2(CALL_ARGS_SVAR, callIFunctionPointerVar); + +} + void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool readonly, bool tailcall, bool newObj, bool isCalli) { uint32_t token = getU4LittleEndian(m_ip + 1); @@ -3259,9 +3269,7 @@ void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool re } else if (isCalli) { - AddIns(tailcall ? INTOP_CALLI_TAIL : INTOP_CALLI); - m_pLastNewIns->data[0] = GetDataItemIndex(calliCookie); - m_pLastNewIns->SetSVars2(CALL_ARGS_SVAR, callIFunctionPointerVar); + EmitCalli(tailcall, calliCookie, callIFunctionPointerVar, callInfo.sig.getCallConv()); } else { @@ -3318,9 +3326,7 @@ void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool re calliCookie = m_compHnd->GetCookieForInterpreterCalliSig(&callInfo.sig); - AddIns(tailcall ? INTOP_CALLI_TAIL : INTOP_CALLI); - m_pLastNewIns->data[0] = GetDataItemIndex(calliCookie); - m_pLastNewIns->SetSVars2(CALL_ARGS_SVAR, codePointerLookupResult); + EmitCalli(tailcall, calliCookie, codePointerLookupResult, callInfo.sig.getCallConv()); break; } case CORINFO_VIRTUALCALL_VTABLE: @@ -3353,9 +3359,7 @@ void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool re calliCookie = m_compHnd->GetCookieForInterpreterCalliSig(&callInfo.sig); - AddIns(tailcall ? INTOP_CALLI_TAIL : INTOP_CALLI); - m_pLastNewIns->data[0] = GetDataItemIndex(calliCookie); - m_pLastNewIns->SetSVars2(CALL_ARGS_SVAR, synthesizedLdvirtftnPtrVar); + EmitCalli(tailcall, calliCookie, synthesizedLdvirtftnPtrVar, callInfo.sig.getCallConv()); } else { diff --git a/src/coreclr/interpreter/compiler.h b/src/coreclr/interpreter/compiler.h index f8c63336cea119..ef9ce28742ddae 100644 --- a/src/coreclr/interpreter/compiler.h +++ b/src/coreclr/interpreter/compiler.h @@ -711,6 +711,7 @@ class InterpCompiler void EmitShiftOp(int32_t opBase); void EmitCompareOp(int32_t opBase); void EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool readonly, bool tailcall, bool newObj, bool isCalli); + void EmitCalli(bool isTailCall, void* calliCookie, int callIFunctionPointerVar, CorInfoCallConv callConv); bool EmitNamedIntrinsicCall(NamedIntrinsic ni, CORINFO_CLASS_HANDLE clsHnd, CORINFO_METHOD_HANDLE method, CORINFO_SIG_INFO sig); void EmitLdind(InterpType type, CORINFO_CLASS_HANDLE clsHnd, int32_t offset); void EmitStind(InterpType type, CORINFO_CLASS_HANDLE clsHnd, int32_t offset, bool reverseSVarOrder); diff --git a/src/coreclr/interpreter/inc/intops.def b/src/coreclr/interpreter/inc/intops.def index 3a6b9754d8b46e..f8ca4befbc9a42 100644 --- a/src/coreclr/interpreter/inc/intops.def +++ b/src/coreclr/interpreter/inc/intops.def @@ -358,7 +358,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_CALLI, "calli", 6, 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_NEWOBJ, "newobj", 5, 1, 1, InterpOpMethodHandle) @@ -367,7 +367,7 @@ OPDEF(INTOP_NEWOBJ_VT, "newobj.vt", 5, 1, 1, InterpOpMethodHandle) // Tail calls OPDEF(INTOP_CALL_TAIL, "call.tail", 4, 1, 1, InterpOpMethodHandle) -OPDEF(INTOP_CALLI_TAIL, "calli", 5, 1, 2, InterpOpLdPtr) +OPDEF(INTOP_CALLI_TAIL, "calli.tail", 6, 1, 2, InterpOpLdPtr) OPDEF(INTOP_CALLVIRT_TAIL, "callvirt.tail", 4, 1, 1, InterpOpMethodHandle) // The following helper call instructions exist in 2 variants, one for normal methods, and one for cases where a shared generic lookup is needed. diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index 040e366456a2b9..8e66a634a8bc19 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -14,7 +14,8 @@ // Call invoker helpers provided by platform. void InvokeManagedMethod(MethodDesc *pMD, int8_t *pArgs, int8_t *pRet, PCODE target); void InvokeUnmanagedMethod(MethodDesc *targetMethod, int8_t *stack, InterpMethodContextFrame *pFrame, int32_t callArgsOffset, int32_t returnOffset, PCODE callTarget); -void InvokeCalliStub(PCODE ftn, void* stubHeaderTemplate, int8_t *pArgs, int8_t *pRet); +void InvokeCalliStub(PCODE ftn, void* cookie, int8_t *pArgs, int8_t *pRet); +void InvokePInvokeCalliStub(PCODE ftn, void *cookie, int8_t *stack, InterpMethodContextFrame *pFrame, int8_t *pArgs, int8_t *pRet); void InvokeDelegateInvokeMethod(MethodDesc *pMDDelegateInvoke, int8_t *pArgs, int8_t *pRet, PCODE target); // Use the NOINLINE to ensure that the InlinedCallFrame in this method is a lower stack address than any InterpMethodContextFrame values. @@ -129,6 +130,40 @@ void InvokeDelegateInvokeMethod(MethodDesc *pMDDelegateInvoke, int8_t *pArgs, in pHeader->Invoke(pHeader->Routines, pArgs, pRet, pHeader->TotalStackSize); } +void InvokePInvokeCalliStub(PCODE ftn, void *cookie, int8_t *stack, InterpMethodContextFrame *pFrame, int8_t *pArgs, int8_t *pRet) +{ + CONTRACTL + { + THROWS; + MODE_ANY; + PRECONDITION(CheckPointer((void*)ftn)); + PRECONDITION(CheckPointer(stubHeaderTemplate)); + } + CONTRACTL_END + + // 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. + CallStubHeader* stubHeaderTemplate = (CallStubHeader*)cookie; + size_t templateSize = stubHeaderTemplate->GetSize(); + uint8_t* actualCallStub = (uint8_t*)alloca(templateSize); + memcpy(actualCallStub, stubHeaderTemplate, templateSize); + CallStubHeader *pHeader = (CallStubHeader*)actualCallStub; + pHeader->SetTarget(ftn); // The method to call + + InlinedCallFrame inlinedCallFrame; + inlinedCallFrame.m_pCallerReturnAddress = (TADDR)pFrame->ip; + inlinedCallFrame.m_pCallSiteSP = pFrame; + inlinedCallFrame.m_pCalleeSavedFP = (TADDR)stack; + inlinedCallFrame.m_pThread = GetThread(); + inlinedCallFrame.m_Datum = NULL; + inlinedCallFrame.Push(); + { + GCX_PREEMP(); + pHeader->Invoke(pHeader->Routines, pArgs, pRet, pHeader->TotalStackSize); + } + inlinedCallFrame.Pop(); +} + void InvokeCalliStub(PCODE ftn, void *cookie, int8_t *pArgs, int8_t *pRet) { CONTRACTL @@ -2117,15 +2152,24 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr callArgsOffset = ip[2]; int32_t calliFunctionPointerVar = ip[3]; int32_t calliCookie = ip[4]; + bool isPInvoke = ip[5] != 0; void* cookie = pMethod->pDataItems[calliCookie]; - ip += 5; + ip += 6; // Save current execution state for when we return from called method pFrame->ip = ip; // Interpreter-FIXME: isTailcall - InvokeCalliStub(LOCAL_VAR(calliFunctionPointerVar, PCODE), cookie, stack + callArgsOffset, stack + returnOffset); + if (isPInvoke) + { + InvokePInvokeCalliStub(LOCAL_VAR(calliFunctionPointerVar, PCODE), cookie, stack, pFrame, stack + callArgsOffset, stack + returnOffset); + } + else + { + InvokeCalliStub(LOCAL_VAR(calliFunctionPointerVar, PCODE), cookie, stack + callArgsOffset, stack + returnOffset); + } + break; } diff --git a/src/coreclr/vm/wasm/helpers.cpp b/src/coreclr/vm/wasm/helpers.cpp index a85456b76413d5..1d3964c5e8292b 100644 --- a/src/coreclr/vm/wasm/helpers.cpp +++ b/src/coreclr/vm/wasm/helpers.cpp @@ -499,6 +499,11 @@ void InvokeCalliStub(PCODE ftn, void* cookie, int8_t *pArgs, int8_t *pRet) ((void(*)(PCODE, int8_t*, int8_t*))cookie)(ftn, pArgs, pRet); } +void InvokePInvokeCalliStub(PCODE ftn, void *cookie, int8_t *stack, InterpMethodContextFrame *pFrame, int8_t *pArgs, int8_t *pRet) +{ + PORTABILITY_ASSERT("InvokePInvokeCalliStub is not implemented on wasm"); +} + 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"); From 77a8c3931e0b74641b2c769a0e654328571601ce Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Fri, 5 Sep 2025 10:18:34 +0200 Subject: [PATCH 2/6] Add handling of pInvokeMarshallingRequired and suppressGCTransition --- src/coreclr/interpreter/compiler.cpp | 17 +++++++++++------ src/coreclr/interpreter/compiler.h | 2 +- src/coreclr/interpreter/inc/interpretershared.h | 8 ++++++++ src/coreclr/vm/interpexec.cpp | 4 ++-- 4 files changed, 22 insertions(+), 9 deletions(-) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index c9104ca927ec31..776ae807986690 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -2854,14 +2854,19 @@ void InterpCompiler::EmitPushLdvirtftn(int thisVar, CORINFO_RESOLVED_TOKEN* pRes m_pLastNewIns->info.pCallInfo->pCallArgs = callArgs; } -void InterpCompiler::EmitCalli(bool isTailCall, void* calliCookie, int callIFunctionPointerVar, CorInfoCallConv callConv) +void InterpCompiler::EmitCalli(bool isTailCall, void* calliCookie, int callIFunctionPointerVar, CORINFO_SIG_INFO* callSiteSig) { AddIns(isTailCall ? INTOP_CALLI_TAIL : INTOP_CALLI); m_pLastNewIns->data[0] = GetDataItemIndex(calliCookie); // data[1] is set to 1 if the calli is calling a pinvoke, 0 otherwise - m_pLastNewIns->data[1] = (callConv == CORINFO_CALLCONV_DEFAULT || callConv == CORINFO_CALLCONV_VARARG) ? 0 : 1; + bool suppressGCTransition = false; + m_compHnd->getUnmanagedCallConv(nullptr, callSiteSig, &suppressGCTransition); + bool isPInvoke = (callSiteSig->callConv != CORINFO_CALLCONV_DEFAULT && callSiteSig->callConv != CORINFO_CALLCONV_VARARG); + bool isPInvokeMarshalled = isPInvoke && m_compHnd->pInvokeMarshalingRequired(NULL, callSiteSig); + m_pLastNewIns->data[1] = (suppressGCTransition ? (int32_t)CalliFlags::SuppressGCTransition : 0) | + (isPInvoke ? (int32_t)CalliFlags::PInvoke : 0) | + (isPInvokeMarshalled ? (int32_t)CalliFlags::PInvokeMarshalled : 0); m_pLastNewIns->SetSVars2(CALL_ARGS_SVAR, callIFunctionPointerVar); - } void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool readonly, bool tailcall, bool newObj, bool isCalli) @@ -3269,7 +3274,7 @@ void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool re } else if (isCalli) { - EmitCalli(tailcall, calliCookie, callIFunctionPointerVar, callInfo.sig.getCallConv()); + EmitCalli(tailcall, calliCookie, callIFunctionPointerVar, &callInfo.sig); } else { @@ -3326,7 +3331,7 @@ void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool re calliCookie = m_compHnd->GetCookieForInterpreterCalliSig(&callInfo.sig); - EmitCalli(tailcall, calliCookie, codePointerLookupResult, callInfo.sig.getCallConv()); + EmitCalli(tailcall, calliCookie, codePointerLookupResult, &callInfo.sig); break; } case CORINFO_VIRTUALCALL_VTABLE: @@ -3359,7 +3364,7 @@ void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool re calliCookie = m_compHnd->GetCookieForInterpreterCalliSig(&callInfo.sig); - EmitCalli(tailcall, calliCookie, synthesizedLdvirtftnPtrVar, callInfo.sig.getCallConv()); + EmitCalli(tailcall, calliCookie, synthesizedLdvirtftnPtrVar, &callInfo.sig); } else { diff --git a/src/coreclr/interpreter/compiler.h b/src/coreclr/interpreter/compiler.h index ef9ce28742ddae..6c7144ec3dc4d5 100644 --- a/src/coreclr/interpreter/compiler.h +++ b/src/coreclr/interpreter/compiler.h @@ -711,7 +711,7 @@ class InterpCompiler void EmitShiftOp(int32_t opBase); void EmitCompareOp(int32_t opBase); void EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool readonly, bool tailcall, bool newObj, bool isCalli); - void EmitCalli(bool isTailCall, void* calliCookie, int callIFunctionPointerVar, CorInfoCallConv callConv); + void EmitCalli(bool isTailCall, void* calliCookie, int callIFunctionPointerVar, CORINFO_SIG_INFO* callSiteSig); bool EmitNamedIntrinsicCall(NamedIntrinsic ni, CORINFO_CLASS_HANDLE clsHnd, CORINFO_METHOD_HANDLE method, CORINFO_SIG_INFO sig); void EmitLdind(InterpType type, CORINFO_CLASS_HANDLE clsHnd, int32_t offset); void EmitStind(InterpType type, CORINFO_CLASS_HANDLE clsHnd, int32_t offset, bool reverseSVarOrder); diff --git a/src/coreclr/interpreter/inc/interpretershared.h b/src/coreclr/interpreter/inc/interpretershared.h index 74af7e8ab0ee3a..b914356f8ebc99 100644 --- a/src/coreclr/interpreter/inc/interpretershared.h +++ b/src/coreclr/interpreter/inc/interpretershared.h @@ -171,4 +171,12 @@ enum class PInvokeCallFlags : int32_t SuppressGCTransition = 1 << 1, // The pinvoke is marked by the SuppressGCTransition attribute }; +enum class CalliFlags : int32_t +{ + None = 0, + SuppressGCTransition = 1 << 1, // The call is marked by the SuppressGCTransition attribute + PInvoke = 1 << 2, // The call is a PInvoke call + PInvokeMarshalled = 1 << 3, // The call is a PInvoke call with marshaling +}; + #endif diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index 8e66a634a8bc19..e3f2d380486fa0 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -2152,7 +2152,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr callArgsOffset = ip[2]; int32_t calliFunctionPointerVar = ip[3]; int32_t calliCookie = ip[4]; - bool isPInvoke = ip[5] != 0; + int32_t flags = ip[5]; void* cookie = pMethod->pDataItems[calliCookie]; ip += 6; @@ -2161,7 +2161,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr pFrame->ip = ip; // Interpreter-FIXME: isTailcall - if (isPInvoke) + if ((flags & (int32_t)CalliFlags::PInvoke) && !(flags & (int32_t)CalliFlags::SuppressGCTransition) && !(flags & (int32_t)CalliFlags::PInvokeMarshalled)) { InvokePInvokeCalliStub(LOCAL_VAR(calliFunctionPointerVar, PCODE), cookie, stack, pFrame, stack + callArgsOffset, stack + returnOffset); } From 137d62b5867f4762c1c25e86d4bbcaf46701392c Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Fri, 5 Sep 2025 15:41:07 +0200 Subject: [PATCH 3/6] Fix argument name --- 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 e3f2d380486fa0..ea519b5eb41940 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -137,7 +137,7 @@ void InvokePInvokeCalliStub(PCODE ftn, void *cookie, int8_t *stack, InterpMethod THROWS; MODE_ANY; PRECONDITION(CheckPointer((void*)ftn)); - PRECONDITION(CheckPointer(stubHeaderTemplate)); + PRECONDITION(CheckPointer(cookie)); } CONTRACTL_END From 720ec76deee6d73c12ded0e0b96d36a63015b20f Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Mon, 8 Sep 2025 15:53:46 +0200 Subject: [PATCH 4/6] PR feedback * Disable calli with marshalled pinvokes * Refactor the target invocation methods --- src/coreclr/interpreter/compiler.cpp | 8 ++-- .../interpreter/inc/interpretershared.h | 1 - src/coreclr/vm/interpexec.cpp | 39 +++++++++++++++++-- 3 files changed, 40 insertions(+), 8 deletions(-) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index 776ae807986690..b076617d43dd8e 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -2862,10 +2862,12 @@ void InterpCompiler::EmitCalli(bool isTailCall, void* calliCookie, int callIFunc bool suppressGCTransition = false; m_compHnd->getUnmanagedCallConv(nullptr, callSiteSig, &suppressGCTransition); bool isPInvoke = (callSiteSig->callConv != CORINFO_CALLCONV_DEFAULT && callSiteSig->callConv != CORINFO_CALLCONV_VARARG); - bool isPInvokeMarshalled = isPInvoke && m_compHnd->pInvokeMarshalingRequired(NULL, callSiteSig); + if (isPInvoke && m_compHnd->pInvokeMarshalingRequired(NULL, callSiteSig)) + { + BADCODE("PInvoke marshalling is not supported in interpreted code"); + } m_pLastNewIns->data[1] = (suppressGCTransition ? (int32_t)CalliFlags::SuppressGCTransition : 0) | - (isPInvoke ? (int32_t)CalliFlags::PInvoke : 0) | - (isPInvokeMarshalled ? (int32_t)CalliFlags::PInvokeMarshalled : 0); + (isPInvoke ? (int32_t)CalliFlags::PInvoke : 0); m_pLastNewIns->SetSVars2(CALL_ARGS_SVAR, callIFunctionPointerVar); } diff --git a/src/coreclr/interpreter/inc/interpretershared.h b/src/coreclr/interpreter/inc/interpretershared.h index b914356f8ebc99..d9e79f3bffea57 100644 --- a/src/coreclr/interpreter/inc/interpretershared.h +++ b/src/coreclr/interpreter/inc/interpretershared.h @@ -176,7 +176,6 @@ enum class CalliFlags : int32_t None = 0, SuppressGCTransition = 1 << 1, // The call is marked by the SuppressGCTransition attribute PInvoke = 1 << 2, // The call is a PInvoke call - PInvokeMarshalled = 1 << 3, // The call is a PInvoke call with marshaling }; #endif diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index ea519b5eb41940..a80bc2b1927e6f 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -15,7 +15,8 @@ void InvokeManagedMethod(MethodDesc *pMD, int8_t *pArgs, int8_t *pRet, PCODE target); void InvokeUnmanagedMethod(MethodDesc *targetMethod, int8_t *stack, InterpMethodContextFrame *pFrame, int32_t callArgsOffset, int32_t returnOffset, PCODE callTarget); void InvokeCalliStub(PCODE ftn, void* cookie, int8_t *pArgs, int8_t *pRet); -void InvokePInvokeCalliStub(PCODE ftn, void *cookie, int8_t *stack, InterpMethodContextFrame *pFrame, int8_t *pArgs, int8_t *pRet); +void InvokeUnmanagedCalli(PCODE ftn, void *cookie, int8_t *pArgs, int8_t *pRet); +void InvokeUnmanagedCalliWithTransition(PCODE ftn, void *cookie, int8_t *stack, InterpMethodContextFrame *pFrame, int8_t *pArgs, int8_t *pRet); void InvokeDelegateInvokeMethod(MethodDesc *pMDDelegateInvoke, int8_t *pArgs, int8_t *pRet, PCODE target); // Use the NOINLINE to ensure that the InlinedCallFrame in this method is a lower stack address than any InterpMethodContextFrame values. @@ -130,7 +131,7 @@ void InvokeDelegateInvokeMethod(MethodDesc *pMDDelegateInvoke, int8_t *pArgs, in pHeader->Invoke(pHeader->Routines, pArgs, pRet, pHeader->TotalStackSize); } -void InvokePInvokeCalliStub(PCODE ftn, void *cookie, int8_t *stack, InterpMethodContextFrame *pFrame, int8_t *pArgs, int8_t *pRet) +void InvokeUnmanagedCalliWithTransition(PCODE ftn, void *cookie, int8_t *stack, InterpMethodContextFrame *pFrame, int8_t *pArgs, int8_t *pRet) { CONTRACTL { @@ -164,6 +165,29 @@ void InvokePInvokeCalliStub(PCODE ftn, void *cookie, int8_t *stack, InterpMethod inlinedCallFrame.Pop(); } +void InvokeUnmanagedCalli(PCODE ftn, void *cookie, int8_t *pArgs, int8_t *pRet) +{ + CONTRACTL + { + THROWS; + MODE_ANY; + PRECONDITION(CheckPointer((void*)ftn)); + PRECONDITION(CheckPointer(cookie)); + } + CONTRACTL_END + + // 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. + CallStubHeader* stubHeaderTemplate = (CallStubHeader*)cookie; + size_t templateSize = stubHeaderTemplate->GetSize(); + uint8_t* actualCallStub = (uint8_t*)alloca(templateSize); + memcpy(actualCallStub, stubHeaderTemplate, templateSize); + CallStubHeader *pHeader = (CallStubHeader*)actualCallStub; + pHeader->SetTarget(ftn); // The method to call + + pHeader->Invoke(pHeader->Routines, pArgs, pRet, pHeader->TotalStackSize); +} + void InvokeCalliStub(PCODE ftn, void *cookie, int8_t *pArgs, int8_t *pRet) { CONTRACTL @@ -2161,9 +2185,16 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr pFrame->ip = ip; // Interpreter-FIXME: isTailcall - if ((flags & (int32_t)CalliFlags::PInvoke) && !(flags & (int32_t)CalliFlags::SuppressGCTransition) && !(flags & (int32_t)CalliFlags::PInvokeMarshalled)) + if (flags & (int32_t)CalliFlags::PInvoke) { - InvokePInvokeCalliStub(LOCAL_VAR(calliFunctionPointerVar, PCODE), cookie, stack, pFrame, stack + callArgsOffset, stack + returnOffset); + if (flags & (int32_t)CalliFlags::SuppressGCTransition) + { + InvokeUnmanagedCalli(LOCAL_VAR(calliFunctionPointerVar, PCODE), cookie, stack + callArgsOffset, stack + returnOffset); + } + else + { + InvokeUnmanagedCalliWithTransition(LOCAL_VAR(calliFunctionPointerVar, PCODE), cookie, stack, pFrame, stack + callArgsOffset, stack + returnOffset); + } } else { From 3302703a6f69fe1ac20a24ec5156baca2644311b Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Mon, 8 Sep 2025 17:56:49 +0200 Subject: [PATCH 5/6] PR feedback and some unification of arguments --- src/coreclr/interpreter/compiler.cpp | 2 +- src/coreclr/vm/interpexec.cpp | 75 ++++++++++++---------------- src/coreclr/vm/wasm/helpers.cpp | 6 +-- 3 files changed, 37 insertions(+), 46 deletions(-) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index b076617d43dd8e..bf747e3d06318d 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -2864,7 +2864,7 @@ void InterpCompiler::EmitCalli(bool isTailCall, void* calliCookie, int callIFunc bool isPInvoke = (callSiteSig->callConv != CORINFO_CALLCONV_DEFAULT && callSiteSig->callConv != CORINFO_CALLCONV_VARARG); if (isPInvoke && m_compHnd->pInvokeMarshalingRequired(NULL, callSiteSig)) { - BADCODE("PInvoke marshalling is not supported in interpreted code"); + BADCODE("PInvoke marshalling for calli is not supported in interpreted code"); } m_pLastNewIns->data[1] = (suppressGCTransition ? (int32_t)CalliFlags::SuppressGCTransition : 0) | (isPInvoke ? (int32_t)CalliFlags::PInvoke : 0); diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index a80bc2b1927e6f..e5bd23c538a2cf 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -13,15 +13,14 @@ // Call invoker helpers provided by platform. void InvokeManagedMethod(MethodDesc *pMD, int8_t *pArgs, int8_t *pRet, PCODE target); -void InvokeUnmanagedMethod(MethodDesc *targetMethod, int8_t *stack, InterpMethodContextFrame *pFrame, int32_t callArgsOffset, int32_t returnOffset, PCODE callTarget); +void InvokeUnmanagedMethod(MethodDesc *targetMethod, int8_t *pArgs, int8_t *pRet, PCODE callTarget); void InvokeCalliStub(PCODE ftn, void* cookie, int8_t *pArgs, int8_t *pRet); void InvokeUnmanagedCalli(PCODE ftn, void *cookie, int8_t *pArgs, int8_t *pRet); -void InvokeUnmanagedCalliWithTransition(PCODE ftn, void *cookie, int8_t *stack, InterpMethodContextFrame *pFrame, int8_t *pArgs, int8_t *pRet); void InvokeDelegateInvokeMethod(MethodDesc *pMDDelegateInvoke, int8_t *pArgs, int8_t *pRet, PCODE target); // Use the NOINLINE to ensure that the InlinedCallFrame in this method is a lower stack address than any InterpMethodContextFrame values. NOINLINE -void InvokeUnmanagedMethodWithTransition(MethodDesc *targetMethod, int8_t *stack, InterpMethodContextFrame *pFrame, int32_t callArgsOffset, int32_t returnOffset, PCODE callTarget) +void InvokeUnmanagedMethodWithTransition(MethodDesc *targetMethod, int8_t *stack, InterpMethodContextFrame *pFrame, int8_t *pArgs, int8_t *pRet, PCODE callTarget) { InlinedCallFrame inlinedCallFrame; inlinedCallFrame.m_pCallerReturnAddress = (TADDR)pFrame->ip; @@ -34,12 +33,38 @@ void InvokeUnmanagedMethodWithTransition(MethodDesc *targetMethod, int8_t *stack { GCX_PREEMP(); // WASM-TODO: Handle unmanaged calling conventions - InvokeManagedMethod(targetMethod, stack + callArgsOffset, stack + returnOffset, callTarget); + InvokeManagedMethod(targetMethod, pArgs, pRet, callTarget); } inlinedCallFrame.Pop(); } +NOINLINE +void InvokeUnmanagedCalliWithTransition(PCODE ftn, void *cookie, int8_t *stack, InterpMethodContextFrame *pFrame, int8_t *pArgs, int8_t *pRet) +{ + CONTRACTL + { + THROWS; + MODE_COOPERATIVE; + PRECONDITION(CheckPointer((void*)ftn)); + PRECONDITION(CheckPointer(cookie)); + } + CONTRACTL_END + + InlinedCallFrame inlinedCallFrame; + inlinedCallFrame.m_pCallerReturnAddress = (TADDR)pFrame->ip; + inlinedCallFrame.m_pCallSiteSP = pFrame; + inlinedCallFrame.m_pCalleeSavedFP = (TADDR)stack; + inlinedCallFrame.m_pThread = GetThread(); + inlinedCallFrame.m_Datum = NULL; + inlinedCallFrame.Push(); + { + GCX_PREEMP(); + InvokeUnmanagedCalli(ftn, cookie, pArgs, pRet); + } + inlinedCallFrame.Pop(); +} + #ifndef TARGET_WASM #include "callstubgenerator.h" @@ -98,9 +123,9 @@ void InvokeManagedMethod(MethodDesc *pMD, int8_t *pArgs, int8_t *pRet, PCODE tar pHeader->Invoke(pHeader->Routines, pArgs, pRet, pHeader->TotalStackSize); } -void InvokeUnmanagedMethod(MethodDesc *targetMethod, int8_t *stack, InterpMethodContextFrame *pFrame, int32_t callArgsOffset, int32_t returnOffset, PCODE callTarget) +void InvokeUnmanagedMethod(MethodDesc *targetMethod, int8_t *pArgs, int8_t *pRet, PCODE callTarget) { - InvokeManagedMethod(targetMethod, stack + callArgsOffset, stack + returnOffset, callTarget); + InvokeManagedMethod(targetMethod, pArgs, pRet, callTarget); } void InvokeDelegateInvokeMethod(MethodDesc *pMDDelegateInvoke, int8_t *pArgs, int8_t *pRet, PCODE target) @@ -131,40 +156,6 @@ void InvokeDelegateInvokeMethod(MethodDesc *pMDDelegateInvoke, int8_t *pArgs, in pHeader->Invoke(pHeader->Routines, pArgs, pRet, pHeader->TotalStackSize); } -void InvokeUnmanagedCalliWithTransition(PCODE ftn, void *cookie, int8_t *stack, InterpMethodContextFrame *pFrame, int8_t *pArgs, int8_t *pRet) -{ - CONTRACTL - { - THROWS; - MODE_ANY; - PRECONDITION(CheckPointer((void*)ftn)); - PRECONDITION(CheckPointer(cookie)); - } - CONTRACTL_END - - // 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. - CallStubHeader* stubHeaderTemplate = (CallStubHeader*)cookie; - size_t templateSize = stubHeaderTemplate->GetSize(); - uint8_t* actualCallStub = (uint8_t*)alloca(templateSize); - memcpy(actualCallStub, stubHeaderTemplate, templateSize); - CallStubHeader *pHeader = (CallStubHeader*)actualCallStub; - pHeader->SetTarget(ftn); // The method to call - - InlinedCallFrame inlinedCallFrame; - inlinedCallFrame.m_pCallerReturnAddress = (TADDR)pFrame->ip; - inlinedCallFrame.m_pCallSiteSP = pFrame; - inlinedCallFrame.m_pCalleeSavedFP = (TADDR)stack; - inlinedCallFrame.m_pThread = GetThread(); - inlinedCallFrame.m_Datum = NULL; - inlinedCallFrame.Push(); - { - GCX_PREEMP(); - pHeader->Invoke(pHeader->Routines, pArgs, pRet, pHeader->TotalStackSize); - } - inlinedCallFrame.Pop(); -} - void InvokeUnmanagedCalli(PCODE ftn, void *cookie, int8_t *pArgs, int8_t *pRet) { CONTRACTL @@ -2227,11 +2218,11 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr if (flags & (int32_t)PInvokeCallFlags::SuppressGCTransition) { - InvokeUnmanagedMethod(targetMethod, stack, pFrame, callArgsOffset, returnOffset, callTarget); + InvokeUnmanagedMethod(targetMethod, stack + callArgsOffset, stack + returnOffset, callTarget); } else { - InvokeUnmanagedMethodWithTransition(targetMethod, stack, pFrame, callArgsOffset, returnOffset, callTarget); + InvokeUnmanagedMethodWithTransition(targetMethod, stack, pFrame, stack + callArgsOffset, stack + returnOffset, callTarget); } break; diff --git a/src/coreclr/vm/wasm/helpers.cpp b/src/coreclr/vm/wasm/helpers.cpp index 1d3964c5e8292b..5db0695c61eebf 100644 --- a/src/coreclr/vm/wasm/helpers.cpp +++ b/src/coreclr/vm/wasm/helpers.cpp @@ -486,7 +486,7 @@ void InvokeManagedMethod(MethodDesc *pMD, int8_t *pArgs, int8_t *pRet, PCODE tar PORTABILITY_ASSERT("Attempted to execute non-interpreter code from interpreter on wasm, this is not yet implemented"); } -void InvokeUnmanagedMethod(MethodDesc *targetMethod, int8_t *stack, InterpMethodContextFrame *pFrame, int32_t callArgsOffset, int32_t returnOffset, PCODE callTarget) +void InvokeUnmanagedMethod(MethodDesc *targetMethod, int8_t *pArgs, int8_t *pRet, PCODE callTarget) { PORTABILITY_ASSERT("Attempted to execute unmanaged code from interpreter on wasm, this is not yet implemented"); } @@ -499,9 +499,9 @@ void InvokeCalliStub(PCODE ftn, void* cookie, int8_t *pArgs, int8_t *pRet) ((void(*)(PCODE, int8_t*, int8_t*))cookie)(ftn, pArgs, pRet); } -void InvokePInvokeCalliStub(PCODE ftn, void *cookie, int8_t *stack, InterpMethodContextFrame *pFrame, int8_t *pArgs, int8_t *pRet) +void InvokeUnmanagedCalli(PCODE ftn, void *cookie, int8_t *pArgs, int8_t *pRet) { - PORTABILITY_ASSERT("InvokePInvokeCalliStub is not implemented on wasm"); + PORTABILITY_ASSERT("Attempted to execute unmanaged calli from interpreter on wasm, this is not yet implemented"); } void InvokeDelegateInvokeMethod(MethodDesc *pMDDelegateInvoke, int8_t *pArgs, int8_t *pRet, PCODE target) From c9a9ef57066f09589422bfa0a7b6ca50d8fe7f35 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Mon, 8 Sep 2025 20:09:41 +0200 Subject: [PATCH 6/6] Fix calling convention check - mask IMAGE_CEE_CS_CALLCONV_MASK --- src/coreclr/interpreter/compiler.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index bf747e3d06318d..69687399a5af8e 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -2860,11 +2860,15 @@ void InterpCompiler::EmitCalli(bool isTailCall, void* calliCookie, int callIFunc m_pLastNewIns->data[0] = GetDataItemIndex(calliCookie); // data[1] is set to 1 if the calli is calling a pinvoke, 0 otherwise bool suppressGCTransition = false; - m_compHnd->getUnmanagedCallConv(nullptr, callSiteSig, &suppressGCTransition); - bool isPInvoke = (callSiteSig->callConv != CORINFO_CALLCONV_DEFAULT && callSiteSig->callConv != CORINFO_CALLCONV_VARARG); - if (isPInvoke && m_compHnd->pInvokeMarshalingRequired(NULL, callSiteSig)) + CorInfoCallConv callConv = (CorInfoCallConv)(callSiteSig->callConv & IMAGE_CEE_CS_CALLCONV_MASK); + bool isPInvoke = (callConv != CORINFO_CALLCONV_DEFAULT && callConv != CORINFO_CALLCONV_VARARG); + if (isPInvoke) { - BADCODE("PInvoke marshalling for calli is not supported in interpreted code"); + if (m_compHnd->pInvokeMarshalingRequired(NULL, callSiteSig)) + { + BADCODE("PInvoke marshalling for calli is not supported in interpreted code"); + } + m_compHnd->getUnmanagedCallConv(nullptr, callSiteSig, &suppressGCTransition); } m_pLastNewIns->data[1] = (suppressGCTransition ? (int32_t)CalliFlags::SuppressGCTransition : 0) | (isPInvoke ? (int32_t)CalliFlags::PInvoke : 0);