From 3088e0ea47c666c9c9db770bb5e9911b2343bdff Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Mon, 14 Jul 2025 08:01:21 -0700 Subject: [PATCH 1/2] Remove GC.Collect intrinsic Checkpoint: Partial removal of tagged helper functions Update ip[] in interpexec side for helper data change Update compiler side for helper indirect tag bit removal Update opcode lengths Cleanup Address copilot review comment Speculative removal of fixme Remove INTOP_FAILFAST Add message to default opcode assert Fix Object.ctor intrinsic eating newobj opcodes Test debugging Checkpoint: Partial revert Repair revert Remove EmitCallIntrinsics Comment out broken check Repair merge damage Test debugging Remove incorrect zeroing of dreg in NEWOBJ_VT Revert diagnostic test changes Prevent newobj dvar from overlapping svars --- src/coreclr/interpreter/compiler.cpp | 160 ++++++++------------ src/coreclr/interpreter/compiler.h | 5 +- src/coreclr/interpreter/interpretershared.h | 8 +- src/coreclr/interpreter/intops.def | 2 - src/coreclr/vm/interpexec.cpp | 71 ++++----- src/tests/JIT/interpreter/Interpreter.cs | 5 + 6 files changed, 107 insertions(+), 144 deletions(-) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index cb585f782687d9..24a8cd49825459 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -2157,32 +2157,32 @@ int32_t InterpCompiler::GetMethodDataItemIndex(CORINFO_METHOD_HANDLE mHandle) return GetDataItemIndex((void*)mHandle); } -int32_t InterpCompiler::GetDataItemIndexForHelperFtn(CorInfoHelpFunc ftn) +int32_t InterpCompiler::GetDataForHelperFtn(CorInfoHelpFunc ftn) { // Interpreter-TODO: Find an existing data item index for this helper if possible and reuse it CORINFO_CONST_LOOKUP ftnLookup; m_compHnd->getHelperFtn(ftn, &ftnLookup); - void* addr = ftnLookup.addr; - if (ftnLookup.accessType == IAT_VALUE) - { - // We can't use the 1 bit to mark indirect addresses because it is used for real code on arm32 (the thumb bit) - // So instead, we mark direct addresses with a 1 and then on the other end we will clear the 1 and re-set it as needed for thumb - addr = (void*)((size_t)addr | INTERP_DIRECT_HELPER_TAG); - } - else if (ftnLookup.accessType == IAT_PPVALUE) - NO_WAY("IAT_PPVALUE helpers not implemented in interpreter"); #ifdef DEBUG - if (!PointerInNameMap(addr)) + if (!PointerInNameMap(ftnLookup.addr)) { const char* name = CorInfoHelperToName(ftn); if (name) - AddPointerToNameMap(addr, name); + AddPointerToNameMap(ftnLookup.addr, name); } #endif - assert(ftnLookup.accessType == IAT_VALUE || ftnLookup.accessType == IAT_PVALUE); - return GetDataItemIndex(addr); + static_assert(sizeof(InterpHelperData) == sizeof(int32_t), "InterpHelperData must be the same size as an int32_t"); + + InterpHelperData result; + result.accessType = ftnLookup.accessType; + int32_t dataItemIndex = GetDataItemIndex(ftnLookup.addr); + result.addressDataItemIndex = dataItemIndex; + + if (((int32_t)result.addressDataItemIndex != dataItemIndex) || ((InfoAccessType)result.accessType != ftnLookup.accessType)) + NO_WAY("Over/underflow in GetDataForHelperFtn"); + + return result.packed; } static int32_t GetLdindForType(InterpType interpType) @@ -2373,49 +2373,6 @@ bool InterpCompiler::EmitNamedIntrinsicCall(NamedIntrinsic ni, CORINFO_CLASS_HAN } } -bool InterpCompiler::EmitCallIntrinsics(CORINFO_METHOD_HANDLE method, CORINFO_SIG_INFO sig) -{ - const char *className = NULL; - const char *namespaceName = NULL; - const char *methodName = m_compHnd->getMethodNameFromMetadata(method, &className, &namespaceName, NULL, 0); - - if (namespaceName && !strcmp(namespaceName, "System")) - { - if (className && !strcmp(className, "Environment")) - { - if (methodName && !strcmp(methodName, "FailFast")) - { - AddIns(INTOP_FAILFAST); // to be removed, not really an intrisic - m_pStackPointer--; - return true; - } - } - else if (className && !strcmp(className, "Object")) - { - // This is needed at this moment because we don't have support for interop - // with compiled code, but it might make sense in the future for this to remain - // in order to avoid redundant interp to jit transition. - if (methodName && !strcmp(methodName, ".ctor")) - { - AddIns(INTOP_NOP); - m_pStackPointer--; - return true; - } - } - else if (className && !strcmp(className, "GC")) - { - if (methodName && !strcmp(methodName, "Collect")) - { - AddIns(INTOP_GC_COLLECT); - // Not reducing the stack pointer because we expect the version with no arguments - return true; - } - } - } - - return false; -} - void InterpCompiler::ResolveToken(uint32_t token, CorInfoTokenKind tokenKind, CORINFO_RESOLVED_TOKEN *pResolvedToken) { pResolvedToken->tokenScope = m_compScopeHnd; @@ -2520,7 +2477,7 @@ void InterpCompiler::EmitPushHelperCall_2(const CorInfoHelpFunc ftn, const CORIN if (handleData.argType == HelperArgType::GenericResolution) { AddIns(INTOP_CALL_HELPER_P_GS); - m_pLastNewIns->data[0] = GetDataItemIndexForHelperFtn(ftn); + m_pLastNewIns->data[0] = GetDataForHelperFtn(ftn); m_pLastNewIns->data[1] = handleData.dataItemIndex; m_pLastNewIns->SetSVars2(handleData.genericVar, arg2); @@ -2529,7 +2486,7 @@ void InterpCompiler::EmitPushHelperCall_2(const CorInfoHelpFunc ftn, const CORIN else { AddIns(INTOP_CALL_HELPER_P_PS); - m_pLastNewIns->data[0] = GetDataItemIndexForHelperFtn(ftn); + m_pLastNewIns->data[0] = GetDataForHelperFtn(ftn); m_pLastNewIns->data[1] = handleData.dataItemIndex; m_pLastNewIns->SetSVar(arg2); @@ -2547,7 +2504,7 @@ void InterpCompiler::EmitPushUnboxAny(const CORINFO_GENERICHANDLE_RESULT& arg1, if (handleData.argType == HelperArgType::GenericResolution) { AddIns(INTOP_UNBOX_ANY_GENERIC); - m_pLastNewIns->data[0] = GetDataItemIndexForHelperFtn(CORINFO_HELP_UNBOX); + m_pLastNewIns->data[0] = GetDataForHelperFtn(CORINFO_HELP_UNBOX); m_pLastNewIns->data[1] = handleData.dataItemIndex; m_pLastNewIns->SetSVars2(handleData.genericVar, arg2); @@ -2556,7 +2513,7 @@ void InterpCompiler::EmitPushUnboxAny(const CORINFO_GENERICHANDLE_RESULT& arg1, else { AddIns(INTOP_UNBOX_ANY); - m_pLastNewIns->data[0] = GetDataItemIndexForHelperFtn(CORINFO_HELP_UNBOX); + m_pLastNewIns->data[0] = GetDataForHelperFtn(CORINFO_HELP_UNBOX); m_pLastNewIns->data[1] = handleData.dataItemIndex; m_pLastNewIns->SetSVar(arg2); @@ -2574,7 +2531,7 @@ void InterpCompiler::EmitPushUnboxAnyNullable(const CORINFO_GENERICHANDLE_RESULT if (handleData.argType == HelperArgType::GenericResolution) { AddIns(INTOP_CALL_HELPER_V_AGS); - m_pLastNewIns->data[0] = GetDataItemIndexForHelperFtn(CORINFO_HELP_UNBOX_NULLABLE); + m_pLastNewIns->data[0] = GetDataForHelperFtn(CORINFO_HELP_UNBOX_NULLABLE); m_pLastNewIns->data[1] = handleData.dataItemIndex; m_pLastNewIns->SetSVars2(handleData.genericVar, arg2); @@ -2583,7 +2540,7 @@ void InterpCompiler::EmitPushUnboxAnyNullable(const CORINFO_GENERICHANDLE_RESULT else { AddIns(INTOP_CALL_HELPER_V_APS); - m_pLastNewIns->data[0] = GetDataItemIndexForHelperFtn(CORINFO_HELP_UNBOX_NULLABLE); + m_pLastNewIns->data[0] = GetDataForHelperFtn(CORINFO_HELP_UNBOX_NULLABLE); m_pLastNewIns->data[1] = handleData.dataItemIndex; m_pLastNewIns->SetSVar(arg2); @@ -2601,7 +2558,7 @@ void InterpCompiler::EmitPushHelperCall_Addr2(const CorInfoHelpFunc ftn, const C if (handleData.argType == HelperArgType::GenericResolution) { AddIns(INTOP_CALL_HELPER_P_GA); - m_pLastNewIns->data[0] = GetDataItemIndexForHelperFtn(ftn); + m_pLastNewIns->data[0] = GetDataForHelperFtn(ftn); m_pLastNewIns->data[1] = handleData.dataItemIndex; m_pLastNewIns->SetSVars2(handleData.genericVar, arg2); @@ -2610,7 +2567,7 @@ void InterpCompiler::EmitPushHelperCall_Addr2(const CorInfoHelpFunc ftn, const C else { AddIns(INTOP_CALL_HELPER_P_PA); - m_pLastNewIns->data[0] = GetDataItemIndexForHelperFtn(ftn); + m_pLastNewIns->data[0] = GetDataForHelperFtn(ftn); m_pLastNewIns->data[1] = handleData.dataItemIndex; m_pLastNewIns->SetSVar(arg2); @@ -2628,7 +2585,7 @@ void InterpCompiler::EmitPushHelperCall(const CorInfoHelpFunc ftn, const CORINFO if (handleData.argType == HelperArgType::GenericResolution) { AddIns(INTOP_CALL_HELPER_P_G); - m_pLastNewIns->data[0] = GetDataItemIndexForHelperFtn(ftn); + m_pLastNewIns->data[0] = GetDataForHelperFtn(ftn); m_pLastNewIns->data[1] = handleData.dataItemIndex; m_pLastNewIns->SetSVar(handleData.genericVar); @@ -2637,7 +2594,7 @@ void InterpCompiler::EmitPushHelperCall(const CorInfoHelpFunc ftn, const CORINFO else { AddIns(INTOP_CALL_HELPER_P_P); - m_pLastNewIns->data[0] = GetDataItemIndexForHelperFtn(ftn); + m_pLastNewIns->data[0] = GetDataForHelperFtn(ftn); m_pLastNewIns->data[1] = handleData.dataItemIndex; m_pLastNewIns->SetDVar(resultVar); @@ -2789,12 +2746,6 @@ void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool re } } - if (EmitCallIntrinsics(callInfo.hMethod, callInfo.sig)) - { - m_ip += 5; - return; - } - if (callInfo.thisTransform != CORINFO_NO_THIS_TRANSFORM) { assert(pConstrainedToken != NULL); @@ -3027,6 +2978,10 @@ void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool re } } m_pLastNewIns->data[0] = GetDataItemIndex(callInfo.hMethod); + + // Ensure that the dvar does not overlap with the svars; it is incorrect for it to overlap because + // the process of initializing the result may trample the args. + m_pVars[dVar].noCallArgs = true; } else if ((callInfo.classFlags & CORINFO_FLG_ARRAY) && newObj) { @@ -3326,7 +3281,7 @@ void InterpCompiler::EmitStaticFieldAddress(CORINFO_FIELD_INFO *pFieldInfo, CORI } // Call helper to obtain thread static base address AddIns(INTOP_CALL_HELPER_P_P); - m_pLastNewIns->data[0] = GetDataItemIndexForHelperFtn(pFieldInfo->helper); + m_pLastNewIns->data[0] = GetDataForHelperFtn(pFieldInfo->helper); m_pLastNewIns->data[1] = GetDataItemIndex(helperArg); PushInterpType(InterpTypeByRef, NULL); m_pLastNewIns->SetDVar(m_pStackPointer[-1].var); @@ -3521,7 +3476,7 @@ void InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo) if (!kind.needsRuntimeLookup) { AddIns(INTOP_CALL_HELPER_P_P); - m_pLastNewIns->data[0] = GetDataItemIndexForHelperFtn(CORINFO_HELP_INITCLASS); + m_pLastNewIns->data[0] = GetDataForHelperFtn(CORINFO_HELP_INITCLASS); m_pLastNewIns->data[1] = GetDataItemIndex(m_classHnd); PushInterpType(InterpTypeI, NULL); m_pLastNewIns->SetDVar(m_pStackPointer[-1].var); @@ -3533,7 +3488,7 @@ void InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo) { case CORINFO_LOOKUP_CLASSPARAM: AddIns(INTOP_CALL_HELPER_P_S); - m_pLastNewIns->data[0] = GetDataItemIndexForHelperFtn(CORINFO_HELP_INITCLASS); + m_pLastNewIns->data[0] = GetDataForHelperFtn(CORINFO_HELP_INITCLASS); m_pLastNewIns->SetSVar(getParamArgIndex()); PushInterpType(InterpTypeI, NULL); m_pLastNewIns->SetDVar(m_pStackPointer[-1].var); @@ -3550,7 +3505,7 @@ void InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo) m_pStackPointer--; AddIns(INTOP_CALL_HELPER_P_SP); - m_pLastNewIns->data[0] = GetDataItemIndexForHelperFtn(CORINFO_HELP_INITINSTCLASS); + m_pLastNewIns->data[0] = GetDataForHelperFtn(CORINFO_HELP_INITINSTCLASS); m_pLastNewIns->data[1] = GetDataItemIndex(m_methodHnd); m_pLastNewIns->SetSVar(thisObjMethodTablePtrVar); PushInterpType(InterpTypeI, NULL); @@ -3561,7 +3516,7 @@ void InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo) case CORINFO_LOOKUP_METHODPARAM: { AddIns(INTOP_CALL_HELPER_P_PS); - m_pLastNewIns->data[0] = GetDataItemIndexForHelperFtn(CORINFO_HELP_INITINSTCLASS); + m_pLastNewIns->data[0] = GetDataForHelperFtn(CORINFO_HELP_INITINSTCLASS); m_pLastNewIns->data[1] = GetDataItemIndex(0); m_pLastNewIns->SetSVar(getParamArgIndex()); PushInterpType(InterpTypeI, NULL); @@ -5628,7 +5583,7 @@ void InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo) AddIns(INTOP_NEWARR_GENERIC); - m_pLastNewIns->data[0] = GetDataItemIndexForHelperFtn(helpFunc); + m_pLastNewIns->data[0] = GetDataForHelperFtn(helpFunc); m_pLastNewIns->data[1] = handleData.dataItemIndex; m_pLastNewIns->SetSVars2(handleData.genericVar, newArrLenVar); @@ -5638,7 +5593,7 @@ void InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo) { AddIns(INTOP_NEWARR); m_pLastNewIns->data[0] = GetDataItemIndex(arrayClsHnd); - m_pLastNewIns->data[1] = GetDataItemIndexForHelperFtn(helpFunc); + m_pLastNewIns->data[1] = GetDataForHelperFtn(helpFunc); m_pLastNewIns->SetSVar(newArrLenVar); m_pLastNewIns->SetDVar(m_pStackPointer[-1].var); @@ -6308,21 +6263,28 @@ void InterpCompiler::PrintPointer(void* pointer) #endif } -void InterpCompiler::PrintHelperFtn(void* helperDirectOrIndirect) +void InterpCompiler::PrintHelperFtn(int32_t _data) { - void* helperAddr = helperDirectOrIndirect; - - if (((size_t)helperDirectOrIndirect) & INTERP_DIRECT_HELPER_TAG) - { - helperAddr = (void*)(((size_t)helperDirectOrIndirect) & ~INTERP_DIRECT_HELPER_TAG); - printf(" (direct)"); - } - else - { - printf(" (indirect)"); - } + InterpHelperData data; + data.packed = _data; + void *helperAddr = GetDataItemAtIndex(data.addressDataItemIndex); PrintPointer(helperAddr); + + switch (data.accessType) { + case IAT_PVALUE: + printf("(indirect) "); + break; + case IAT_PPVALUE: + printf("(double-indirect) "); + break; + case IAT_VALUE: + printf("(direct) "); + break; + default: + printf("(corrupted) "); + break; + } } void InterpCompiler::PrintInsData(InterpInst *ins, int32_t insOffset, const int32_t *pData, int32_t opcode) @@ -6369,7 +6331,7 @@ void InterpCompiler::PrintInsData(InterpInst *ins, int32_t insOffset, const int3 } case InterpOpGenericHelperFtn: { - PrintHelperFtn((void*)GetDataItemAtIndex(pData[0])); + PrintHelperFtn(pData[0]); InterpGenericLookup *pGenericLookup = (InterpGenericLookup*)GetAddrOfDataItemAtIndex(pData[1]); PrintInterpGenericLookup(pGenericLookup); break; @@ -6413,21 +6375,23 @@ void InterpCompiler::PrintInsData(InterpInst *ins, int32_t insOffset, const int3 } case InterpOpHelperFtnNoArgs: { - PrintHelperFtn((void*)GetDataItemAtIndex(pData[0])); + PrintHelperFtn(pData[0]); break; } case InterpOpHelperFtn: { - PrintHelperFtn((void*)GetDataItemAtIndex(pData[0])); - printf(", "); - PrintPointer((void*)GetDataItemAtIndex(pData[1])); + PrintHelperFtn(pData[0]); + if (GetDataLen(opcode) > 1) { + printf(", "); + PrintPointer((void*)GetDataItemAtIndex(pData[1])); + } break; } case InterpOpPointerHelperFtn: { PrintPointer((void*)GetDataItemAtIndex(pData[0])); printf(", "); - PrintHelperFtn((void*)GetDataItemAtIndex(pData[1])); + PrintHelperFtn(pData[1]); break; } case InterpOpPointerInt: diff --git a/src/coreclr/interpreter/compiler.h b/src/coreclr/interpreter/compiler.h index 1ea75c8db439bb..15d0050d074f09 100644 --- a/src/coreclr/interpreter/compiler.h +++ b/src/coreclr/interpreter/compiler.h @@ -527,7 +527,7 @@ class InterpCompiler void* GetDataItemAtIndex(int32_t index); void* GetAddrOfDataItemAtIndex(int32_t index); int32_t GetMethodDataItemIndex(CORINFO_METHOD_HANDLE mHandle); - int32_t GetDataItemIndexForHelperFtn(CorInfoHelpFunc ftn); + int32_t GetDataForHelperFtn(CorInfoHelpFunc ftn); void GenerateCode(CORINFO_METHOD_INFO* methodInfo); InterpBasicBlock* GenerateCodeForFinallyCallIslands(InterpBasicBlock *pNewBB, InterpBasicBlock *pPrevBB); @@ -697,7 +697,6 @@ class InterpCompiler void EmitCompareOp(int32_t opBase); void EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool readonly, bool tailcall, bool newObj, bool isCalli); bool EmitNamedIntrinsicCall(NamedIntrinsic ni, CORINFO_CLASS_HANDLE clsHnd, CORINFO_METHOD_HANDLE method, CORINFO_SIG_INFO sig); - bool EmitCallIntrinsics(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); void EmitLdelem(int32_t opcode, InterpType type); @@ -745,7 +744,7 @@ class InterpCompiler void PrintBBCode(InterpBasicBlock *pBB); void PrintIns(InterpInst *ins); void PrintPointer(void* pointer); - void PrintHelperFtn(void* helperAddr); + void PrintHelperFtn(int32_t _data); void PrintInsData(InterpInst *ins, int32_t offset, const int32_t *pData, int32_t opcode); void PrintCompiledCode(); void PrintCompiledIns(const int32_t *ip, const int32_t *start); diff --git a/src/coreclr/interpreter/interpretershared.h b/src/coreclr/interpreter/interpretershared.h index 210bc1c1ad36db..d92934d496f7b6 100644 --- a/src/coreclr/interpreter/interpretershared.h +++ b/src/coreclr/interpreter/interpretershared.h @@ -17,7 +17,13 @@ #define INTERP_STACK_SLOT_SIZE 8 // Alignment of each var offset on the interpreter stack #define INTERP_STACK_ALIGNMENT 16 // Alignment of interpreter stack at the start of a frame -#define INTERP_DIRECT_HELPER_TAG 1 // When a helper ftn's address is direct we tag it with this tag bit +union InterpHelperData { + struct { + uint32_t addressDataItemIndex : 29; + uint32_t accessType : 3; + }; + int32_t packed; +}; struct CallStubHeader; diff --git a/src/coreclr/interpreter/intops.def b/src/coreclr/interpreter/intops.def index 8de811f7b52c1c..0e1d73769a44fa 100644 --- a/src/coreclr/interpreter/intops.def +++ b/src/coreclr/interpreter/intops.def @@ -400,8 +400,6 @@ OPDEF(INTOP_LEAVE_CATCH, "leavecatch", 2, 0, 0, InterpOpBranch) OPDEF(INTOP_LOAD_EXCEPTION, "load.exception", 2, 1, 0, InterpOpNoArgs) OPDEF(INTOP_THROW_PNSE, "throw.pnse", 1, 0, 0, InterpOpNoArgs) -OPDEF(INTOP_FAILFAST, "failfast", 1, 0, 0, InterpOpNoArgs) -OPDEF(INTOP_GC_COLLECT, "gc.collect", 1, 0, 0, InterpOpNoArgs) OPDEF(INTOP_LOAD_FRAMEVAR, "load.framevar", 2, 1, 0, InterpOpNoArgs) diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index 927bd812a4af0b..086128c6eb91da 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -162,14 +162,23 @@ static OBJECTREF CreateMultiDimArray(MethodTable* arrayClass, int8_t* stack, int #define LOCAL_VAR(offset,type) (*LOCAL_VAR_ADDR(offset, type)) #define NULL_CHECK(o) do { if ((o) == NULL) { COMPlusThrow(kNullReferenceException); } } while (0) -template static THelper GetPossiblyIndirectHelper(void* dataItem) +template static THelper GetPossiblyIndirectHelper(const InterpMethod *pMethod, int32_t _data) { - size_t helperDirectOrIndirect = (size_t)dataItem; - if (helperDirectOrIndirect & INTERP_DIRECT_HELPER_TAG) - // Clear the direct flag and then raise the thumb bit as needed - return (THelper)PINSTRToPCODE((TADDR)(helperDirectOrIndirect & ~INTERP_DIRECT_HELPER_TAG)); - else - return *(THelper *)helperDirectOrIndirect; + InterpHelperData data; + data.packed = _data; + + void *addr = pMethod->pDataItems[data.addressDataItemIndex]; + switch (data.accessType) { + case IAT_VALUE: + return (THelper)addr; + case IAT_PVALUE: + return *(THelper *)addr; + case IAT_PPVALUE: + return **(THelper **)addr; + default: + COMPlusThrowHR(COR_E_EXECUTIONENGINE); + return (THelper)nullptr; + } } // At present our behavior for float to int conversions is to perform a saturating conversion down to either 32 or 64 bits @@ -1695,7 +1704,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr case INTOP_CALL_HELPER_P_P: { - HELPER_FTN_P_P helperFtn = GetPossiblyIndirectHelper(pMethod->pDataItems[ip[2]]); + HELPER_FTN_P_P helperFtn = GetPossiblyIndirectHelper(pMethod, ip[2]); void* helperArg = pMethod->pDataItems[ip[3]]; LOCAL_VAR(ip[1], void*) = helperFtn(helperArg); @@ -1705,7 +1714,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr case INTOP_CALL_HELPER_P_S: { - HELPER_FTN_P_P helperFtn = GetPossiblyIndirectHelper(pMethod->pDataItems[ip[2]]); + HELPER_FTN_P_P helperFtn = GetPossiblyIndirectHelper(pMethod, ip[2]); void* helperArg = LOCAL_VAR(ip[3], void*); LOCAL_VAR(ip[1], void*) = helperFtn(helperArg); @@ -1715,7 +1724,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr case INTOP_CALL_HELPER_P_PS: { - HELPER_FTN_P_PP helperFtn = GetPossiblyIndirectHelper(pMethod->pDataItems[ip[3]]); + HELPER_FTN_P_PP helperFtn = GetPossiblyIndirectHelper(pMethod, ip[3]); void* helperArg = pMethod->pDataItems[ip[4]]; LOCAL_VAR(ip[1], void*) = helperFtn(helperArg, LOCAL_VAR(ip[2], void*)); @@ -1725,7 +1734,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr case INTOP_CALL_HELPER_P_SP: { - HELPER_FTN_P_PP helperFtn = GetPossiblyIndirectHelper(pMethod->pDataItems[ip[3]]); + HELPER_FTN_P_PP helperFtn = GetPossiblyIndirectHelper(pMethod, ip[3]); void* helperArg = pMethod->pDataItems[ip[4]]; LOCAL_VAR(ip[1], void*) = helperFtn(LOCAL_VAR(ip[2], void*), helperArg); @@ -1738,7 +1747,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr InterpGenericLookup *pLookup = (InterpGenericLookup*)&pMethod->pDataItems[ip[4]]; void* helperArg = DoGenericLookup(LOCAL_VAR(ip[2], void*), pLookup); - HELPER_FTN_P_P helperFtn = GetPossiblyIndirectHelper(pMethod->pDataItems[ip[3]]); + HELPER_FTN_P_P helperFtn = GetPossiblyIndirectHelper(pMethod, ip[3]); LOCAL_VAR(ip[1], void*) = helperFtn(helperArg); ip += 5; @@ -1750,7 +1759,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr InterpGenericLookup *pLookup = (InterpGenericLookup*)&pMethod->pDataItems[ip[5]]; void* helperArg = DoGenericLookup(LOCAL_VAR(ip[2], void*), pLookup); - HELPER_FTN_P_PP helperFtn = GetPossiblyIndirectHelper(pMethod->pDataItems[ip[4]]); + HELPER_FTN_P_PP helperFtn = GetPossiblyIndirectHelper(pMethod, ip[4]); LOCAL_VAR(ip[1], void*) = helperFtn(helperArg, LOCAL_VAR(ip[3], void*)); ip += 6; @@ -1762,7 +1771,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr InterpGenericLookup *pLookup = (InterpGenericLookup*)&pMethod->pDataItems[ip[5]]; void* helperArg = DoGenericLookup(LOCAL_VAR(ip[2], void*), pLookup); - HELPER_FTN_P_PP helperFtn = GetPossiblyIndirectHelper(pMethod->pDataItems[ip[4]]); + HELPER_FTN_P_PP helperFtn = GetPossiblyIndirectHelper(pMethod, ip[4]); LOCAL_VAR(ip[1], void*) = helperFtn(helperArg, LOCAL_VAR_ADDR(ip[3], void*)); ip += 6; break; @@ -1770,7 +1779,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr case INTOP_CALL_HELPER_P_PA: { - HELPER_FTN_P_PP helperFtn = GetPossiblyIndirectHelper(pMethod->pDataItems[ip[3]]); + HELPER_FTN_P_PP helperFtn = GetPossiblyIndirectHelper(pMethod, ip[3]); void* helperArg = pMethod->pDataItems[ip[4]]; LOCAL_VAR(ip[1], void*) = helperFtn(helperArg, LOCAL_VAR_ADDR(ip[2], void*)); ip += 5; @@ -1782,7 +1791,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr InterpGenericLookup *pLookup = (InterpGenericLookup*)&pMethod->pDataItems[ip[5]]; void* helperArg = DoGenericLookup(LOCAL_VAR(ip[2], void*), pLookup); - HELPER_FTN_V_PPP helperFtn = GetPossiblyIndirectHelper(pMethod->pDataItems[ip[4]]); + HELPER_FTN_V_PPP helperFtn = GetPossiblyIndirectHelper(pMethod, ip[4]); helperFtn(LOCAL_VAR_ADDR(ip[1], void*), helperArg, LOCAL_VAR(ip[3], void*)); ip += 6; break; @@ -1790,7 +1799,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr case INTOP_CALL_HELPER_V_APS: { - HELPER_FTN_V_PPP helperFtn = GetPossiblyIndirectHelper(pMethod->pDataItems[ip[3]]); + HELPER_FTN_V_PPP helperFtn = GetPossiblyIndirectHelper(pMethod, ip[3]); void* helperArg = pMethod->pDataItems[ip[4]]; helperFtn(LOCAL_VAR_ADDR(ip[1], void*), helperArg, LOCAL_VAR(ip[2], void*)); ip += 5; @@ -1986,9 +1995,6 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr int32_t vtSize = ip[4]; void *vtThis = stack + returnOffset; - // clear the valuetype - memset(vtThis, 0, vtSize); - // pass the address of the valuetype LOCAL_VAR(callArgsOffset, void*) = vtThis; @@ -2021,18 +2027,6 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr ip += 3; break; } - case INTOP_GC_COLLECT: - { - // HACK: blocking gc of all generations to enable early stackwalk testing - // Interpreter-TODO: Remove this - { - pInterpreterFrame->SetTopInterpMethodContextFrame(pFrame); - GCX_COOP(); - GCHeapUtilities::GetGCHeap()->GarbageCollect(-1, false, collection_blocking | collection_aggressive); - } - ip++; - break; - } case INTOP_THROW: { OBJECTREF throwable; @@ -2068,7 +2062,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr int opcode = *ip; int dreg = ip[1]; int sreg = ip[2]; - HELPER_FTN_BOX_UNBOX helper = GetPossiblyIndirectHelper(pMethod->pDataItems[ip[3]]); + HELPER_FTN_BOX_UNBOX helper = GetPossiblyIndirectHelper(pMethod, ip[3]); MethodTable *pMT = (MethodTable*)pMethod->pDataItems[ip[4]]; // private static ref byte Unbox(MethodTable* toTypeHnd, object obj) @@ -2088,7 +2082,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr InterpGenericLookup *pLookup = (InterpGenericLookup*)&pMethod->pDataItems[ip[5]]; MethodTable *pMTBoxedObj = (MethodTable*)DoGenericLookup(LOCAL_VAR(ip[2], void*), pLookup); - HELPER_FTN_BOX_UNBOX helper = GetPossiblyIndirectHelper(pMethod->pDataItems[ip[4]]); + HELPER_FTN_BOX_UNBOX helper = GetPossiblyIndirectHelper(pMethod, ip[4]); // private static ref byte Unbox(MethodTable* toTypeHnd, object obj) Object *src = LOCAL_VAR(sreg, Object*); @@ -2105,7 +2099,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr COMPlusThrow(kArgumentOutOfRangeException); MethodTable* arrayClsHnd = (MethodTable*)pMethod->pDataItems[ip[3]]; - HELPER_FTN_NEWARR helper = GetPossiblyIndirectHelper(pMethod->pDataItems[ip[4]]); + HELPER_FTN_NEWARR helper = GetPossiblyIndirectHelper(pMethod, ip[4]); Object* arr = helper(arrayClsHnd, (intptr_t)length); LOCAL_VAR(ip[1], OBJECTREF) = ObjectToOBJECTREF(arr); @@ -2122,7 +2116,7 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr InterpGenericLookup *pLookup = (InterpGenericLookup*)&pMethod->pDataItems[ip[5]]; MethodTable *arrayClsHnd = (MethodTable*)DoGenericLookup(LOCAL_VAR(ip[2], void*), pLookup); - HELPER_FTN_NEWARR helper = GetPossiblyIndirectHelper(pMethod->pDataItems[ip[4]]); + HELPER_FTN_NEWARR helper = GetPossiblyIndirectHelper(pMethod, ip[4]); Object* arr = helper(arrayClsHnd, (intptr_t)length); LOCAL_VAR(ip[1], OBJECTREF) = ObjectToOBJECTREF(arr); @@ -2460,11 +2454,8 @@ do \ case INTOP_THROW_PNSE: COMPlusThrow(kPlatformNotSupportedException); break; - case INTOP_FAILFAST: - assert(0); - break; default: - assert(0); + assert(!"Unimplemented or invalid interpreter opcode"); break; } } diff --git a/src/tests/JIT/interpreter/Interpreter.cs b/src/tests/JIT/interpreter/Interpreter.cs index 61c02d4bb61e6d..90516f0466a737 100644 --- a/src/tests/JIT/interpreter/Interpreter.cs +++ b/src/tests/JIT/interpreter/Interpreter.cs @@ -2075,6 +2075,11 @@ public static T TestUnboxInst(object o) struct GenericStruct { public T Value; + + public override string ToString() + { + return "GenericStruct: " + (Value?.ToString() ?? ""); + } } public static int preciseInitCctorsRun = 0; From 6c9abb14692c0d5a5b0ff2309abe6adc1c2b7c8e Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Tue, 15 Jul 2025 14:16:55 -0700 Subject: [PATCH 2/2] Use memcpy instead of union for type punning --- src/coreclr/interpreter/compiler.cpp | 6 ++++-- src/coreclr/interpreter/interpretershared.h | 9 +++------ src/coreclr/vm/interpexec.cpp | 2 +- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index 24a8cd49825459..51dce8717e448b 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -2182,7 +2182,9 @@ int32_t InterpCompiler::GetDataForHelperFtn(CorInfoHelpFunc ftn) if (((int32_t)result.addressDataItemIndex != dataItemIndex) || ((InfoAccessType)result.accessType != ftnLookup.accessType)) NO_WAY("Over/underflow in GetDataForHelperFtn"); - return result.packed; + int32_t packed; + memcpy(&packed, &result, sizeof(result)); + return packed; } static int32_t GetLdindForType(InterpType interpType) @@ -6266,7 +6268,7 @@ void InterpCompiler::PrintPointer(void* pointer) void InterpCompiler::PrintHelperFtn(int32_t _data) { InterpHelperData data; - data.packed = _data; + memcpy(&data, &_data, sizeof(int32_t)); void *helperAddr = GetDataItemAtIndex(data.addressDataItemIndex); PrintPointer(helperAddr); diff --git a/src/coreclr/interpreter/interpretershared.h b/src/coreclr/interpreter/interpretershared.h index d92934d496f7b6..b4bb490237ed6d 100644 --- a/src/coreclr/interpreter/interpretershared.h +++ b/src/coreclr/interpreter/interpretershared.h @@ -17,12 +17,9 @@ #define INTERP_STACK_SLOT_SIZE 8 // Alignment of each var offset on the interpreter stack #define INTERP_STACK_ALIGNMENT 16 // Alignment of interpreter stack at the start of a frame -union InterpHelperData { - struct { - uint32_t addressDataItemIndex : 29; - uint32_t accessType : 3; - }; - int32_t packed; +struct InterpHelperData { + uint32_t addressDataItemIndex : 29; + uint32_t accessType : 3; }; struct CallStubHeader; diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index 086128c6eb91da..ea0a3509320bc3 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -165,7 +165,7 @@ static OBJECTREF CreateMultiDimArray(MethodTable* arrayClass, int8_t* stack, int template static THelper GetPossiblyIndirectHelper(const InterpMethod *pMethod, int32_t _data) { InterpHelperData data; - data.packed = _data; + memcpy(&data, &_data, sizeof(int32_t)); void *addr = pMethod->pDataItems[data.addressDataItemIndex]; switch (data.accessType) {