From bfec23b8f4929c27ffc330c4f0108ec28c766697 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Thu, 15 May 2025 18:06:51 -0700 Subject: [PATCH 01/11] Instrinic support for transient codegen. The previous transient codegen paths only handled lifetime management when the method had no metadata back IL (that is, RVA == 0). The InstanceCalli logic is marked as an intrinsic and has metadata, but that metadata is copied and then modified falling back to a middle ground that the existing logic didn't handle properly. The tricky scenario is when these new instrinsic types were inlined (lifetimes were being ignored). This change fixes that up but did require a bit of clean-up in the UnsafeJitFunction as it relates to the JIT and interpreter interaction. --- src/coreclr/vm/jitinterface.cpp | 411 ++++++++++++++++++-------------- src/coreclr/vm/jitinterface.h | 25 +- 2 files changed, 241 insertions(+), 195 deletions(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index c9bcb1e75f3b63..d48630bc3be6bc 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -6612,14 +6612,14 @@ void CEEInfo::setMethodAttribs ( } /*********************************************************************/ -class MethodInfoHelperContext final +class MethodInfoWorkerContext final { public: MethodDesc* Method; COR_ILMETHOD_DECODER* Header; DynamicResolver* TransientResolver; - MethodInfoHelperContext(MethodDesc* pMD, _In_opt_ COR_ILMETHOD_DECODER* header = NULL) + MethodInfoWorkerContext(MethodDesc* pMD, _In_opt_ COR_ILMETHOD_DECODER* header = NULL) : Method{ pMD } , Header{ header } , TransientResolver{} @@ -6628,17 +6628,17 @@ class MethodInfoHelperContext final _ASSERTE(pMD != NULL); } - MethodInfoHelperContext(const MethodInfoHelperContext&) = delete; - MethodInfoHelperContext(MethodInfoHelperContext&& other) = delete; + MethodInfoWorkerContext(const MethodInfoWorkerContext&) = delete; + MethodInfoWorkerContext(MethodInfoWorkerContext&& other) = delete; - ~MethodInfoHelperContext() + ~MethodInfoWorkerContext() { STANDARD_VM_CONTRACT; delete TransientResolver; } - MethodInfoHelperContext& operator=(const MethodInfoHelperContext&) = delete; - MethodInfoHelperContext& operator=(MethodInfoHelperContext&& other) = delete; + MethodInfoWorkerContext& operator=(const MethodInfoWorkerContext&) = delete; + MethodInfoWorkerContext& operator=(MethodInfoWorkerContext&& other) = delete; bool HasTransientMethodDetails() const { @@ -6699,7 +6699,7 @@ void getMethodInfoILMethodHeaderHelper( (CorInfoOptions)((header->GetFlags() & CorILMethod_InitLocals) ? CORINFO_OPT_INIT_LOCALS : 0) ; } -mdToken FindGenericMethodArgTypeSpec(IMDInternalImport* pInternalImport) +static mdToken FindGenericMethodArgTypeSpec(IMDInternalImport* pInternalImport) { STANDARD_VM_CONTRACT; @@ -7289,7 +7289,7 @@ bool IsBitwiseEquatable(TypeHandle typeHandle, MethodTable * methodTable) } static bool getILIntrinsicImplementationForRuntimeHelpers( - MethodInfoHelperContext& cxt, + MethodInfoWorkerContext& cxt, CORINFO_METHOD_INFO* methInfo, SigPointer* localSig) { @@ -7494,19 +7494,24 @@ static bool getILIntrinsicImplementationForActivator(MethodDesc* ftn, //--------------------------------------------------------------------------------------- // - -static void getMethodInfoHelper( - MethodInfoHelperContext& cxt, +void CEEInfo::getMethodInfoWorker( + MethodInfoWorkerContext& cxt, CORINFO_METHOD_INFO* methInfo, - CORINFO_CONTEXT_HANDLE exactContext = NULL) + CORINFO_CONTEXT_HANDLE exactContext) { STANDARD_VM_CONTRACT; + _ASSERTE(cxt.Method != NULL); _ASSERTE(methInfo != NULL); MethodDesc* ftn = cxt.Method; methInfo->ftn = (CORINFO_METHOD_HANDLE)ftn; methInfo->regionKind = CORINFO_REGION_JIT; + _ASSERTE(!cxt.HasTransientMethodDetails()); + TransientMethodDetails* detailsMaybe = NULL; + if (FindTransientMethodDetails(ftn, &detailsMaybe)) + cxt.UpdateWith(*detailsMaybe); + CORINFO_MODULE_HANDLE scopeHnd = NULL; /* Grab information from the IL header */ @@ -7657,23 +7662,6 @@ static void getMethodInfoHelper( &context, CONV_TO_JITSIG_FLAGS_LOCALSIG, &methInfo->locals); -} // getMethodInfoHelper - - -void CEEInfo::getTransientMethodInfo(MethodDesc* pMD, CORINFO_METHOD_INFO* methInfo) -{ - MethodInfoHelperContext cxt{ pMD }; - - // We will either find or create transient method details. - _ASSERTE(!cxt.HasTransientMethodDetails()); - - // IL methods with no RVA indicate there is no implementation defined in metadata. - // Check if we previously generated details/implementation for this method. - TransientMethodDetails* detailsMaybe = NULL; - if (FindTransientMethodDetails(pMD, &detailsMaybe)) - cxt.UpdateWith(*detailsMaybe); - - getMethodInfoHelper(cxt, methInfo); // If we have transient method details we need to handle // the lifetime of the details. @@ -7688,7 +7676,7 @@ void CEEInfo::getTransientMethodInfo(MethodDesc* pMD, CORINFO_METHOD_INFO* methI // transferred or was found in this instance. cxt.UpdateWith({}); } -} +} // getMethodInfoWorker //--------------------------------------------------------------------------------------- // @@ -7708,31 +7696,27 @@ CEEInfo::getMethodInfo( JIT_TO_EE_TRANSITION(); - MethodDesc * ftn = GetMethod(ftnHnd); + MethodDesc* ftn = GetMethod(ftnHnd); - // Get the IL header - MethodInfoHelperContext cxt{ ftn }; - if (ftn->IsDynamicMethod()) + MethodInfoWorkerContext cxt{ ftn }; + if (ftn->IsDynamicMethod() + || (ftn->IsIL() && ftn->GetRVA() == 0)) // IL methods with no RVA indicate there is no implementation defined in metadata. { - getMethodInfoHelper(cxt, methInfo, context); + getMethodInfoWorker(cxt, methInfo, context); result = true; } else if (!ftn->IsWrapperStub() && ftn->HasILHeader()) { + // Get the IL header and set it. COR_ILMETHOD_DECODER header(ftn->GetILHeader(), ftn->GetMDImport(), NULL); cxt.Header = &header; - getMethodInfoHelper(cxt, methInfo, context); - result = true; - } - else if (ftn->IsIL() && ftn->GetRVA() == 0) - { - getTransientMethodInfo(ftn, methInfo); + getMethodInfoWorker(cxt, methInfo, context); result = true; } if (result) { - LOG((LF_JIT, LL_INFO100000, "Getting method info (possible inline) %s::%s%s\n", + LOG((LF_JIT, LL_INFO100000, "Getting method info (possible inline) %s::%s %s\n", ftn->m_pszDebugClassName, ftn->m_pszDebugMethodName, ftn->m_pszDebugMethodSignature)); } @@ -7909,9 +7893,9 @@ CorInfoInline CEEInfo::canInline (CORINFO_METHOD_HANDLE hCaller, } else if (pCallee->IsIL() && pCallee->GetRVA() == 0) { + MethodInfoWorkerContext cxt{ pCallee }; CORINFO_METHOD_INFO methodInfo; - getTransientMethodInfo(pCallee, &methodInfo); - + getMethodInfoWorker(cxt, &methodInfo); if (methodInfo.EHcount > 0) { result = INLINE_FAIL; @@ -12309,6 +12293,26 @@ void* CEECodeGenInfo::getMethodSync(CORINFO_METHOD_HANDLE ftnHnd, return result; } +CORINFO_METHOD_INFO CEECodeGenInfo::getMethodInfo( + MethodInfoWorkerContext& cxt) +{ + STANDARD_VM_CONTRACT; + _ASSERTE(cxt.Method != NULL); + + CORINFO_METHOD_INFO methInfo; + getMethodInfoWorker(cxt, &methInfo); + + // Verify method attributes and signature are consistant + _ASSERTE(!!cxt.Method->IsStatic() == ((methInfo.args.callConv & CORINFO_CALLCONV_HASTHIS) == 0)); + + // Either the member header was NULL or remains the same as the input. + _ASSERTE(m_ILHeader == NULL || cxt.Header == m_ILHeader); + + // Update the member with the result of the MethodInfo call. + m_ILHeader = cxt.Header; + return methInfo; +} + /*********************************************************************/ HRESULT CEEJitInfo::allocPgoInstrumentationBySchema( CORINFO_METHOD_HANDLE ftnHnd, /* IN */ @@ -13067,13 +13071,9 @@ void ThrowExceptionForJit(HRESULT res) // ******************************************************************** //#define PERF_TRACK_METHOD_JITTIMES -#ifdef TARGET_AMD64 -BOOL g_fAllowRel32 = TRUE; -#endif - -PCODE UnsafeJitFunctionWorker(EECodeGenManager *pJitMgr, CEECodeGenInfo *pJitInfo, CORJIT_FLAGS* pJitFlags, CORINFO_METHOD_INFO methodInfo, - MethodInfoHelperContext *pCxt, NativeCodeVersion nativeCodeVersion, ULONG* pSizeOfCode) +static TADDR UnsafeJitFunctionWorker(EECodeGenManager *pJitMgr, CEECodeGenInfo *pJitInfo, CORJIT_FLAGS* pJitFlags, CORINFO_METHOD_INFO methodInfo, + MethodInfoWorkerContext *pCxt, NativeCodeVersion nativeCodeVersion, ULONG* pSizeOfCode) { STANDARD_VM_CONTRACT; if (pCxt->HasTransientMethodDetails()) @@ -13082,11 +13082,6 @@ PCODE UnsafeJitFunctionWorker(EECodeGenManager *pJitMgr, CEECodeGenInfo *pJitInf MethodDesc * pMethodForSecurity = pJitInfo->GetMethodForSecurity(methodInfo.ftn); MethodDesc * ftn = nativeCodeVersion.GetMethodDesc(); -#ifdef _DEBUG - LPCUTF8 cls = ftn->GetMethodTable()->GetDebugClassName(); - LPCUTF8 name = ftn->GetName(); -#endif - //Since the check could trigger a demand, we have to do this every time. //This is actually an overly complicated way to make sure that a method can access all its arguments //and its return type. @@ -13182,8 +13177,6 @@ PCODE UnsafeJitFunctionWorker(EECodeGenManager *pJitMgr, CEECodeGenInfo *pJitInf } - LOG((LF_JIT, LL_INFO10000, "Done Jitting method %s::%s %s }\n",cls,name, ftn->m_pszDebugMethodSignature)); - if (res == CORJIT_SKIPPED) { // We are done @@ -13231,9 +13224,133 @@ PCODE UnsafeJitFunctionWorker(EECodeGenManager *pJitMgr, CEECodeGenInfo *pJitInf ClrFlushInstructionCache(nativeEntry, sizeOfCode, /* hasCodeExecutedBefore */ true); // We are done - return (PCODE)nativeEntry; + return (TADDR)nativeEntry; } +class JumpStubOverflowCheck final +{ +#if defined(TARGET_AMD64) || defined(TARGET_ARM64) +#if defined(TARGET_AMD64) + static BOOL g_fAllowRel32; +#endif // TARGET_AMD64 + + BOOL _fForceJumpStubOverflow; + BOOL _fAllowRel32; + size_t _reserveForJumpStubs; +#endif // defined(TARGET_AMD64) || defined(TARGET_ARM64) + +public: + JumpStubOverflowCheck() + { + STANDARD_VM_CONTRACT; + +#if defined(TARGET_AMD64) || defined(TARGET_ARM64) + _fForceJumpStubOverflow = FALSE; +#ifdef _DEBUG + // Always exercise the overflow codepath with force relocs + if (PEDecoder::GetForceRelocs()) + _fForceJumpStubOverflow = TRUE; +#endif + + _fAllowRel32 = FALSE; +#if defined(TARGET_AMD64) + _fAllowRel32 = (g_fAllowRel32 | _fForceJumpStubOverflow) && g_pConfig->JitEnableOptionalRelocs(); +#endif // TARGET_AMD64 + + _reserveForJumpStubs = 0; +#endif // defined(TARGET_AMD64) || defined(TARGET_ARM64) + } + + ~JumpStubOverflowCheck() = default; + + void Enable(CEEJitInfo& jitInfo) + { + STANDARD_VM_CONTRACT; + +#if defined(TARGET_AMD64) || defined(TARGET_ARM64) +#if defined(TARGET_AMD64) + if (_fForceJumpStubOverflow) + jitInfo.SetJumpStubOverflow(_fAllowRel32); + jitInfo.SetAllowRel32(_fAllowRel32); +#else + if (_fForceJumpStubOverflow) + jitInfo.SetJumpStubOverflow(_fForceJumpStubOverflow); +#endif + jitInfo.SetReserveForJumpStubs(_reserveForJumpStubs); +#endif // defined(TARGET_AMD64) || defined(TARGET_ARM64) + } + + bool Detected(CEEJitInfo& jitInfo, EEJitManager* jitMgr) + { + STANDARD_VM_CONTRACT; + +#if defined(TARGET_AMD64) || defined(TARGET_ARM64) + if (jitInfo.IsJumpStubOverflow()) + { + // Backout and try again with fAllowRel32 == FALSE. + jitInfo.BackoutJitData(jitMgr); + +#if defined(TARGET_AMD64) + // Disallow rel32 relocs in future. + g_fAllowRel32 = FALSE; + + _fAllowRel32 = FALSE; +#endif // TARGET_AMD64 +#if defined(TARGET_ARM64) + _fForceJumpStubOverflow = FALSE; +#endif // TARGET_ARM64 + + _reserveForJumpStubs = jitInfo.GetReserveForJumpStubs(); + return true; + } +#endif // defined(TARGET_AMD64) || defined(TARGET_ARM64) + + // No overflow detected + return false; + } +}; + +#if defined(TARGET_AMD64) +BOOL JumpStubOverflowCheck::g_fAllowRel32 = TRUE; +#endif // TARGET_AMD64 + +#ifdef LOGGING +class LogJitMethod final +{ + MethodDesc* _ftn; +public: + LogJitMethod(MethodDesc* ftn) + : _ftn{ ftn } + { + STANDARD_VM_CONTRACT; + _ASSERTE(_ftn != NULL); + if (_ftn->IsNoMetadata()) + { + if (_ftn->IsILStub()) + { + LOG((LF_JIT, LL_INFO10000, "{ Jitting IL Stub (%p)\n", _ftn)); + } + else + { + LOG((LF_JIT, LL_INFO10000, "{ Jitting dynamic method (%p)\n", _ftn)); + } + } + else + { + LPCUTF8 cls = _ftn->GetMethodTable()->GetDebugClassName(); + LPCUTF8 name = _ftn->GetName(); + LOG((LF_JIT, LL_INFO10000, "{ Jitting method (%p) %s::%s %s\n", _ftn, cls, name, _ftn->m_pszDebugMethodSignature)); + } + } + + ~LogJitMethod() + { + STANDARD_VM_CONTRACT; + LOG((LF_JIT, LL_INFO10000, "Done Jitting method (%p) }\n", _ftn)); + } +}; +#endif // LOGGING + // ******************************************************************** // README!! // ******************************************************************** @@ -13250,7 +13367,7 @@ PCODE UnsafeJitFunctionWorker(EECodeGenManager *pJitMgr, CEECodeGenInfo *pJitInf PCODE UnsafeJitFunction(PrepareCodeConfig* config, _In_opt_ COR_ILMETHOD_DECODER* ILHeader, _In_ CORJIT_FLAGS* pJitFlags, - _In_opt_ ULONG* pSizeOfCode) + _In_ ULONG* pSizeOfCode) { STANDARD_VM_CONTRACT; _ASSERTE(config != NULL); @@ -13259,6 +13376,13 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, NativeCodeVersion nativeCodeVersion = config->GetCodeVersion(); MethodDesc* ftn = nativeCodeVersion.GetMethodDesc(); + // If it's generic then we can only enter through an instantiated MethodDesc + _ASSERTE(!ftn->IsGenericMethodDefinition()); + +#ifdef LOGGING + LogJitMethod{ ftn }; +#endif // LOGGING + PCODE ret = (PCODE)NULL; NormalizedTimer timer; int64_t c100nsTicksInJit = 0; @@ -13267,124 +13391,65 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, timer.Start(); -#ifdef FEATURE_INTERPRETER - InterpreterJitManager *interpreterMgr = ExecutionManager::GetInterpreterJitManager(); - - LPWSTR interpreterConfig; - IfFailThrow(CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_Interpreter, &interpreterConfig)); + // Negate the check for no inlining so the logic is easier to follow + bool enableInlining = !config->GetJitCompilationFlags().IsSet(CORJIT_FLAGS::CORJIT_FLAG_NO_INLINING); - if ((interpreterConfig != NULL) && !interpreterMgr->LoadInterpreter()) - { - EEPOLICY_HANDLE_FATAL_ERROR_WITH_MESSAGE(COR_E_EXECUTIONENGINE, W("Failed to load interpreter")); - } -#endif // FEATURE_INTERPRETER + CORINFO_METHOD_INFO methodInfo{}; + MethodInfoWorkerContext cxt{ ftn, ILHeader }; - EEJitManager *jitMgr = ExecutionManager::GetEEJitManager(); - if (!jitMgr->LoadJIT()) +#ifdef FEATURE_INTERPRETER + InterpreterJitManager* interpreterMgr = ExecutionManager::GetInterpreterJitManager(); + if (!interpreterMgr->IsInterpreterLoaded()) { -#ifdef ALLOW_SXS_JIT - if (!jitMgr->IsMainJitLoaded()) + LPWSTR interpreterConfig; + IfFailThrow(CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_Interpreter, &interpreterConfig)); + if ((interpreterConfig != NULL) && !interpreterMgr->LoadInterpreter()) { - // Don't want to throw InvalidProgram from here. - EEPOLICY_HANDLE_FATAL_ERROR_WITH_MESSAGE(COR_E_EXECUTIONENGINE, W("Failed to load JIT compiler")); + EEPOLICY_HANDLE_FATAL_ERROR_WITH_MESSAGE(COR_E_EXECUTIONENGINE, W("Failed to load interpreter")); } - if (!jitMgr->IsAltJitLoaded()) - { - // Don't want to throw InvalidProgram from here. - EEPOLICY_HANDLE_FATAL_ERROR_WITH_MESSAGE(COR_E_EXECUTIONENGINE, W("Failed to load alternative JIT compiler")); - } -#else // ALLOW_SXS_JIT - // Don't want to throw InvalidProgram from here. - EEPOLICY_HANDLE_FATAL_ERROR_WITH_MESSAGE(COR_E_EXECUTIONENGINE, W("Failed to load JIT compiler")); -#endif // ALLOW_SXS_JIT } -#ifdef _DEBUG - // This is here so we can see the name and class easily in the debugger - - LPCUTF8 cls = ftn->GetMethodTable()->GetDebugClassName(); - LPCUTF8 name = ftn->GetName(); - - if (ftn->IsNoMetadata()) - { - if (ftn->IsILStub()) - { - LOG((LF_JIT, LL_INFO10000, "{ Jitting IL Stub }\n")); - } - else - { - LOG((LF_JIT, LL_INFO10000, "{ Jitting dynamic method }\n")); - } - } - else + // If the interpreter was loaded, use it. + if (interpreterMgr->IsInterpreterLoaded()) { - SString methodString; - if (LoggingOn(LF_JIT, LL_INFO10000)) - TypeString::AppendMethodDebug(methodString, ftn); - - LOG((LF_JIT, LL_INFO10000, "{ Jitting method (%p) %s %s\n", ftn, methodString.GetUTF8(), ftn->m_pszDebugMethodSignature)); - } -#endif // _DEBUG + CInterpreterJitInfo interpreterJitInfo{ ftn, ILHeader, interpreterMgr, enableInlining }; + methodInfo = interpreterJitInfo.getMethodInfo(cxt); + *pJitFlags = GetCompileFlags(config, ftn, &methodInfo); - MethodInfoHelperContext cxt{ ftn, ILHeader }; - CORINFO_METHOD_INFO methodInfo; - getMethodInfoHelper(cxt, &methodInfo); - - if (ILHeader == nullptr) - ILHeader = cxt.Header; - - // If it's generic then we can only enter through an instantiated MethodDesc - _ASSERTE(!ftn->IsGenericMethodDefinition()); - - // method attributes and signature are consistant - _ASSERTE(!!ftn->IsStatic() == ((methodInfo.args.callConv & CORINFO_CALLCONV_HASTHIS) == 0)); - - *pJitFlags = GetCompileFlags(config, ftn, &methodInfo); - -#ifdef FEATURE_INTERPRETER - bool useInterpreter = interpreterMgr->IsInterpreterLoaded(); - - if (useInterpreter) - { - CInterpreterJitInfo interpreterJitInfo(ftn, ILHeader, interpreterMgr, !pJitFlags->IsSet(CORJIT_FLAGS::CORJIT_FLAG_NO_INLINING)); ret = UnsafeJitFunctionWorker(interpreterMgr, &interpreterJitInfo, pJitFlags, methodInfo, &cxt, nativeCodeVersion, pSizeOfCode); } #endif // FEATURE_INTERPRETER if (!ret) { -#if defined(TARGET_AMD64) || defined(TARGET_ARM64) - BOOL fForceJumpStubOverflow = FALSE; - -#ifdef _DEBUG - // Always exercise the overflow codepath with force relocs - if (PEDecoder::GetForceRelocs()) - fForceJumpStubOverflow = TRUE; -#endif - -#if defined(TARGET_AMD64) - BOOL fAllowRel32 = (g_fAllowRel32 | fForceJumpStubOverflow) && g_pConfig->JitEnableOptionalRelocs(); -#endif - - size_t reserveForJumpStubs = 0; - -#endif // defined(TARGET_AMD64) || defined(TARGET_ARM64) + EEJitManager *jitMgr = ExecutionManager::GetEEJitManager(); + if (!jitMgr->LoadJIT()) + { +#ifdef ALLOW_SXS_JIT + if (!jitMgr->IsMainJitLoaded()) + { + // Don't want to throw InvalidProgram from here. + EEPOLICY_HANDLE_FATAL_ERROR_WITH_MESSAGE(COR_E_EXECUTIONENGINE, W("Failed to load JIT compiler")); + } + if (!jitMgr->IsAltJitLoaded()) + { + // Don't want to throw InvalidProgram from here. + EEPOLICY_HANDLE_FATAL_ERROR_WITH_MESSAGE(COR_E_EXECUTIONENGINE, W("Failed to load alternative JIT compiler")); + } +#else // ALLOW_SXS_JIT + // Don't want to throw InvalidProgram from here. + EEPOLICY_HANDLE_FATAL_ERROR_WITH_MESSAGE(COR_E_EXECUTIONENGINE, W("Failed to load JIT compiler")); +#endif // ALLOW_SXS_JIT + } + JumpStubOverflowCheck jsoCheck{}; while (true) { - CEEJitInfo jitInfo(ftn, ILHeader, jitMgr, !pJitFlags->IsSet(CORJIT_FLAGS::CORJIT_FLAG_NO_INLINING)); + CEEJitInfo jitInfo{ ftn, ILHeader, jitMgr, enableInlining }; + methodInfo = jitInfo.getMethodInfo(cxt); + *pJitFlags = GetCompileFlags(config, ftn, &methodInfo); -#if defined(TARGET_AMD64) || defined(TARGET_ARM64) -#if defined(TARGET_AMD64) - if (fForceJumpStubOverflow) - jitInfo.SetJumpStubOverflow(fAllowRel32); - jitInfo.SetAllowRel32(fAllowRel32); -#else - if (fForceJumpStubOverflow) - jitInfo.SetJumpStubOverflow(fForceJumpStubOverflow); -#endif - jitInfo.SetReserveForJumpStubs(reserveForJumpStubs); -#endif // defined(TARGET_AMD64) || defined(TARGET_ARM64) + jsoCheck.Enable(jitInfo); #ifdef FEATURE_ON_STACK_REPLACEMENT // If this is an OSR jit request, grab the OSR info so we can pass it to the jit @@ -13396,28 +13461,12 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, } #endif // FEATURE_ON_STACK_REPLACEMENT - ret = UnsafeJitFunctionWorker(jitMgr, &jitInfo, pJitFlags, methodInfo, &cxt, nativeCodeVersion, pSizeOfCode); - if (!ret) + TADDR retMaybe = UnsafeJitFunctionWorker(jitMgr, &jitInfo, pJitFlags, methodInfo, &cxt, nativeCodeVersion, pSizeOfCode); + if (!retMaybe) COMPlusThrow(kInvalidProgramException); -#if (defined(TARGET_AMD64) || defined(TARGET_ARM64)) - if (jitInfo.IsJumpStubOverflow()) + if (jsoCheck.Detected(jitInfo, jitMgr)) { - // Backout and try again with fAllowRel32 == FALSE. - jitInfo.BackoutJitData(jitMgr); - -#ifdef TARGET_AMD64 - // Disallow rel32 relocs in future. - g_fAllowRel32 = FALSE; - - fAllowRel32 = FALSE; -#endif // TARGET_AMD64 -#ifdef TARGET_ARM64 - fForceJumpStubOverflow = FALSE; -#endif // TARGET_ARM64 - - reserveForJumpStubs = jitInfo.GetReserveForJumpStubs(); - // Get any transient method details and take ownership // from the JITInfo instance. We are going to be recreating // a new JITInfo and will reuse these details there. @@ -13425,12 +13474,8 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, cxt.TakeOwnership(std::move(details)); continue; } -#endif // (TARGET_AMD64 || TARGET_ARM64) - -#ifdef TARGET_ARM - ret |= THUMB_CODE; -#endif + ret = PINSTRToPCODE(retMaybe); jitInfo.PublishFinalCodeAddress(ret); // We are done diff --git a/src/coreclr/vm/jitinterface.h b/src/coreclr/vm/jitinterface.h index 1b421ab3a20281..728e9f554ff13c 100644 --- a/src/coreclr/vm/jitinterface.h +++ b/src/coreclr/vm/jitinterface.h @@ -334,6 +334,9 @@ struct TransientMethodDetails final TransientMethodDetails& operator=(TransientMethodDetails&&); }; +// Forward declaration +class MethodInfoWorkerContext; + class CEEInfo : public ICorJitInfo { friend class CEEDynamicCodeInfo; @@ -445,12 +448,6 @@ class CEEInfo : public ICorJitInfo void setJitFlags(const CORJIT_FLAGS& jitFlags); -private: -#ifdef _DEBUG - InlineSString ssClsNameBuff; - InlineSString ssClsNameBuffUTF8; -#endif - public: MethodDesc * GetMethodForSecurity(CORINFO_METHOD_HANDLE callerHandle); @@ -472,8 +469,11 @@ class CEEInfo : public ICorJitInfo TransientMethodDetails RemoveTransientMethodDetails(MethodDesc* pMD); bool FindTransientMethodDetails(MethodDesc* pMD, TransientMethodDetails** details); - // Get method info for a transient method - void getTransientMethodInfo(MethodDesc* pMD, CORINFO_METHOD_INFO* methInfo); +protected: + void getMethodInfoWorker( + MethodInfoWorkerContext& cxt, + CORINFO_METHOD_INFO* methInfo, + CORINFO_CONTEXT_HANDLE exactContext = NULL); protected: SArray* m_pJitHandles; // GC handles used by JIT @@ -517,7 +517,7 @@ class CEECodeGenInfo : public CEEInfo { public: // ICorJitInfo stuff - CEECodeGenInfo(MethodDesc* fd, COR_ILMETHOD_DECODER* header, EECodeGenManager* jm, bool allowInlining = true) + CEECodeGenInfo(MethodDesc* fd, COR_ILMETHOD_DECODER* header, EECodeGenManager* jm, bool allowInlining) : CEEInfo(fd, allowInlining), m_jitManager(jm), m_CodeHeader(NULL), @@ -642,6 +642,7 @@ class CEECodeGenInfo : public CEEInfo InfoAccessType emptyStringLiteral(void ** ppValue) override; CORINFO_CLASS_HANDLE getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* pIsSpeculative) override; void* getMethodSync(CORINFO_METHOD_HANDLE ftnHnd, void **ppIndirection) override; + CORINFO_METHOD_INFO getMethodInfo(MethodInfoWorkerContext& cxt); protected: @@ -672,7 +673,7 @@ class CEECodeGenInfo : public CEEInfo size_t m_codeWriteBufferSize; BYTE* m_pRealCodeHeader; HeapList* m_pCodeHeap; - COR_ILMETHOD_DECODER * m_ILHeader; // the code header as exist in the file + COR_ILMETHOD_DECODER* m_ILHeader; // the code header to use. This may have been generated due to dynamic IL generation. #if defined(_DEBUG) ULONG m_codeSize; // Code size requested via allocMem @@ -880,7 +881,7 @@ class CEEJitInfo final : public CEECodeGenInfo void PublishFinalCodeAddress(PCODE addr); CEEJitInfo(MethodDesc* fd, COR_ILMETHOD_DECODER* header, - EECodeGenManager* jm, bool allowInlining = true) + EECodeGenManager* jm, bool allowInlining) : CEECodeGenInfo(fd, header, jm, allowInlining) #ifdef FEATURE_EH_FUNCLETS , m_moduleBase(0), @@ -1010,7 +1011,7 @@ class CInterpreterJitInfo final : public CEECodeGenInfo // ICorJitInfo stuff CInterpreterJitInfo(MethodDesc* fd, COR_ILMETHOD_DECODER* header, - EECodeGenManager* jm, bool allowInlining = true) + EECodeGenManager* jm, bool allowInlining) : CEECodeGenInfo(fd, header, jm, allowInlining) { CONTRACTL From d4462370e4dcb8745617eb5e2d4b9138b25f4548 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Thu, 15 May 2025 18:39:29 -0700 Subject: [PATCH 02/11] Fix build break. --- src/coreclr/vm/jitinterface.cpp | 6 +++--- src/coreclr/vm/jitinterface.h | 5 ++++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index d48630bc3be6bc..e0e86f82b97a28 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -12293,7 +12293,7 @@ void* CEECodeGenInfo::getMethodSync(CORINFO_METHOD_HANDLE ftnHnd, return result; } -CORINFO_METHOD_INFO CEECodeGenInfo::getMethodInfo( +CORINFO_METHOD_INFO CEECodeGenInfo::getMethodInfoWithContext( MethodInfoWorkerContext& cxt) { STANDARD_VM_CONTRACT; @@ -13413,7 +13413,7 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, if (interpreterMgr->IsInterpreterLoaded()) { CInterpreterJitInfo interpreterJitInfo{ ftn, ILHeader, interpreterMgr, enableInlining }; - methodInfo = interpreterJitInfo.getMethodInfo(cxt); + methodInfo = interpreterJitInfo.getMethodInfoWithContext(cxt); *pJitFlags = GetCompileFlags(config, ftn, &methodInfo); ret = UnsafeJitFunctionWorker(interpreterMgr, &interpreterJitInfo, pJitFlags, methodInfo, &cxt, nativeCodeVersion, pSizeOfCode); @@ -13446,7 +13446,7 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, while (true) { CEEJitInfo jitInfo{ ftn, ILHeader, jitMgr, enableInlining }; - methodInfo = jitInfo.getMethodInfo(cxt); + methodInfo = jitInfo.getMethodInfoWithContext(cxt); *pJitFlags = GetCompileFlags(config, ftn, &methodInfo); jsoCheck.Enable(jitInfo); diff --git a/src/coreclr/vm/jitinterface.h b/src/coreclr/vm/jitinterface.h index 728e9f554ff13c..83dad435f8cfef 100644 --- a/src/coreclr/vm/jitinterface.h +++ b/src/coreclr/vm/jitinterface.h @@ -642,7 +642,10 @@ class CEECodeGenInfo : public CEEInfo InfoAccessType emptyStringLiteral(void ** ppValue) override; CORINFO_CLASS_HANDLE getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* pIsSpeculative) override; void* getMethodSync(CORINFO_METHOD_HANDLE ftnHnd, void **ppIndirection) override; - CORINFO_METHOD_INFO getMethodInfo(MethodInfoWorkerContext& cxt); + + // This is an internal helper used on the VM side. It should be called + // shortly after creating an instance of CEECodeGenInfo. + CORINFO_METHOD_INFO getMethodInfoWithContext(MethodInfoWorkerContext& cxt); protected: From 5c762a927df1fcfb57895c6280f458716d8f2278 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Fri, 16 May 2025 10:19:03 -0700 Subject: [PATCH 03/11] Feedback --- src/coreclr/utilcode/format1.cpp | 6 +-- src/coreclr/vm/jitinterface.cpp | 69 ++++++++++++++------------------ src/coreclr/vm/jitinterface.h | 10 ++--- src/coreclr/vm/prestub.cpp | 6 +-- 4 files changed, 38 insertions(+), 53 deletions(-) diff --git a/src/coreclr/utilcode/format1.cpp b/src/coreclr/utilcode/format1.cpp index f8ef94406bfb93..306248465e76f4 100644 --- a/src/coreclr/utilcode/format1.cpp +++ b/src/coreclr/utilcode/format1.cpp @@ -61,15 +61,13 @@ COR_ILMETHOD_DECODER::COR_ILMETHOD_DECODER( fErrorInInit = true; Code = 0; SetLocalVarSigTok(0); - if (wbStatus != NULL) - { - *wbStatus = FORMAT_ERROR; - } } PAL_ENDTRY if (fErrorInInit) { + if (wbStatus != NULL) + *wbStatus = FORMAT_ERROR; return; } diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index e0e86f82b97a28..afdb749037be9d 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -7494,20 +7494,21 @@ static bool getILIntrinsicImplementationForActivator(MethodDesc* ftn, //--------------------------------------------------------------------------------------- // -void CEEInfo::getMethodInfoWorker( - MethodInfoWorkerContext& cxt, +COR_ILMETHOD_DECODER* CEEInfo::getMethodInfoWorker( + MethodDesc* ftn, + COR_ILMETHOD_DECODER* header, CORINFO_METHOD_INFO* methInfo, CORINFO_CONTEXT_HANDLE exactContext) { STANDARD_VM_CONTRACT; - _ASSERTE(cxt.Method != NULL); + _ASSERTE(ftn != NULL); _ASSERTE(methInfo != NULL); - MethodDesc* ftn = cxt.Method; methInfo->ftn = (CORINFO_METHOD_HANDLE)ftn; methInfo->regionKind = CORINFO_REGION_JIT; - _ASSERTE(!cxt.HasTransientMethodDetails()); + MethodInfoWorkerContext cxt{ ftn, header }; + TransientMethodDetails* detailsMaybe = NULL; if (FindTransientMethodDetails(ftn, &detailsMaybe)) cxt.UpdateWith(*detailsMaybe); @@ -7663,6 +7664,8 @@ void CEEInfo::getMethodInfoWorker( CONV_TO_JITSIG_FLAGS_LOCALSIG, &methInfo->locals); + COR_ILMETHOD_DECODER* ilHeader = cxt.Header; + // If we have transient method details we need to handle // the lifetime of the details. if (cxt.HasTransientMethodDetails()) @@ -7676,6 +7679,8 @@ void CEEInfo::getMethodInfoWorker( // transferred or was found in this instance. cxt.UpdateWith({}); } + + return ilHeader; } // getMethodInfoWorker //--------------------------------------------------------------------------------------- @@ -7697,20 +7702,17 @@ CEEInfo::getMethodInfo( JIT_TO_EE_TRANSITION(); MethodDesc* ftn = GetMethod(ftnHnd); - - MethodInfoWorkerContext cxt{ ftn }; if (ftn->IsDynamicMethod() || (ftn->IsIL() && ftn->GetRVA() == 0)) // IL methods with no RVA indicate there is no implementation defined in metadata. { - getMethodInfoWorker(cxt, methInfo, context); + getMethodInfoWorker(ftn, NULL, methInfo, context); result = true; } else if (!ftn->IsWrapperStub() && ftn->HasILHeader()) { // Get the IL header and set it. COR_ILMETHOD_DECODER header(ftn->GetILHeader(), ftn->GetMDImport(), NULL); - cxt.Header = &header; - getMethodInfoWorker(cxt, methInfo, context); + getMethodInfoWorker(ftn, &header, methInfo, context); result = true; } @@ -7893,9 +7895,8 @@ CorInfoInline CEEInfo::canInline (CORINFO_METHOD_HANDLE hCaller, } else if (pCallee->IsIL() && pCallee->GetRVA() == 0) { - MethodInfoWorkerContext cxt{ pCallee }; CORINFO_METHOD_INFO methodInfo; - getMethodInfoWorker(cxt, &methodInfo); + getMethodInfoWorker(pCallee, NULL, &methodInfo); if (methodInfo.EHcount > 0) { result = INLINE_FAIL; @@ -12293,23 +12294,18 @@ void* CEECodeGenInfo::getMethodSync(CORINFO_METHOD_HANDLE ftnHnd, return result; } -CORINFO_METHOD_INFO CEECodeGenInfo::getMethodInfoWithContext( - MethodInfoWorkerContext& cxt) +CORINFO_METHOD_INFO CEECodeGenInfo::getMethodInfoInternal() { STANDARD_VM_CONTRACT; - _ASSERTE(cxt.Method != NULL); CORINFO_METHOD_INFO methInfo; - getMethodInfoWorker(cxt, &methInfo); - - // Verify method attributes and signature are consistant - _ASSERTE(!!cxt.Method->IsStatic() == ((methInfo.args.callConv & CORINFO_CALLCONV_HASTHIS) == 0)); + COR_ILMETHOD_DECODER* ilHeader = getMethodInfoWorker(m_pMethodBeingCompiled, m_ILHeader, &methInfo); // Either the member header was NULL or remains the same as the input. - _ASSERTE(m_ILHeader == NULL || cxt.Header == m_ILHeader); + _ASSERTE(m_ILHeader == NULL || ilHeader == m_ILHeader); // Update the member with the result of the MethodInfo call. - m_ILHeader = cxt.Header; + m_ILHeader = ilHeader; return methInfo; } @@ -13072,12 +13068,15 @@ void ThrowExceptionForJit(HRESULT res) // ******************************************************************** //#define PERF_TRACK_METHOD_JITTIMES -static TADDR UnsafeJitFunctionWorker(EECodeGenManager *pJitMgr, CEECodeGenInfo *pJitInfo, CORJIT_FLAGS* pJitFlags, CORINFO_METHOD_INFO methodInfo, - MethodInfoWorkerContext *pCxt, NativeCodeVersion nativeCodeVersion, ULONG* pSizeOfCode) +static TADDR UnsafeJitFunctionWorker( + EECodeGenManager *pJitMgr, + CEECodeGenInfo *pJitInfo, + CORJIT_FLAGS jitFlags, + CORINFO_METHOD_INFO methodInfo, + NativeCodeVersion nativeCodeVersion, + ULONG* pSizeOfCode) { STANDARD_VM_CONTRACT; - if (pCxt->HasTransientMethodDetails()) - pJitInfo->AddTransientMethodDetails(pCxt->CreateTransientMethodDetails()); MethodDesc * pMethodForSecurity = pJitInfo->GetMethodForSecurity(methodInfo.ftn); MethodDesc * ftn = nativeCodeVersion.GetMethodDesc(); @@ -13142,7 +13141,7 @@ static TADDR UnsafeJitFunctionWorker(EECodeGenManager *pJitMgr, CEECodeGenInfo * res = invokeCompileMethod(pJitMgr, pJitInfo, &methodInfo, - *pJitFlags, + jitFlags, &nativeEntry, &sizeOfCode); @@ -13395,7 +13394,6 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, bool enableInlining = !config->GetJitCompilationFlags().IsSet(CORJIT_FLAGS::CORJIT_FLAG_NO_INLINING); CORINFO_METHOD_INFO methodInfo{}; - MethodInfoWorkerContext cxt{ ftn, ILHeader }; #ifdef FEATURE_INTERPRETER InterpreterJitManager* interpreterMgr = ExecutionManager::GetInterpreterJitManager(); @@ -13413,10 +13411,10 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, if (interpreterMgr->IsInterpreterLoaded()) { CInterpreterJitInfo interpreterJitInfo{ ftn, ILHeader, interpreterMgr, enableInlining }; - methodInfo = interpreterJitInfo.getMethodInfoWithContext(cxt); + methodInfo = interpreterJitInfo.getMethodInfoInternal(); *pJitFlags = GetCompileFlags(config, ftn, &methodInfo); - ret = UnsafeJitFunctionWorker(interpreterMgr, &interpreterJitInfo, pJitFlags, methodInfo, &cxt, nativeCodeVersion, pSizeOfCode); + ret = UnsafeJitFunctionWorker(interpreterMgr, &interpreterJitInfo, *pJitFlags, methodInfo, nativeCodeVersion, pSizeOfCode); } #endif // FEATURE_INTERPRETER @@ -13446,9 +13444,10 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, while (true) { CEEJitInfo jitInfo{ ftn, ILHeader, jitMgr, enableInlining }; - methodInfo = jitInfo.getMethodInfoWithContext(cxt); + methodInfo = jitInfo.getMethodInfoInternal(); *pJitFlags = GetCompileFlags(config, ftn, &methodInfo); + // Enable the jump stub overflow check jsoCheck.Enable(jitInfo); #ifdef FEATURE_ON_STACK_REPLACEMENT @@ -13461,19 +13460,13 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, } #endif // FEATURE_ON_STACK_REPLACEMENT - TADDR retMaybe = UnsafeJitFunctionWorker(jitMgr, &jitInfo, pJitFlags, methodInfo, &cxt, nativeCodeVersion, pSizeOfCode); + TADDR retMaybe = UnsafeJitFunctionWorker(jitMgr, &jitInfo, *pJitFlags, methodInfo, nativeCodeVersion, pSizeOfCode); if (!retMaybe) COMPlusThrow(kInvalidProgramException); + // If we detect a jump stub overflow, we need to retry. if (jsoCheck.Detected(jitInfo, jitMgr)) - { - // Get any transient method details and take ownership - // from the JITInfo instance. We are going to be recreating - // a new JITInfo and will reuse these details there. - TransientMethodDetails details = jitInfo.RemoveTransientMethodDetails(ftn); - cxt.TakeOwnership(std::move(details)); continue; - } ret = PINSTRToPCODE(retMaybe); jitInfo.PublishFinalCodeAddress(ret); diff --git a/src/coreclr/vm/jitinterface.h b/src/coreclr/vm/jitinterface.h index 83dad435f8cfef..002484b6f10cc1 100644 --- a/src/coreclr/vm/jitinterface.h +++ b/src/coreclr/vm/jitinterface.h @@ -334,9 +334,6 @@ struct TransientMethodDetails final TransientMethodDetails& operator=(TransientMethodDetails&&); }; -// Forward declaration -class MethodInfoWorkerContext; - class CEEInfo : public ICorJitInfo { friend class CEEDynamicCodeInfo; @@ -470,8 +467,9 @@ class CEEInfo : public ICorJitInfo bool FindTransientMethodDetails(MethodDesc* pMD, TransientMethodDetails** details); protected: - void getMethodInfoWorker( - MethodInfoWorkerContext& cxt, + COR_ILMETHOD_DECODER* getMethodInfoWorker( + MethodDesc* ftn, + COR_ILMETHOD_DECODER* header, CORINFO_METHOD_INFO* methInfo, CORINFO_CONTEXT_HANDLE exactContext = NULL); @@ -645,7 +643,7 @@ class CEECodeGenInfo : public CEEInfo // This is an internal helper used on the VM side. It should be called // shortly after creating an instance of CEECodeGenInfo. - CORINFO_METHOD_INFO getMethodInfoWithContext(MethodInfoWorkerContext& cxt); + CORINFO_METHOD_INFO getMethodInfoInternal(); protected: diff --git a/src/coreclr/vm/prestub.cpp b/src/coreclr/vm/prestub.cpp index 57b0af8fdbb755..6e728f947632d9 100644 --- a/src/coreclr/vm/prestub.cpp +++ b/src/coreclr/vm/prestub.cpp @@ -743,11 +743,7 @@ namespace return NULL; COR_ILMETHOD_DECODER::DecoderStatus status = COR_ILMETHOD_DECODER::FORMAT_ERROR; - { - // Decoder ctor can AV on a malformed method header - AVInRuntimeImplOkayHolder AVOkay; - pHeader = new (pDecoderMemory) COR_ILMETHOD_DECODER(ilHeader, pMD->GetMDImport(), &status); - } + pHeader = new (pDecoderMemory) COR_ILMETHOD_DECODER(ilHeader, pMD->GetMDImport(), &status); if (status == COR_ILMETHOD_DECODER::FORMAT_ERROR) COMPlusThrowHR(COR_E_BADIMAGEFORMAT, BFA_BAD_IL); From 46eacd3323c6d3f63a8e613333912d9808336cbb Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Fri, 16 May 2025 10:50:29 -0700 Subject: [PATCH 04/11] Change logging approach. --- src/coreclr/vm/jitinterface.cpp | 55 ++++++++++++++++----------------- 1 file changed, 26 insertions(+), 29 deletions(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index afdb749037be9d..7a1f9ca43e3395 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -13313,42 +13313,39 @@ class JumpStubOverflowCheck final BOOL JumpStubOverflowCheck::g_fAllowRel32 = TRUE; #endif // TARGET_AMD64 -#ifdef LOGGING -class LogJitMethod final +static void LogJitMethodBegin(MethodDesc* ftn) { - MethodDesc* _ftn; -public: - LogJitMethod(MethodDesc* ftn) - : _ftn{ ftn } + STANDARD_VM_CONTRACT; + _ASSERTE(ftn != NULL); +#ifdef LOGGING + if (ftn->IsNoMetadata()) { - STANDARD_VM_CONTRACT; - _ASSERTE(_ftn != NULL); - if (_ftn->IsNoMetadata()) + if (ftn->IsILStub()) { - if (_ftn->IsILStub()) - { - LOG((LF_JIT, LL_INFO10000, "{ Jitting IL Stub (%p)\n", _ftn)); - } - else - { - LOG((LF_JIT, LL_INFO10000, "{ Jitting dynamic method (%p)\n", _ftn)); - } + LOG((LF_JIT, LL_INFO10000, "{ Jitting IL Stub (%p)\n", ftn)); } else { - LPCUTF8 cls = _ftn->GetMethodTable()->GetDebugClassName(); - LPCUTF8 name = _ftn->GetName(); - LOG((LF_JIT, LL_INFO10000, "{ Jitting method (%p) %s::%s %s\n", _ftn, cls, name, _ftn->m_pszDebugMethodSignature)); + LOG((LF_JIT, LL_INFO10000, "{ Jitting dynamic method (%p)\n", ftn)); } } - - ~LogJitMethod() + else { - STANDARD_VM_CONTRACT; - LOG((LF_JIT, LL_INFO10000, "Done Jitting method (%p) }\n", _ftn)); + LPCUTF8 cls = ftn->GetMethodTable()->GetDebugClassName(); + LPCUTF8 name = ftn->GetName(); + LOG((LF_JIT, LL_INFO10000, "{ Jitting method (%p) %s::%s %s\n", ftn, cls, name, ftn->m_pszDebugMethodSignature)); } -}; #endif // LOGGING +} + +static void LogJitMethodEnd(MethodDesc* ftn) +{ + STANDARD_VM_CONTRACT; + _ASSERTE(ftn != NULL); +#ifdef LOGGING + LOG((LF_JIT, LL_INFO10000, "Done Jitting method (%p) }\n", ftn)); +#endif // LOGGING +} // ******************************************************************** // README!! @@ -13378,16 +13375,14 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, // If it's generic then we can only enter through an instantiated MethodDesc _ASSERTE(!ftn->IsGenericMethodDefinition()); -#ifdef LOGGING - LogJitMethod{ ftn }; -#endif // LOGGING - PCODE ret = (PCODE)NULL; NormalizedTimer timer; int64_t c100nsTicksInJit = 0; COOPERATIVE_TRANSITION_BEGIN(); + LogJitMethodBegin(ftn); + timer.Start(); // Negate the check for no inlining so the logic is easier to follow @@ -13498,6 +13493,8 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, InterlockedIncrement64((LONG64*)&g_cMethodsJitted); t_cMethodsJittedForThread++; + LogJitMethodEnd(ftn); + COOPERATIVE_TRANSITION_END(); return ret; } From bd7682178e192eeba8addb8d9d5db98d90f1a8dd Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Fri, 16 May 2025 12:08:56 -0700 Subject: [PATCH 05/11] Move initialization to ctor instead of in a secondary function. --- src/coreclr/vm/jitinterface.cpp | 13 ++----------- src/coreclr/vm/jitinterface.h | 18 +++++++++--------- 2 files changed, 11 insertions(+), 20 deletions(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 7a1f9ca43e3395..6533716c9101c5 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -12294,19 +12294,10 @@ void* CEECodeGenInfo::getMethodSync(CORINFO_METHOD_HANDLE ftnHnd, return result; } -CORINFO_METHOD_INFO CEECodeGenInfo::getMethodInfoInternal() +CORINFO_METHOD_INFO CEECodeGenInfo::getMethodInfoInternal() const { STANDARD_VM_CONTRACT; - - CORINFO_METHOD_INFO methInfo; - COR_ILMETHOD_DECODER* ilHeader = getMethodInfoWorker(m_pMethodBeingCompiled, m_ILHeader, &methInfo); - - // Either the member header was NULL or remains the same as the input. - _ASSERTE(m_ILHeader == NULL || ilHeader == m_ILHeader); - - // Update the member with the result of the MethodInfo call. - m_ILHeader = ilHeader; - return methInfo; + return m_MethodInfo; } /*********************************************************************/ diff --git a/src/coreclr/vm/jitinterface.h b/src/coreclr/vm/jitinterface.h index 002484b6f10cc1..b989ed632fa542 100644 --- a/src/coreclr/vm/jitinterface.h +++ b/src/coreclr/vm/jitinterface.h @@ -524,6 +524,7 @@ class CEECodeGenInfo : public CEEInfo m_pRealCodeHeader(NULL), m_pCodeHeap(NULL), m_ILHeader(header), + m_MethodInfo(), m_GCinfo_len(0), m_EHinfo_len(0), m_iOffsetMapping(0), @@ -536,12 +537,12 @@ class CEECodeGenInfo : public CEEInfo m_numRichOffsetMappings(0), m_gphCache() { - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - MODE_ANY; - } CONTRACTL_END; + STANDARD_VM_CONTRACT; + COR_ILMETHOD_DECODER* ilHeader = getMethodInfoWorker(m_pMethodBeingCompiled, m_ILHeader, &m_MethodInfo); + + // Either the member header was NULL or remains the same as the input. + _ASSERTE(m_ILHeader == NULL || ilHeader == m_ILHeader); + m_ILHeader = ilHeader; } ~CEECodeGenInfo() @@ -641,9 +642,7 @@ class CEECodeGenInfo : public CEEInfo CORINFO_CLASS_HANDLE getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* pIsSpeculative) override; void* getMethodSync(CORINFO_METHOD_HANDLE ftnHnd, void **ppIndirection) override; - // This is an internal helper used on the VM side. It should be called - // shortly after creating an instance of CEECodeGenInfo. - CORINFO_METHOD_INFO getMethodInfoInternal(); + CORINFO_METHOD_INFO getMethodInfoInternal() const; protected: @@ -675,6 +674,7 @@ class CEECodeGenInfo : public CEEInfo BYTE* m_pRealCodeHeader; HeapList* m_pCodeHeap; COR_ILMETHOD_DECODER* m_ILHeader; // the code header to use. This may have been generated due to dynamic IL generation. + CORINFO_METHOD_INFO m_MethodInfo; #if defined(_DEBUG) ULONG m_codeSize; // Code size requested via allocMem From 221242d2555df42bc5096407595e56a1df76b154 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Fri, 16 May 2025 14:39:31 -0700 Subject: [PATCH 06/11] Remove incorrect assert. --- src/coreclr/vm/jitinterface.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/jitinterface.h b/src/coreclr/vm/jitinterface.h index b989ed632fa542..c159029667e1b3 100644 --- a/src/coreclr/vm/jitinterface.h +++ b/src/coreclr/vm/jitinterface.h @@ -540,8 +540,9 @@ class CEECodeGenInfo : public CEEInfo STANDARD_VM_CONTRACT; COR_ILMETHOD_DECODER* ilHeader = getMethodInfoWorker(m_pMethodBeingCompiled, m_ILHeader, &m_MethodInfo); - // Either the member header was NULL or remains the same as the input. - _ASSERTE(m_ILHeader == NULL || ilHeader == m_ILHeader); + // The header maybe replaced during the call to getMethodInfoWorker. This is most probable + // when the input is null (that is, no metadata), but we can also examine the method and generate + // an entirely new IL body (for example, intrinsic methods). m_ILHeader = ilHeader; } From 35462fb7434783c1821d7bf28355c85b42ccd408 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 21 May 2025 12:20:40 -0700 Subject: [PATCH 07/11] Feedback --- src/coreclr/vm/jitinterface.cpp | 353 +++++++++++++++++--------------- src/coreclr/vm/jitinterface.h | 50 +---- 2 files changed, 204 insertions(+), 199 deletions(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index c839ba02e747c2..ed881b2a5eeb94 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -6671,16 +6671,6 @@ class MethodInfoWorkerContext final Header = details.Header; TransientResolver = details.Scope != NULL ? GetDynamicResolver(details.Scope) : NULL; } - - void TakeOwnership(TransientMethodDetails details) - { - LIMITED_METHOD_CONTRACT; - UpdateWith(details); - - // Default initialize local instance since we are - // taking ownership of the transient method details. - details = {}; - } }; void getMethodInfoILMethodHeaderHelper( @@ -7507,19 +7497,22 @@ COR_ILMETHOD_DECODER* CEEInfo::getMethodInfoWorker( methInfo->ftn = (CORINFO_METHOD_HANDLE)ftn; methInfo->regionKind = CORINFO_REGION_JIT; - MethodInfoWorkerContext cxt{ ftn, header }; - - TransientMethodDetails* detailsMaybe = NULL; - if (FindTransientMethodDetails(ftn, &detailsMaybe)) - cxt.UpdateWith(*detailsMaybe); - CORINFO_MODULE_HANDLE scopeHnd = NULL; /* Grab information from the IL header */ SigPointer localSig{}; - // Having a header means there is backing IL for the method. - if (NULL != cxt.Header) + MethodInfoWorkerContext cxt{ ftn, header }; + + TransientMethodDetails* detailsMaybe = NULL; + if (FindTransientMethodDetails(ftn, &detailsMaybe)) + { + cxt.UpdateWith(*detailsMaybe); + scopeHnd = cxt.CreateScopeHandle(); + getMethodInfoILMethodHeaderHelper(cxt.Header, methInfo); + localSig = SigPointer{ cxt.Header->LocalVarSig, cxt.Header->cbLocalVarSig }; + } + else if (NULL != cxt.Header) // Having a header means there is backing IL for the method. { bool fILIntrinsic = false; MethodTable* pMT = ftn->GetMethodTable(); @@ -7702,8 +7695,7 @@ CEEInfo::getMethodInfo( JIT_TO_EE_TRANSITION(); MethodDesc* ftn = GetMethod(ftnHnd); - if (ftn->IsDynamicMethod() - || (ftn->IsIL() && ftn->GetRVA() == 0)) // IL methods with no RVA indicate there is no implementation defined in metadata. + if (ftn->IsDynamicMethod()) { getMethodInfoWorker(ftn, NULL, methInfo, context); result = true; @@ -7715,6 +7707,11 @@ CEEInfo::getMethodInfo( getMethodInfoWorker(ftn, &header, methInfo, context); result = true; } + else if (ftn->IsIL() && ftn->GetRVA() == 0) // IL methods with no RVA indicate there is no implementation defined in metadata. + { + getMethodInfoWorker(ftn, NULL, methInfo, context); + result = true; + } if (result) { @@ -10716,8 +10713,146 @@ bool CEEInfo::logMsg(unsigned level, const char* fmt, va_list args) return result; } - /*********************************************************************/ +static CORJIT_FLAGS GetCompileFlags(PrepareCodeConfig* prepareConfig, MethodDesc* ftn, CORINFO_METHOD_INFO* methodInfo) +{ + STANDARD_VM_CONTRACT; + _ASSERTE(prepareConfig != NULL); + _ASSERTE(ftn != NULL); + _ASSERTE(methodInfo->regionKind == CORINFO_REGION_JIT); + + CORJIT_FLAGS flags = prepareConfig->GetJitCompilationFlags(); + + // + // Get the compile flags that are shared between JIT and NGen + // + flags.Add(CEEInfo::GetBaseCompileFlags(ftn)); + + if (ftn->IsAsyncMethod()) + flags.Add(CORJIT_FLAGS::CORJIT_FLAG_ASYNC); + + // + // Get CPU specific flags + // + flags.Add(ExecutionManager::GetEEJitManager()->GetCPUCompileFlags()); + + // + // Find the debugger and profiler related flags + // + +#ifdef DEBUGGING_SUPPORTED + flags.Add(GetDebuggerCompileFlags(ftn->GetModule(), flags)); +#endif + +#ifdef PROFILING_SUPPORTED + if (CORProfilerTrackEnterLeave() && !ftn->IsNoMetadata()) + flags.Set(CORJIT_FLAGS::CORJIT_FLAG_PROF_ENTERLEAVE); + + if (CORProfilerTrackTransitions()) + flags.Set(CORJIT_FLAGS::CORJIT_FLAG_PROF_NO_PINVOKE_INLINE); +#endif // PROFILING_SUPPORTED + + // Don't allow allocations on FOH from collectible contexts to avoid memory leaks + if (!ftn->GetLoaderAllocator()->CanUnload()) + { + flags.Set(CORJIT_FLAGS::CORJIT_FLAG_FROZEN_ALLOC_ALLOWED); + } + + // Set optimization flags + if (!flags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_MIN_OPT)) + { + unsigned optType = g_pConfig->GenOptimizeType(); + _ASSERTE(optType <= OPT_RANDOM); + + if (optType == OPT_RANDOM) + optType = methodInfo->ILCodeSize % OPT_RANDOM; + + if (g_pConfig->JitMinOpts()) + { + flags.Set(CORJIT_FLAGS::CORJIT_FLAG_MIN_OPT); + } + else + { + if (!flags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_TIER0)) + flags.Set(CORJIT_FLAGS::CORJIT_FLAG_BBOPT); + } + + if (optType == OPT_SIZE) + { + flags.Set(CORJIT_FLAGS::CORJIT_FLAG_SIZE_OPT); + } + else if (optType == OPT_SPEED) + { + flags.Set(CORJIT_FLAGS::CORJIT_FLAG_SPEED_OPT); + } + } + + if (IsDynamicScope(methodInfo->scope)) + { + // no debug info available for IL stubs + if (!g_pConfig->GetTrackDynamicMethodDebugInfo()) + flags.Clear(CORJIT_FLAGS::CORJIT_FLAG_DEBUG_INFO); + + DynamicResolver* pResolver = GetDynamicResolver(methodInfo->scope); + flags.Add(pResolver->GetJitFlags()); + } + +#ifdef ARM_SOFTFP + flags.Set(CORJIT_FLAGS::CORJIT_FLAG_SOFTFP_ABI); +#endif // ARM_SOFTFP + +#ifdef FEATURE_PGO + + if (CLRConfig::GetConfigValue(CLRConfig::INTERNAL_ReadPGOData) > 0) + { + flags.Set(CORJIT_FLAGS::CORJIT_FLAG_BBOPT); + } + else if (g_pConfig->TieredPGO() && flags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_TIER1)) + { + flags.Set(CORJIT_FLAGS::CORJIT_FLAG_BBOPT); + } + +#endif + + return flags; +} + +CEECodeGenInfo::CEECodeGenInfo(PrepareCodeConfig* config, MethodDesc* fd, COR_ILMETHOD_DECODER* header, EECodeGenManager* jm) + : CEEInfo(fd) + , m_jitManager(jm) + , m_CodeHeader(NULL) + , m_CodeHeaderRW(NULL) + , m_codeWriteBufferSize(0) + , m_pRealCodeHeader(NULL) + , m_pCodeHeap(NULL) + , m_ILHeader(header) + , m_MethodInfo() + , m_GCinfo_len(0) + , m_EHinfo_len(0) + , m_iOffsetMapping(0) + , m_pOffsetMapping(NULL) + , m_iNativeVarInfo(0) + , m_pNativeVarInfo(NULL) + , m_inlineTreeNodes(NULL) + , m_numInlineTreeNodes(0) + , m_richOffsetMappings(NULL) + , m_numRichOffsetMappings(0) + , m_gphCache() +{ + STANDARD_VM_CONTRACT; + _ASSERTE(config != NULL); + COR_ILMETHOD_DECODER* ilHeader = getMethodInfoWorker(m_pMethodBeingCompiled, m_ILHeader, &m_MethodInfo); + + // The header maybe replaced during the call to getMethodInfoWorker. This is most probable + // when the input is null (that is, no metadata), but we can also examine the method and generate + // an entirely new IL body (for example, intrinsic methods). + m_ILHeader = ilHeader; + + m_jitFlags = GetCompileFlags(config, m_pMethodBeingCompiled, &m_MethodInfo); + + // Negate the check for no inlining so the logic is easier to follow + m_allowInlining = !m_jitFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_NO_INLINING); +} void* CEECodeGenInfo::getHelperFtn(CorInfoHelpFunc ftnNum, /* IN */ void ** ppIndirection) /* OUT */ @@ -12297,10 +12432,16 @@ void* CEECodeGenInfo::getMethodSync(CORINFO_METHOD_HANDLE ftnHnd, return result; } -CORINFO_METHOD_INFO CEECodeGenInfo::getMethodInfoInternal() const +CORINFO_METHOD_INFO* CEECodeGenInfo::getMethodInfoInternal() +{ + STANDARD_VM_CONTRACT; + return &m_MethodInfo; +} + +CORJIT_FLAGS* CEECodeGenInfo::getJitFlagsInternal() { STANDARD_VM_CONTRACT; - return m_MethodInfo; + return &m_jitFlags; } /*********************************************************************/ @@ -12725,10 +12866,9 @@ void CEECodeGenInfo::getEHinfo( EE_TO_JIT_TRANSITION(); } -CorJitResult invokeCompileMethodHelper(EECodeGenManager *jitMgr, +static CorJitResult invokeCompileMethodHelper(EECodeGenManager *jitMgr, CEECodeGenInfo *comp, struct CORINFO_METHOD_INFO *info, - CORJIT_FLAGS jitFlags, BYTE **nativeEntry, uint32_t *nativeSizeOfCode) { @@ -12738,12 +12878,11 @@ CorJitResult invokeCompileMethodHelper(EECodeGenManager *jitMgr, CorJitResult ret = CORJIT_SKIPPED; // Note that CORJIT_SKIPPED is an error exit status code - comp->setJitFlags(jitFlags); - #if defined(ALLOW_SXS_JIT) ICorJitCompiler *jitCompiler = jitMgr->GetAltCompiler(); if (jitCompiler != NULL) { + CORJIT_FLAGS jitFlags = *comp->getJitFlagsInternal(); CORJIT_FLAGS altJitFlags = jitFlags; altJitFlags.Set(CORJIT_FLAGS::CORJIT_FLAG_ALT_JIT); comp->setJitFlags(altJitFlags); @@ -12760,11 +12899,12 @@ CorJitResult invokeCompileMethodHelper(EECodeGenManager *jitMgr, comp->ResetForJitRetry(); ret = CORJIT_SKIPPED; } + + // Restore the original JIT flags + comp->setJitFlags(jitFlags); } #endif // defined(ALLOW_SXS_JIT) - comp->setJitFlags(jitFlags); - // CORJIT_SKIPPED also evaluates as "FAILED" if (FAILED(ret)) { @@ -12796,12 +12936,10 @@ CorJitResult invokeCompileMethodHelper(EECodeGenManager *jitMgr, return ret; } - /*********************************************************************/ -CorJitResult invokeCompileMethod(EECodeGenManager *jitMgr, +static CorJitResult invokeCompileMethod(EECodeGenManager *jitMgr, CEECodeGenInfo *comp, struct CORINFO_METHOD_INFO *info, - CORJIT_FLAGS jitFlags, BYTE **nativeEntry, uint32_t *nativeSizeOfCode) { @@ -12816,7 +12954,7 @@ CorJitResult invokeCompileMethod(EECodeGenManager *jitMgr, GCX_PREEMP(); - CorJitResult ret = invokeCompileMethodHelper(jitMgr, comp, info, jitFlags, nativeEntry, nativeSizeOfCode); + CorJitResult ret = invokeCompileMethodHelper(jitMgr, comp, info, nativeEntry, nativeSizeOfCode); // // Verify that we are still in preemptive mode when we return @@ -12925,109 +13063,6 @@ CORJIT_FLAGS GetDebuggerCompileFlags(Module* pModule, CORJIT_FLAGS flags) return flags; } -static CORJIT_FLAGS GetCompileFlags(PrepareCodeConfig* prepareConfig, MethodDesc* ftn, CORINFO_METHOD_INFO* methodInfo) -{ - STANDARD_VM_CONTRACT; - _ASSERTE(prepareConfig != NULL); - _ASSERTE(ftn != NULL); - _ASSERTE(methodInfo->regionKind == CORINFO_REGION_JIT); - - CORJIT_FLAGS flags = prepareConfig->GetJitCompilationFlags(); - - // - // Get the compile flags that are shared between JIT and NGen - // - flags.Add(CEEInfo::GetBaseCompileFlags(ftn)); - - if (ftn->IsAsyncMethod()) - flags.Add(CORJIT_FLAGS::CORJIT_FLAG_ASYNC); - - // - // Get CPU specific flags - // - flags.Add(ExecutionManager::GetEEJitManager()->GetCPUCompileFlags()); - - // - // Find the debugger and profiler related flags - // - -#ifdef DEBUGGING_SUPPORTED - flags.Add(GetDebuggerCompileFlags(ftn->GetModule(), flags)); -#endif - -#ifdef PROFILING_SUPPORTED - if (CORProfilerTrackEnterLeave() && !ftn->IsNoMetadata()) - flags.Set(CORJIT_FLAGS::CORJIT_FLAG_PROF_ENTERLEAVE); - - if (CORProfilerTrackTransitions()) - flags.Set(CORJIT_FLAGS::CORJIT_FLAG_PROF_NO_PINVOKE_INLINE); -#endif // PROFILING_SUPPORTED - - // Don't allow allocations on FOH from collectible contexts to avoid memory leaks - if (!ftn->GetLoaderAllocator()->CanUnload()) - { - flags.Set(CORJIT_FLAGS::CORJIT_FLAG_FROZEN_ALLOC_ALLOWED); - } - - // Set optimization flags - if (!flags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_MIN_OPT)) - { - unsigned optType = g_pConfig->GenOptimizeType(); - _ASSERTE(optType <= OPT_RANDOM); - - if (optType == OPT_RANDOM) - optType = methodInfo->ILCodeSize % OPT_RANDOM; - - if (g_pConfig->JitMinOpts()) - { - flags.Set(CORJIT_FLAGS::CORJIT_FLAG_MIN_OPT); - } - else - { - if (!flags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_TIER0)) - flags.Set(CORJIT_FLAGS::CORJIT_FLAG_BBOPT); - } - - if (optType == OPT_SIZE) - { - flags.Set(CORJIT_FLAGS::CORJIT_FLAG_SIZE_OPT); - } - else if (optType == OPT_SPEED) - { - flags.Set(CORJIT_FLAGS::CORJIT_FLAG_SPEED_OPT); - } - } - - if (IsDynamicScope(methodInfo->scope)) - { - // no debug info available for IL stubs - if (!g_pConfig->GetTrackDynamicMethodDebugInfo()) - flags.Clear(CORJIT_FLAGS::CORJIT_FLAG_DEBUG_INFO); - - DynamicResolver* pResolver = GetDynamicResolver(methodInfo->scope); - flags.Add(pResolver->GetJitFlags()); - } - -#ifdef ARM_SOFTFP - flags.Set(CORJIT_FLAGS::CORJIT_FLAG_SOFTFP_ABI); -#endif // ARM_SOFTFP - -#ifdef FEATURE_PGO - - if (CLRConfig::GetConfigValue(CLRConfig::INTERNAL_ReadPGOData) > 0) - { - flags.Set(CORJIT_FLAGS::CORJIT_FLAG_BBOPT); - } - else if (g_pConfig->TieredPGO() && flags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_TIER1)) - { - flags.Set(CORJIT_FLAGS::CORJIT_FLAG_BBOPT); - } - -#endif - - return flags; -} - // ******************************************************************** // Throw the right type of exception for the given JIT result @@ -13065,15 +13100,15 @@ void ThrowExceptionForJit(HRESULT res) static TADDR UnsafeJitFunctionWorker( EECodeGenManager *pJitMgr, CEECodeGenInfo *pJitInfo, - CORJIT_FLAGS jitFlags, - CORINFO_METHOD_INFO methodInfo, NativeCodeVersion nativeCodeVersion, - ULONG* pSizeOfCode) + _Out_ ULONG* pSizeOfCode, + _Out_ ULONG* sizeOfILCode) { STANDARD_VM_CONTRACT; - MethodDesc * pMethodForSecurity = pJitInfo->GetMethodForSecurity(methodInfo.ftn); - MethodDesc * ftn = nativeCodeVersion.GetMethodDesc(); + CORINFO_METHOD_INFO* methodInfo = pJitInfo->getMethodInfoInternal(); + MethodDesc* pMethodForSecurity = pJitInfo->GetMethodForSecurity(methodInfo->ftn); + MethodDesc* ftn = nativeCodeVersion.GetMethodDesc(); //Since the check could trigger a demand, we have to do this every time. //This is actually an overly complicated way to make sure that a method can access all its arguments @@ -13134,8 +13169,7 @@ static TADDR UnsafeJitFunctionWorker( res = invokeCompileMethod(pJitMgr, pJitInfo, - &methodInfo, - jitFlags, + methodInfo, &nativeEntry, &sizeOfCode); @@ -13147,6 +13181,8 @@ static TADDR UnsafeJitFunctionWorker( } #endif + *sizeOfILCode = methodInfo->ILCodeSize; + #ifdef PERF_TRACK_METHOD_JITTIMES //store the time in the string buffer. Module name and token are unique enough. Also, do not //capture importing time, just actual compilation time. @@ -13356,8 +13392,8 @@ static void LogJitMethodEnd(MethodDesc* ftn) // are OK since they discard the return value of this method. PCODE UnsafeJitFunction(PrepareCodeConfig* config, _In_opt_ COR_ILMETHOD_DECODER* ILHeader, - _In_ CORJIT_FLAGS* pJitFlags, - _In_ ULONG* pSizeOfCode) + _Out_ CORJIT_FLAGS* pJitFlags, + _Out_ ULONG* pSizeOfCode) { STANDARD_VM_CONTRACT; _ASSERTE(config != NULL); @@ -13379,10 +13415,7 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, timer.Start(); - // Negate the check for no inlining so the logic is easier to follow - bool enableInlining = !config->GetJitCompilationFlags().IsSet(CORJIT_FLAGS::CORJIT_FLAG_NO_INLINING); - - CORINFO_METHOD_INFO methodInfo{}; + ULONG sizeOfILCode = 0; #ifdef FEATURE_INTERPRETER InterpreterJitManager* interpreterMgr = ExecutionManager::GetInterpreterJitManager(); @@ -13399,11 +13432,12 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, // If the interpreter was loaded, use it. if (interpreterMgr->IsInterpreterLoaded()) { - CInterpreterJitInfo interpreterJitInfo{ ftn, ILHeader, interpreterMgr, enableInlining }; - methodInfo = interpreterJitInfo.getMethodInfoInternal(); - *pJitFlags = GetCompileFlags(config, ftn, &methodInfo); + CInterpreterJitInfo interpreterJitInfo{ config, ftn, ILHeader, interpreterMgr }; + ret = UnsafeJitFunctionWorker(interpreterMgr, &interpreterJitInfo, nativeCodeVersion, pSizeOfCode, &sizeOfILCode); - ret = UnsafeJitFunctionWorker(interpreterMgr, &interpreterJitInfo, *pJitFlags, methodInfo, nativeCodeVersion, pSizeOfCode); + // Collect the JIT flags for the interpreter + if (ret) + *pJitFlags = *interpreterJitInfo.getJitFlagsInternal(); } #endif // FEATURE_INTERPRETER @@ -13432,16 +13466,14 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, JumpStubOverflowCheck jsoCheck{}; while (true) { - CEEJitInfo jitInfo{ ftn, ILHeader, jitMgr, enableInlining }; - methodInfo = jitInfo.getMethodInfoInternal(); - *pJitFlags = GetCompileFlags(config, ftn, &methodInfo); + CEEJitInfo jitInfo{ config, ftn, ILHeader, jitMgr }; // Enable the jump stub overflow check jsoCheck.Enable(jitInfo); #ifdef FEATURE_ON_STACK_REPLACEMENT // If this is an OSR jit request, grab the OSR info so we can pass it to the jit - if (pJitFlags->IsSet(CORJIT_FLAGS::CORJIT_FLAG_OSR)) + if (jitInfo.getJitFlagsInternal()->IsSet(CORJIT_FLAGS::CORJIT_FLAG_OSR)) { unsigned ilOffset = 0; PatchpointInfo* patchpointInfo = nativeCodeVersion.GetOSRInfo(&ilOffset); @@ -13449,7 +13481,7 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, } #endif // FEATURE_ON_STACK_REPLACEMENT - TADDR retMaybe = UnsafeJitFunctionWorker(jitMgr, &jitInfo, *pJitFlags, methodInfo, nativeCodeVersion, pSizeOfCode); + TADDR retMaybe = UnsafeJitFunctionWorker(jitMgr, &jitInfo, nativeCodeVersion, pSizeOfCode, &sizeOfILCode); if (!retMaybe) COMPlusThrow(kInvalidProgramException); @@ -13459,6 +13491,7 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, ret = PINSTRToPCODE(retMaybe); jitInfo.PublishFinalCodeAddress(ret); + *pJitFlags = *jitInfo.getJitFlagsInternal(); // We are done break; @@ -13481,8 +13514,8 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, InterlockedExchangeAdd64((LONG64*)&g_c100nsTicksInJit, c100nsTicksInJit); t_c100nsTicksInJitForThread += c100nsTicksInJit; - InterlockedExchangeAdd64((LONG64*)&g_cbILJitted, methodInfo.ILCodeSize); - t_cbILJittedForThread += methodInfo.ILCodeSize; + InterlockedExchangeAdd64((LONG64*)&g_cbILJitted, sizeOfILCode); + t_cbILJittedForThread += sizeOfILCode; InterlockedIncrement64((LONG64*)&g_cMethodsJitted); t_cMethodsJittedForThread++; diff --git a/src/coreclr/vm/jitinterface.h b/src/coreclr/vm/jitinterface.h index 5b9d7bef58d9fa..c78ca6831ab726 100644 --- a/src/coreclr/vm/jitinterface.h +++ b/src/coreclr/vm/jitinterface.h @@ -356,7 +356,7 @@ class CEEInfo : public ICorJitInfo TypeHandle typeHnd = TypeHandle() /* optional in */, CORINFO_CLASS_HANDLE *clsRet = NULL /* optional out */ ); - CEEInfo(MethodDesc * fd = NULL, bool fAllowInlining = true) : + CEEInfo(MethodDesc * fd = NULL) : m_pJitHandles(nullptr), m_pMethodBeingCompiled(fd), m_transientDetails(NULL), @@ -366,7 +366,7 @@ class CEEInfo : public ICorJitInfo #if defined(FEATURE_GDBJIT) m_pCalledMethods(NULL), #endif - m_allowInlining(fAllowInlining) + m_allowInlining(true) { LIMITED_METHOD_CONTRACT; } @@ -467,36 +467,7 @@ class CEECodeGenInfo : public CEEInfo { public: // ICorJitInfo stuff - CEECodeGenInfo(MethodDesc* fd, COR_ILMETHOD_DECODER* header, EECodeGenManager* jm, bool allowInlining) - : CEEInfo(fd, allowInlining), - m_jitManager(jm), - m_CodeHeader(NULL), - m_CodeHeaderRW(NULL), - m_codeWriteBufferSize(0), - m_pRealCodeHeader(NULL), - m_pCodeHeap(NULL), - m_ILHeader(header), - m_MethodInfo(), - m_GCinfo_len(0), - m_EHinfo_len(0), - m_iOffsetMapping(0), - m_pOffsetMapping(NULL), - m_iNativeVarInfo(0), - m_pNativeVarInfo(NULL), - m_inlineTreeNodes(NULL), - m_numInlineTreeNodes(0), - m_richOffsetMappings(NULL), - m_numRichOffsetMappings(0), - m_gphCache() - { - STANDARD_VM_CONTRACT; - COR_ILMETHOD_DECODER* ilHeader = getMethodInfoWorker(m_pMethodBeingCompiled, m_ILHeader, &m_MethodInfo); - - // The header maybe replaced during the call to getMethodInfoWorker. This is most probable - // when the input is null (that is, no metadata), but we can also examine the method and generate - // an entirely new IL body (for example, intrinsic methods). - m_ILHeader = ilHeader; - } + CEECodeGenInfo(PrepareCodeConfig* config, MethodDesc* fd, COR_ILMETHOD_DECODER* header, EECodeGenManager* jm); ~CEECodeGenInfo() { @@ -595,7 +566,8 @@ class CEECodeGenInfo : public CEEInfo CORINFO_CLASS_HANDLE getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* pIsSpeculative) override; void* getMethodSync(CORINFO_METHOD_HANDLE ftnHnd, void **ppIndirection) override; - CORINFO_METHOD_INFO getMethodInfoInternal() const; + CORINFO_METHOD_INFO* getMethodInfoInternal(); + CORJIT_FLAGS* getJitFlagsInternal(); protected: @@ -834,9 +806,9 @@ class CEEJitInfo final : public CEECodeGenInfo void PublishFinalCodeAddress(PCODE addr); - CEEJitInfo(MethodDesc* fd, COR_ILMETHOD_DECODER* header, - EECodeGenManager* jm, bool allowInlining) - : CEECodeGenInfo(fd, header, jm, allowInlining) + CEEJitInfo(PrepareCodeConfig* config, MethodDesc* fd, COR_ILMETHOD_DECODER* header, + EECodeGenManager* jm) + : CEECodeGenInfo(config, fd, header, jm) #ifdef FEATURE_EH_FUNCLETS , m_moduleBase(0), m_totalUnwindSize(0), @@ -964,9 +936,9 @@ class CInterpreterJitInfo final : public CEECodeGenInfo public: // ICorJitInfo stuff - CInterpreterJitInfo(MethodDesc* fd, COR_ILMETHOD_DECODER* header, - EECodeGenManager* jm, bool allowInlining) - : CEECodeGenInfo(fd, header, jm, allowInlining) + CInterpreterJitInfo(PrepareCodeConfig* config, MethodDesc* fd, COR_ILMETHOD_DECODER* header, + EECodeGenManager* jm) + : CEECodeGenInfo(config, fd, header, jm) { CONTRACTL { From 7aea910f3ea8cc659e040b54b3fd441ad62fc0b2 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 21 May 2025 16:14:39 -0700 Subject: [PATCH 08/11] Feedback --- src/coreclr/vm/jitinterface.cpp | 124 ++++++++++---------------------- src/coreclr/vm/jitinterface.h | 25 +++---- src/coreclr/vm/prestub.cpp | 8 ++- 3 files changed, 52 insertions(+), 105 deletions(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index ed881b2a5eeb94..c85a61cf41bfc4 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -7919,7 +7919,7 @@ CorInfoInline CEEInfo::canInline (CORINFO_METHOD_HANDLE hCaller, // Currently the rejit path is the only path which sets this. // If we get more reasons to set this then we may need to change // the failure reason message or disambiguate them. - if (!m_allowInlining) + if (m_jitFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_NO_INLINING)) { result = INLINE_FAIL; szFailReason = "ReJIT request disabled inlining from caller"; @@ -10519,14 +10519,6 @@ CORINFO_METHOD_HANDLE CEEInfo::embedMethodHandle(CORINFO_METHOD_HANDLE handle, return handle; } -/*********************************************************************/ -void CEEInfo::setJitFlags(const CORJIT_FLAGS& jitFlags) -{ - LIMITED_METHOD_CONTRACT; - - m_jitFlags = jitFlags; -} - /*********************************************************************/ uint32_t CEEInfo::getJitFlags(CORJIT_FLAGS* jitFlags, uint32_t sizeInBytes) { @@ -10849,9 +10841,6 @@ CEECodeGenInfo::CEECodeGenInfo(PrepareCodeConfig* config, MethodDesc* fd, COR_IL m_ILHeader = ilHeader; m_jitFlags = GetCompileFlags(config, m_pMethodBeingCompiled, &m_MethodInfo); - - // Negate the check for no inlining so the logic is easier to follow - m_allowInlining = !m_jitFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_NO_INLINING); } void* CEECodeGenInfo::getHelperFtn(CorInfoHelpFunc ftnNum, /* IN */ @@ -12434,13 +12423,13 @@ void* CEECodeGenInfo::getMethodSync(CORINFO_METHOD_HANDLE ftnHnd, CORINFO_METHOD_INFO* CEECodeGenInfo::getMethodInfoInternal() { - STANDARD_VM_CONTRACT; + LIMITED_METHOD_CONTRACT; return &m_MethodInfo; } CORJIT_FLAGS* CEECodeGenInfo::getJitFlagsInternal() { - STANDARD_VM_CONTRACT; + LIMITED_METHOD_CONTRACT; return &m_jitFlags; } @@ -12866,29 +12855,26 @@ void CEECodeGenInfo::getEHinfo( EE_TO_JIT_TRANSITION(); } -static CorJitResult invokeCompileMethodHelper(EECodeGenManager *jitMgr, +static CorJitResult invokeCompileMethod(EECodeGenManager *jitMgr, CEECodeGenInfo *comp, - struct CORINFO_METHOD_INFO *info, BYTE **nativeEntry, uint32_t *nativeSizeOfCode) { - STATIC_CONTRACT_THROWS; - STATIC_CONTRACT_GC_TRIGGERS; - STATIC_CONTRACT_MODE_PREEMPTIVE; + STANDARD_VM_CONTRACT; + ICorJitCompiler* jitCompiler; CorJitResult ret = CORJIT_SKIPPED; // Note that CORJIT_SKIPPED is an error exit status code + CORINFO_METHOD_INFO* methodInfo = comp->getMethodInfoInternal(); #if defined(ALLOW_SXS_JIT) - ICorJitCompiler *jitCompiler = jitMgr->GetAltCompiler(); + jitCompiler = jitMgr->GetAltCompiler(); if (jitCompiler != NULL) { - CORJIT_FLAGS jitFlags = *comp->getJitFlagsInternal(); - CORJIT_FLAGS altJitFlags = jitFlags; - altJitFlags.Set(CORJIT_FLAGS::CORJIT_FLAG_ALT_JIT); - comp->setJitFlags(altJitFlags); + CORJIT_FLAGS* jitFlags = comp->getJitFlagsInternal(); + jitFlags->Set(CORJIT_FLAGS::CORJIT_FLAG_ALT_JIT); ret = jitCompiler->compileMethod(comp, - info, + methodInfo, CORJIT_FLAGS::CORJIT_FLAG_CALL_GETJITFLAGS, nativeEntry, nativeSizeOfCode); @@ -12901,7 +12887,7 @@ static CorJitResult invokeCompileMethodHelper(EECodeGenManager *jitMgr, } // Restore the original JIT flags - comp->setJitFlags(jitFlags); + jitFlags->Clear(CORJIT_FLAGS::CORJIT_FLAG_ALT_JIT); } #endif // defined(ALLOW_SXS_JIT) @@ -12910,7 +12896,7 @@ static CorJitResult invokeCompileMethodHelper(EECodeGenManager *jitMgr, { jitCompiler = jitMgr->GetCompiler(); ret = jitCompiler->compileMethod(comp, - info, + methodInfo, CORJIT_FLAGS::CORJIT_FLAG_CALL_GETJITFLAGS, nativeEntry, nativeSizeOfCode); @@ -12930,42 +12916,12 @@ static CorJitResult invokeCompileMethodHelper(EECodeGenManager *jitMgr, if (SUCCEEDED(ret) && !comp->JitAgain()) { comp->CompressDebugInfo((PCODE)*nativeEntry); - comp->MethodCompileComplete(info->ftn); + comp->MethodCompileComplete(methodInfo->ftn); } return ret; } -/*********************************************************************/ -static CorJitResult invokeCompileMethod(EECodeGenManager *jitMgr, - CEECodeGenInfo *comp, - struct CORINFO_METHOD_INFO *info, - BYTE **nativeEntry, - uint32_t *nativeSizeOfCode) -{ - CONTRACTL { - THROWS; - GC_TRIGGERS; - MODE_COOPERATIVE; - } CONTRACTL_END; - // - // The JIT runs in preemptive mode - // - - GCX_PREEMP(); - - CorJitResult ret = invokeCompileMethodHelper(jitMgr, comp, info, nativeEntry, nativeSizeOfCode); - - // - // Verify that we are still in preemptive mode when we return - // from the JIT - // - - _ASSERTE(GetThread()->PreemptiveGCDisabled() == FALSE); - - return ret; -} - /*********************************************************************/ // Figures out the compile flags that are used by both JIT and NGen @@ -13101,8 +13057,7 @@ static TADDR UnsafeJitFunctionWorker( EECodeGenManager *pJitMgr, CEECodeGenInfo *pJitInfo, NativeCodeVersion nativeCodeVersion, - _Out_ ULONG* pSizeOfCode, - _Out_ ULONG* sizeOfILCode) + _Out_ ULONG* pSizeOfCode) { STANDARD_VM_CONTRACT; @@ -13154,35 +13109,32 @@ static TADDR UnsafeJitFunctionWorker( uint32_t sizeOfCode = 0; { - GCX_COOP(); - - /* There is a double indirection to call compileMethod - can we - improve this with the new structure? */ + // + // The JIT runs in preemptive mode + // + GCX_PREEMP(); #ifdef PERF_TRACK_METHOD_JITTIMES //Because we're not calling QPC enough. I'm not going to track times if we're just importing. LARGE_INTEGER methodJitTimeStart = {0}; - QueryPerformanceCounter (&methodJitTimeStart); + QueryPerformanceCounter(&methodJitTimeStart); #endif LOG((LF_CORDB, LL_EVERYTHING, "Calling invokeCompileMethod...\n")); - res = invokeCompileMethod(pJitMgr, pJitInfo, - methodInfo, &nativeEntry, &sizeOfCode); + // Verify that we are still in preemptive mode when we return + // from the JIT + _ASSERTE(GetThread()->PreemptiveGCDisabled() == FALSE); + #if FEATURE_PERFMAP // Save the code size so that it can be reported to the perfmap. - if (pSizeOfCode != NULL) - { - *pSizeOfCode = sizeOfCode; - } + *pSizeOfCode = sizeOfCode; #endif - *sizeOfILCode = methodInfo->ILCodeSize; - #ifdef PERF_TRACK_METHOD_JITTIMES //store the time in the string buffer. Module name and token are unique enough. Also, do not //capture importing time, just actual compilation time. @@ -13203,7 +13155,6 @@ static TADDR UnsafeJitFunctionWorker( OutputDebugStringUtf8(codeBase.GetUTF8()); } #endif // PERF_TRACK_METHOD_JITTIMES - } if (res == CORJIT_SKIPPED) @@ -13384,20 +13335,14 @@ static void LogJitMethodEnd(MethodDesc* ftn) // The reason that this is named UnsafeJitFunction is that this helper // method is not thread safe! When multiple threads get in here for // the same pMD, ALL of them MUST return the SAME value. -// To ensure that this happens you must call MakeJitWorker. -// It creates a DeadlockAware list of methods being jitted and prevents us -// from trying to jit the same method more that once. -// -// Calls to this method that occur to check if inlining can occur on x86, -// are OK since they discard the return value of this method. PCODE UnsafeJitFunction(PrepareCodeConfig* config, _In_opt_ COR_ILMETHOD_DECODER* ILHeader, - _Out_ CORJIT_FLAGS* pJitFlags, + _Out_ bool* isTier0, _Out_ ULONG* pSizeOfCode) { STANDARD_VM_CONTRACT; _ASSERTE(config != NULL); - _ASSERTE(pJitFlags != NULL); + _ASSERTE(isTier0 != NULL); NativeCodeVersion nativeCodeVersion = config->GetCodeVersion(); MethodDesc* ftn = nativeCodeVersion.GetMethodDesc(); @@ -13433,11 +13378,14 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, if (interpreterMgr->IsInterpreterLoaded()) { CInterpreterJitInfo interpreterJitInfo{ config, ftn, ILHeader, interpreterMgr }; - ret = UnsafeJitFunctionWorker(interpreterMgr, &interpreterJitInfo, nativeCodeVersion, pSizeOfCode, &sizeOfILCode); + ret = UnsafeJitFunctionWorker(interpreterMgr, &interpreterJitInfo, nativeCodeVersion, pSizeOfCode); - // Collect the JIT flags for the interpreter + // If successful, record data. if (ret) - *pJitFlags = *interpreterJitInfo.getJitFlagsInternal(); + { + sizeOfILCode = interpreterJitInfo.getMethodInfoInternal()->ILCodeSize; + *isTier0 = interpreterJitInfo.getJitFlagsInternal()->IsSet(CORJIT_FLAGS::CORJIT_FLAG_TIER0); + } } #endif // FEATURE_INTERPRETER @@ -13481,7 +13429,7 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, } #endif // FEATURE_ON_STACK_REPLACEMENT - TADDR retMaybe = UnsafeJitFunctionWorker(jitMgr, &jitInfo, nativeCodeVersion, pSizeOfCode, &sizeOfILCode); + TADDR retMaybe = UnsafeJitFunctionWorker(jitMgr, &jitInfo, nativeCodeVersion, pSizeOfCode); if (!retMaybe) COMPlusThrow(kInvalidProgramException); @@ -13491,7 +13439,9 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, ret = PINSTRToPCODE(retMaybe); jitInfo.PublishFinalCodeAddress(ret); - *pJitFlags = *jitInfo.getJitFlagsInternal(); + + sizeOfILCode = jitInfo.getMethodInfoInternal()->ILCodeSize; + *isTier0 = jitInfo.getJitFlagsInternal()->IsSet(CORJIT_FLAGS::CORJIT_FLAG_TIER0); // We are done break; diff --git a/src/coreclr/vm/jitinterface.h b/src/coreclr/vm/jitinterface.h index c78ca6831ab726..0080db0a1c40e8 100644 --- a/src/coreclr/vm/jitinterface.h +++ b/src/coreclr/vm/jitinterface.h @@ -69,8 +69,8 @@ void InitJITHelpers1(); PCODE UnsafeJitFunction(PrepareCodeConfig* config, COR_ILMETHOD_DECODER* header, - CORJIT_FLAGS* pJitFlags, - ULONG* pSizeOfCode); + _Out_ bool* isTier0, + _Out_ ULONG* pSizeOfCode); void getMethodInfoILMethodHeaderHelper( COR_ILMETHOD_DECODER* header, @@ -356,17 +356,16 @@ class CEEInfo : public ICorJitInfo TypeHandle typeHnd = TypeHandle() /* optional in */, CORINFO_CLASS_HANDLE *clsRet = NULL /* optional out */ ); - CEEInfo(MethodDesc * fd = NULL) : - m_pJitHandles(nullptr), - m_pMethodBeingCompiled(fd), - m_transientDetails(NULL), - m_pThread(GetThreadNULLOk()), - m_hMethodForSecurity_Key(NULL), - m_pMethodForSecurity_Value(NULL), + CEEInfo(MethodDesc * fd = NULL) + : m_pJitHandles(nullptr) + , m_pMethodBeingCompiled(fd) + , m_transientDetails(NULL) + , m_pThread(GetThreadNULLOk()) + , m_hMethodForSecurity_Key(NULL) + , m_pMethodForSecurity_Value(NULL) #if defined(FEATURE_GDBJIT) - m_pCalledMethods(NULL), + , m_pCalledMethods(NULL) #endif - m_allowInlining(true) { LIMITED_METHOD_CONTRACT; } @@ -395,8 +394,6 @@ class CEEInfo : public ICorJitInfo // Performs any work JIT-related work that should be performed at process shutdown. void JitProcessShutdownWork(); - void setJitFlags(const CORJIT_FLAGS& jitFlags); - public: MethodDesc * GetMethodForSecurity(CORINFO_METHOD_HANDLE callerHandle); @@ -449,8 +446,6 @@ class CEEInfo : public ICorJitInfo CalledMethod * m_pCalledMethods; #endif - bool m_allowInlining; - void EnsureActive(TypeHandle th, MethodDesc * pMD = NULL); }; diff --git a/src/coreclr/vm/prestub.cpp b/src/coreclr/vm/prestub.cpp index 6e728f947632d9..f4a7f2b1a7acf8 100644 --- a/src/coreclr/vm/prestub.cpp +++ b/src/coreclr/vm/prestub.cpp @@ -911,16 +911,18 @@ PCODE MethodDesc::JitCompileCodeLockedEventWrapper(PrepareCodeConfig* pConfig, J PCODE MethodDesc::JitCompileCodeLocked(PrepareCodeConfig* pConfig, COR_ILMETHOD_DECODER* pilHeader, JitListLockEntry* pEntry, ULONG* pSizeOfCode) { STANDARD_VM_CONTRACT; + _ASSERTE(pConfig != NULL); + _ASSERTE(pEntry != NULL); PCODE pCode = (PCODE)NULL; - CORJIT_FLAGS jitFlags; + bool isTier0 = false; PCODE pOtherCode = (PCODE)NULL; EX_TRY { Thread::CurrentPrepareCodeConfigHolder threadPrepareCodeConfigHolder(GetThread(), pConfig); - pCode = UnsafeJitFunction(pConfig, pilHeader, &jitFlags, pSizeOfCode); + pCode = UnsafeJitFunction(pConfig, pilHeader, &isTier0, pSizeOfCode); } EX_CATCH { @@ -979,7 +981,7 @@ PCODE MethodDesc::JitCompileCodeLocked(PrepareCodeConfig* pConfig, COR_ILMETHOD_ #ifdef FEATURE_TIERED_COMPILATION // Finalize the optimization tier before SetNativeCode() is called - bool shouldCountCalls = jitFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_TIER0) && pConfig->FinalizeOptimizationTierForTier0LoadOrJit(); + bool shouldCountCalls = isTier0 && pConfig->FinalizeOptimizationTierForTier0LoadOrJit(); #endif // Aside from rejit, performing a SetNativeCodeInterlocked at this point From b066960d3421261e4e509bbf2515e24c0c5e65c8 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Wed, 21 May 2025 16:23:27 -0700 Subject: [PATCH 09/11] Update src/coreclr/vm/jitinterface.cpp --- src/coreclr/vm/jitinterface.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index c85a61cf41bfc4..3496e237e2cac4 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -13109,11 +13109,6 @@ static TADDR UnsafeJitFunctionWorker( uint32_t sizeOfCode = 0; { - // - // The JIT runs in preemptive mode - // - GCX_PREEMP(); - #ifdef PERF_TRACK_METHOD_JITTIMES //Because we're not calling QPC enough. I'm not going to track times if we're just importing. LARGE_INTEGER methodJitTimeStart = {0}; From a37ebb10fd403d383a69424924ca753d97dc5d6c Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 21 May 2025 16:41:22 -0700 Subject: [PATCH 10/11] Nits --- src/coreclr/vm/jitinterface.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 3496e237e2cac4..5d1edb07a7a578 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -12919,6 +12919,9 @@ static CorJitResult invokeCompileMethod(EECodeGenManager *jitMgr, comp->MethodCompileComplete(methodInfo->ftn); } + // Verify that we are still in preemptive mode when we return + // from the JIT + _ASSERTE(GetThread()->PreemptiveGCDisabled() == FALSE); return ret; } @@ -13104,7 +13107,7 @@ static TADDR UnsafeJitFunctionWorker( } } - CorJitResult res = CORJIT_SKIPPED; + CorJitResult res; PBYTE nativeEntry = NULL; uint32_t sizeOfCode = 0; @@ -13113,22 +13116,18 @@ static TADDR UnsafeJitFunctionWorker( //Because we're not calling QPC enough. I'm not going to track times if we're just importing. LARGE_INTEGER methodJitTimeStart = {0}; QueryPerformanceCounter(&methodJitTimeStart); +#endif // PERF_TRACK_METHOD_JITTIMES -#endif LOG((LF_CORDB, LL_EVERYTHING, "Calling invokeCompileMethod...\n")); res = invokeCompileMethod(pJitMgr, pJitInfo, &nativeEntry, &sizeOfCode); - // Verify that we are still in preemptive mode when we return - // from the JIT - _ASSERTE(GetThread()->PreemptiveGCDisabled() == FALSE); - #if FEATURE_PERFMAP // Save the code size so that it can be reported to the perfmap. *pSizeOfCode = sizeOfCode; -#endif +#endif // FEATURE_PERFMAP #ifdef PERF_TRACK_METHOD_JITTIMES //store the time in the string buffer. Module name and token are unique enough. Also, do not From 1b30aca5f7f54f213c75d19e4a31689dde00087c Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 21 May 2025 16:52:56 -0700 Subject: [PATCH 11/11] Feedback --- src/coreclr/vm/jitinterface.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 5d1edb07a7a578..b36e5161b808ca 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -13125,6 +13125,7 @@ static TADDR UnsafeJitFunctionWorker( &sizeOfCode); #if FEATURE_PERFMAP + _ASSERTE(pSizeOfCode != NULL); // Save the code size so that it can be reported to the perfmap. *pSizeOfCode = sizeOfCode; #endif // FEATURE_PERFMAP @@ -13144,7 +13145,7 @@ static TADDR UnsafeJitFunctionWorker( moduleName.GetUTF8(), //module name ftn->GetMemberDef(), //method token (unsigned)(methodJitTimeStop.QuadPart - methodJitTimeStart.QuadPart), //cycle count - methodInfo.ILCodeSize //il size + methodInfo->ILCodeSize // IL size ); OutputDebugStringUtf8(codeBase.GetUTF8()); }