From e3ac237a613198e6c3ff71e349a27321e47340b8 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Fri, 11 Apr 2025 14:37:34 -0700 Subject: [PATCH 1/5] Micro optimizations to improve the performance of EH stackwalking, particularly in the X86 with Funclets model - Implement EHEnumInitFromStackFrameIterator as a SuppressGCTransition QCall and optimize its performance - This allows skipping setting hte InlinedCallFrame to indicate that it is an EH frame (as suppress GC transition frames are NOT generated in that situation) - On X86 this also allows skipping using an EH prolog for this function - Only Update the MethodRegionInfo if there are EH regions to walk - Improve the codegen and reduce the usage of UpdateRuntimeWrappedExceptions api - Previously we would call IsRuntimeWrappedExceptions, which would lazily compute the flag. However, since we can't actually run the lazy computation during EH, we had already forced it to be initialized, so we didn't actually need to have the full lazy computation logic in place. - Also, we were setting this flag as we walked the stack frame via SfiInit and SfiNext, but only checkinging it when parsing the EH clause data. Move the computation to the EHEnumInitFromStackFrameIterator api, and only compute the correct version of the flag IF there are clauses to walk. - Only Update the IsRuntimeWrappedExceptions flag and the MethodRegionInfo if there are EH regions to walk - Improve the performance of AppendExceptionStackFrame slightly be using the variant of GCX_COOP which takes a Thread* instead of getting it from the TLS data. - Improve the performance of StackTraceInfo::AppendElement - It always calls EnsureStackTraceArray which ALSO needs to have a protected GC variable. Instead of doing that locally in EnsureStackTraceArray, instead make the GCPROTECT in EnsureStackTraceArray be a bit larger. This allows avoiding modifying the TLS linked list of GCFrames, as well as avoids needing an x86 EH prolog for EnsureStackTraceArray - Update ExceptionObject::GetStackTrace to use an out of line copy of code to clone the stack trace array in the presence of the multi-threaded scenario. This avoids the EH prolog on X86. - Update ExceptionObject::GetStackTrace to avoid needing to regather the current thread, instead taking it as a parameter - Change ExceptionObject::GetStackTraceParts to use a faster technique for checking to see if the array is an sbyte array or an object[]. - Update NotifyFunctionEnter to check CORProfileTrackExceptions before calling the various profiler reporting functions. This allows the check to happen only once instead of 4 times, and also allowed me to outline some logic so that the function didn't need an EH prolog on X86 - For handling of the m_emptyDebuggerExState on the ThreadExceptionState object, move that into a global static variable to improve the performance of calls of the ThreadExceptionState::GetDebuggerState api, and add a new ThreadExceptionState::SetDebuggerIndicatedFramePointer api to avoid even touching the empty debugger state - Refactor the EECodeInfo::DecodeGCHdrInfo function into a fast inlineable path, and a slow path that does a lot of work. --- .../ExceptionServices/InternalCalls.cs | 1 + src/coreclr/vm/ceeload.cpp | 16 ++++++- src/coreclr/vm/ceeload.h | 1 + src/coreclr/vm/clrex.h | 7 ++- src/coreclr/vm/codeman.h | 13 ++++- src/coreclr/vm/eedbginterfaceimpl.inl | 4 +- src/coreclr/vm/excep.cpp | 48 +++++++++---------- src/coreclr/vm/exceptionhandling.cpp | 33 +++++++------ src/coreclr/vm/exstate.cpp | 27 +++++++---- src/coreclr/vm/exstate.h | 1 + src/coreclr/vm/jitinterface.cpp | 13 ++--- src/coreclr/vm/object.cpp | 17 +++++-- src/coreclr/vm/object.h | 10 +++- src/coreclr/vm/stackwalk.h | 2 +- 14 files changed, 125 insertions(+), 68 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ExceptionServices/InternalCalls.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ExceptionServices/InternalCalls.cs index f1d8e70c91f45f..608d52d768911e 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ExceptionServices/InternalCalls.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ExceptionServices/InternalCalls.cs @@ -39,6 +39,7 @@ internal static unsafe partial bool RhpCallFilterFunclet( internal static unsafe partial void RhpAppendExceptionStackFrame(ObjectHandleOnStack exceptionObj, IntPtr ip, UIntPtr sp, int flags, EH.ExInfo* exInfo); [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "EHEnumInitFromStackFrameIterator")] + [SuppressGCTransition] [return: MarshalAs(UnmanagedType.U1)] internal static unsafe partial bool RhpEHEnumInitFromStackFrameIterator(ref StackFrameIterator pFrameIter, out EH.MethodRegionInfo pMethodRegionInfo, void* pEHEnum); diff --git a/src/coreclr/vm/ceeload.cpp b/src/coreclr/vm/ceeload.cpp index 88ef2ffc5f1c8a..60a9373cb44cbe 100644 --- a/src/coreclr/vm/ceeload.cpp +++ b/src/coreclr/vm/ceeload.cpp @@ -1066,12 +1066,26 @@ BOOL Module::IsRuntimeWrapExceptionsStatusComputed() return (m_dwPersistedFlags & COMPUTED_WRAP_EXCEPTIONS); } +BOOL Module::IsRuntimeWrapExceptionsDuringEH() +{ + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + MODE_ANY; + } + CONTRACTL_END + + _ASSERTE(IsRuntimeWrapExceptionsStatusComputed()); + return (m_dwPersistedFlags & WRAP_EXCEPTIONS) != 0; +} + BOOL Module::IsRuntimeWrapExceptions() { CONTRACTL { NOTHROW; - if (IsRuntimeWrapExceptionsStatusComputed()) GC_NOTRIGGER; else GC_TRIGGERS; + GC_TRIGGERS; MODE_ANY; } CONTRACTL_END diff --git a/src/coreclr/vm/ceeload.h b/src/coreclr/vm/ceeload.h index 431b84085ed7ea..07a7c24fb46742 100644 --- a/src/coreclr/vm/ceeload.h +++ b/src/coreclr/vm/ceeload.h @@ -1554,6 +1554,7 @@ class Module : public ModuleBase // words, they become compliant //----------------------------------------------------------------------------------------- BOOL IsRuntimeWrapExceptions(); + BOOL IsRuntimeWrapExceptionsDuringEH(); //----------------------------------------------------------------------------------------- // If true, the built-in runtime-generated marshalling subsystem will be used for diff --git a/src/coreclr/vm/clrex.h b/src/coreclr/vm/clrex.h index bf6f2cc80b4a4a..8cd79a8ed954d2 100644 --- a/src/coreclr/vm/clrex.h +++ b/src/coreclr/vm/clrex.h @@ -56,8 +56,13 @@ struct StackTraceElement class StackTraceInfo { + struct StackTraceArrayProtect + { + StackTraceArray m_pStackTraceArray; + StackTraceArray m_pStackTraceArrayNew; + }; static OBJECTREF GetKeepAliveObject(MethodDesc* pMethod); - static void EnsureStackTraceArray(StackTraceArray *pStackTrace, size_t neededSize); + static void EnsureStackTraceArray(StackTraceArrayProtect *pStackTraceArrayProtected, size_t neededSize); static void EnsureKeepAliveArray(PTRARRAYREF *ppKeepAliveArray, size_t neededSize); public: static void AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, UINT_PTR currentSP, MethodDesc* pFunc, CrawlFrame* pCf); diff --git a/src/coreclr/vm/codeman.h b/src/coreclr/vm/codeman.h index f15202bc321eff..4081b849281d10 100644 --- a/src/coreclr/vm/codeman.h +++ b/src/coreclr/vm/codeman.h @@ -2906,7 +2906,18 @@ class EECodeInfo return GetCodeManager()->GetFrameSize(GetGCInfoToken()); } - PTR_CBYTE DecodeGCHdrInfo(hdrInfo ** infoPtr); + FORCEINLINE PTR_CBYTE DecodeGCHdrInfo(hdrInfo ** infoPtr) + { + if (m_hdrInfoTable == NULL) + { + return DecodeGCHdrInfoHelper(infoPtr); + } + + *infoPtr = &m_hdrInfoBody; + return m_hdrInfoTable; + } +private: + PTR_CBYTE DecodeGCHdrInfoHelper(hdrInfo ** infoPtr); #endif // TARGET_X86 #if defined(TARGET_WASM) diff --git a/src/coreclr/vm/eedbginterfaceimpl.inl b/src/coreclr/vm/eedbginterfaceimpl.inl index 3f1eb866c7aae9..bc16a336f253d3 100644 --- a/src/coreclr/vm/eedbginterfaceimpl.inl +++ b/src/coreclr/vm/eedbginterfaceimpl.inl @@ -29,7 +29,7 @@ class EEToDebuggerExceptionInterfaceWrapper CONTRACTL_END; ThreadExceptionState* pExState = pThread->GetExceptionState(); - pExState->GetDebuggerState()->SetDebuggerIndicatedFramePointer((LPVOID)currentSP); + pExState->SetDebuggerIndicatedFramePointer((LPVOID)currentSP); if (CORDebuggerAttached()) { @@ -56,7 +56,7 @@ class EEToDebuggerExceptionInterfaceWrapper ThreadExceptionState* pExState = pThread->GetExceptionState(); - pExState->GetDebuggerState()->SetDebuggerIndicatedFramePointer((LPVOID)currentSP); + pExState->SetDebuggerIndicatedFramePointer((LPVOID)currentSP); if (CORDebuggerAttached()) { diff --git a/src/coreclr/vm/excep.cpp b/src/coreclr/vm/excep.cpp index a33ffcf96164af..0d5d0e377ed51a 100644 --- a/src/coreclr/vm/excep.cpp +++ b/src/coreclr/vm/excep.cpp @@ -494,7 +494,7 @@ OBJECTREF PossiblyUnwrapThrowable(OBJECTREF throwable, Assembly *pAssembly) } CONTRACTL_END; - if (fIsRuntimeWrappedException && (!pAssembly->GetModule()->IsRuntimeWrapExceptions())) + if (fIsRuntimeWrappedException && (!pAssembly->GetModule()->IsRuntimeWrapExceptionsDuringEH())) { // We already created the instance, fetched the field. We know it is // not marshal by ref, or any of the other cases that might trigger GC. @@ -2876,20 +2876,17 @@ void SetupWatsonBucket(UINT_PTR currentIP, CrawlFrame* pCf) #endif // !TARGET_UNIX // Ensure that there is space for neededSize elements in the stack trace array. -void StackTraceInfo::EnsureStackTraceArray(StackTraceArray *pStackTrace, size_t neededSize) +void StackTraceInfo::EnsureStackTraceArray(StackTraceArrayProtect *pStackTraceProtected, size_t neededSize) { CONTRACTL { GC_TRIGGERS; THROWS; - PRECONDITION(CheckPointer(pStackTrace)); + PRECONDITION(CheckPointer(pStackTraceProtected)); } CONTRACTL_END; - StackTraceArray newStackTrace; - GCPROTECT_BEGIN(newStackTrace); - - size_t stackTraceCapacity = pStackTrace->Capacity(); + size_t stackTraceCapacity = pStackTraceProtected->m_pStackTraceArray.Capacity(); if (neededSize > stackTraceCapacity) { S_SIZE_T newCapacity = S_SIZE_T(stackTraceCapacity) * S_SIZE_T(2); @@ -2905,17 +2902,16 @@ void StackTraceInfo::EnsureStackTraceArray(StackTraceArray *pStackTrace, size_t stackTraceCapacity = newCapacity.Value(); // Allocate a new array with the needed size - newStackTrace.Allocate(stackTraceCapacity); - if (pStackTrace->Get() != NULL) + pStackTraceProtected->m_pStackTraceArrayNew.Allocate(stackTraceCapacity); + if (pStackTraceProtected->m_pStackTraceArray.Get() != NULL) { // Copy the original array to the new one - newStackTrace.CopyDataFrom(*pStackTrace); - _ASSERTE(newStackTrace.Size() == (neededSize - 1)); + pStackTraceProtected->m_pStackTraceArrayNew.CopyDataFrom(pStackTraceProtected->m_pStackTraceArray); + _ASSERTE(pStackTraceProtected->m_pStackTraceArrayNew.Size() == (neededSize - 1)); } // Update the stack trace array - pStackTrace->Set(newStackTrace.Get()); + pStackTraceProtected->m_pStackTraceArray.Set(pStackTraceProtected->m_pStackTraceArrayNew.Get()); } - GCPROTECT_END(); } // Ensure that there is space for the neededSize elements in the keepAlive array. @@ -3073,7 +3069,7 @@ void StackTraceInfo::AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, { struct { - StackTraceArray stackTrace; + StackTraceArrayProtect stackTrace; PTRARRAYREF pKeepAliveArray = NULL; // Object array of Managed Resolvers / Loader Allocators of methods that can be collected OBJECTREF keepAliveObject = NULL; } gc; @@ -3082,30 +3078,30 @@ void StackTraceInfo::AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, // Fetch the stacktrace and the keepAlive array from the exception object. It returns clones of those arrays in case the // stack trace was created by a different thread. - ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->GetStackTrace(gc.stackTrace, &gc.pKeepAliveArray); + ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->GetStackTrace(gc.stackTrace.m_pStackTraceArray, &gc.pKeepAliveArray, pThread); // The stack trace returned by the GetStackTrace has to be created by the current thread or be NULL. - _ASSERTE((gc.stackTrace.Get() == NULL) || (gc.stackTrace.GetObjectThread() == pThread)); + _ASSERTE((gc.stackTrace.m_pStackTraceArray.Get() == NULL) || (gc.stackTrace.m_pStackTraceArray.GetObjectThread() == pThread)); - EnsureStackTraceArray(&gc.stackTrace, gc.stackTrace.Size() + 1); + EnsureStackTraceArray(&gc.stackTrace, gc.stackTrace.m_pStackTraceArray.Size() + 1); if (fRaisingForeignException) { // Just before we append to the stack trace, mark the last recorded frame to be from // the foreign thread so that we can insert an annotation indicating so when building // the stack trace string. - size_t numCurrentFrames = gc.stackTrace.Size(); + size_t numCurrentFrames = gc.stackTrace.m_pStackTraceArray.Size(); if (numCurrentFrames > 0) { // "numCurrentFrames" can be zero if the user created an EDI using // an unthrown exception. - StackTraceElement & refLastElementFromForeignStackTrace = gc.stackTrace[numCurrentFrames - 1]; + StackTraceElement & refLastElementFromForeignStackTrace = gc.stackTrace.m_pStackTraceArray[numCurrentFrames - 1]; refLastElementFromForeignStackTrace.flags |= STEF_LAST_FRAME_FROM_FOREIGN_STACK_TRACE; } } - uint32_t keepAliveItemsCount = gc.stackTrace.GetKeepAliveItemsCount(); - _ASSERTE(keepAliveItemsCount == gc.stackTrace.ComputeKeepAliveItemsCount()); + uint32_t keepAliveItemsCount = gc.stackTrace.m_pStackTraceArray.GetKeepAliveItemsCount(); + _ASSERTE(keepAliveItemsCount == gc.stackTrace.m_pStackTraceArray.ComputeKeepAliveItemsCount()); gc.keepAliveObject = GetKeepAliveObject(pFunc); if (gc.keepAliveObject != NULL) @@ -3131,21 +3127,21 @@ void StackTraceInfo::AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, gc.pKeepAliveArray = NULL; } - gc.stackTrace.SetKeepAliveItemsCount(keepAliveItemsCount); + gc.stackTrace.m_pStackTraceArray.SetKeepAliveItemsCount(keepAliveItemsCount); - gc.stackTrace.Append(&stackTraceElem); - _ASSERTE(gc.stackTrace.ComputeKeepAliveItemsCount() == keepAliveItemsCount); + gc.stackTrace.m_pStackTraceArray.Append(&stackTraceElem); + _ASSERTE(gc.stackTrace.m_pStackTraceArray.ComputeKeepAliveItemsCount() == keepAliveItemsCount); if (gc.pKeepAliveArray != NULL) { _ASSERTE(keepAliveItemsCount > 0); - gc.pKeepAliveArray->SetAt(0, gc.stackTrace.Get()); + gc.pKeepAliveArray->SetAt(0, gc.stackTrace.m_pStackTraceArray.Get()); ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->SetStackTrace(dac_cast(gc.pKeepAliveArray)); } else { _ASSERTE(keepAliveItemsCount == 0); - ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->SetStackTrace(dac_cast(gc.stackTrace.Get())); + ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->SetStackTrace(dac_cast(gc.stackTrace.m_pStackTraceArray.Get())); } // Clear the _stackTraceString field as it no longer matches the stack trace diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index dfe0e4abe69751..22fbe6865e7d95 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -2973,7 +2973,7 @@ extern "C" void QCALLTYPE AppendExceptionStackFrame(QCall::ObjectHandleOnStack e Thread* pThread = GET_THREAD(); { - GCX_COOP(); + GCX_COOP_THREAD_EXISTS(pThread); Frame* pFrame = pThread->GetFrame(); MarkInlinedCallFrameAsEHHelperCall(pFrame); @@ -3454,25 +3454,24 @@ struct ExtendedEHClauseEnumerator : EH_CLAUSE_ENUMERATOR extern "C" CLR_BOOL QCALLTYPE EHEnumInitFromStackFrameIterator(StackFrameIterator *pFrameIter, IJitManager::MethodRegionInfo* pMethodRegionInfo, EH_CLAUSE_ENUMERATOR * pEHEnum) { - QCALL_CONTRACT; + QCALL_CONTRACT_NO_GC_TRANSITION; ExtendedEHClauseEnumerator *pExtendedEHEnum = (ExtendedEHClauseEnumerator*)pEHEnum; pExtendedEHEnum->pFrameIter = pFrameIter; - BEGIN_QCALL; - Thread* pThread = GET_THREAD(); - Frame* pFrame = pThread->GetFrame(); - MarkInlinedCallFrameAsEHHelperCall(pFrame); - IJitManager* pJitMan = pFrameIter->m_crawl.GetJitManager(); const METHODTOKEN& MethToken = pFrameIter->m_crawl.GetMethodToken(); - pJitMan->JitTokenToMethodRegionInfo(MethToken, pMethodRegionInfo); pExtendedEHEnum->EHCount = pJitMan->InitializeEHEnumeration(MethToken, pEHEnum); - EH_LOG((LL_INFO100, "Initialized EH enumeration, %d clauses found\n", pExtendedEHEnum->EHCount)); - END_QCALL; - return pExtendedEHEnum->EHCount != 0; + if (pExtendedEHEnum->EHCount == 0) + { + return FALSE; + } + + pJitMan->JitTokenToMethodRegionInfo(MethToken, pMethodRegionInfo); + pFrameIter->UpdateIsRuntimeWrappedExceptions(); + return TRUE; } extern "C" CLR_BOOL QCALLTYPE EHEnumNext(EH_CLAUSE_ENUMERATOR* pEHEnum, RhEHClause* pEHClause) @@ -3719,7 +3718,7 @@ static void NotifyExceptionPassStarted(StackFrameIterator *pThis, Thread *pThrea } } -static void NotifyFunctionEnter(StackFrameIterator *pThis, Thread *pThread, ExInfo *pExInfo) +NOINLINE static void NotifyFunctionEnterHelper(StackFrameIterator *pThis, Thread *pThread, ExInfo *pExInfo) { MethodDesc *pMD = pThis->m_crawl.GetFunction(); @@ -3743,6 +3742,14 @@ static void NotifyFunctionEnter(StackFrameIterator *pThis, Thread *pThread, ExIn pExInfo->m_pMDToReportFunctionLeave = pMD; } +static void NotifyFunctionEnter(StackFrameIterator *pThis, Thread *pThread, ExInfo *pExInfo) +{ + BEGIN_PROFILER_CALLBACK(CORProfilerTrackExceptions()); + // We don't need to do any notifications for the profiler if we are not tracking exceptions. + NotifyFunctionEnterHelper(pThis, pThread, pExInfo); + END_PROFILER_CALLBACK(); +} + extern "C" CLR_BOOL QCALLTYPE SfiInit(StackFrameIterator* pThis, CONTEXT* pStackwalkCtx, CLR_BOOL instructionFault, CLR_BOOL* pfIsExceptionIntercepted) { QCALL_CONTRACT; @@ -3858,7 +3865,6 @@ extern "C" CLR_BOOL QCALLTYPE SfiInit(StackFrameIterator* pThis, CONTEXT* pStack controlPC -= STACKWALK_CONTROLPC_ADJUST_OFFSET; } pThis->SetAdjustedControlPC(controlPC); - pThis->UpdateIsRuntimeWrappedExceptions(); *pfIsExceptionIntercepted = CheckExceptionInterception(pThis, pExInfo); EH_LOG((LL_INFO100, "SfiInit (pass %d): Exception stack walking starting at IP=%p, SP=%p, method %s::%s\n", @@ -4137,7 +4143,6 @@ Exit:; controlPC -= STACKWALK_CONTROLPC_ADJUST_OFFSET; } pThis->SetAdjustedControlPC(controlPC); - pThis->UpdateIsRuntimeWrappedExceptions(); *pfIsExceptionIntercepted = CheckExceptionInterception(pThis, pTopExInfo); diff --git a/src/coreclr/vm/exstate.cpp b/src/coreclr/vm/exstate.cpp index b60d4897ef6596..e70c6c9c6edf6f 100644 --- a/src/coreclr/vm/exstate.cpp +++ b/src/coreclr/vm/exstate.cpp @@ -291,6 +291,8 @@ ExceptionFlags* ThreadExceptionState::GetFlags() #if !defined(DACCESS_COMPILE) #ifdef DEBUGGING_SUPPORTED +static DebuggerExState m_emptyDebuggerExState; + DebuggerExState* ThreadExceptionState::GetDebuggerState() { #ifdef FEATURE_EH_FUNCLETS @@ -301,14 +303,6 @@ DebuggerExState* ThreadExceptionState::GetDebuggerState() else { _ASSERTE(!"unexpected use of GetDebuggerState() when no exception in flight"); -#if defined(_MSC_VER) - #pragma warning(disable : 4640) -#endif - static DebuggerExState m_emptyDebuggerExState; - -#if defined(_MSC_VER) - #pragma warning(default : 4640) -#endif return &m_emptyDebuggerExState; } #else // FEATURE_EH_FUNCLETS @@ -316,6 +310,23 @@ DebuggerExState* ThreadExceptionState::GetDebuggerState() #endif // FEATURE_EH_FUNCLETS } +void ThreadExceptionState::SetDebuggerIndicatedFramePointer(LPVOID indicatedFramePointer) +{ + WRAPPER_NO_CONTRACT; +#ifdef FEATURE_EH_FUNCLETS + if (m_pCurrentTracker) + { + m_pCurrentTracker->m_DebuggerExState.SetDebuggerIndicatedFramePointer(indicatedFramePointer); + } + else + { + _ASSERTE(!"unexpected use of SetDebuggerIndicatedFramePointer() when no exception in flight"); + } +#else // FEATURE_EH_FUNCLETS + m_currentExInfo.m_DebuggerExState.SetDebuggerIndicatedFramePointer(indicatedFramePointer); +#endif // FEATURE_EH_FUNCLETS +} + BOOL ThreadExceptionState::IsDebuggerInterceptable() { LIMITED_METHOD_CONTRACT; diff --git a/src/coreclr/vm/exstate.h b/src/coreclr/vm/exstate.h index 4c0b37681ff782..463283c0c4598a 100644 --- a/src/coreclr/vm/exstate.h +++ b/src/coreclr/vm/exstate.h @@ -93,6 +93,7 @@ class ThreadExceptionState #ifdef DEBUGGING_SUPPORTED // DebuggerExState stores information necessary for intercepting an exception DebuggerExState* GetDebuggerState(); + void SetDebuggerIndicatedFramePointer(LPVOID indicatedFramePointer); // check to see if the current exception is interceptable BOOL IsDebuggerInterceptable(); diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 484f27f8a4b9e4..46879c15c8d308 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -14924,15 +14924,12 @@ BOOL EECodeInfo::HasFrameRegister() #if defined(TARGET_X86) -PTR_CBYTE EECodeInfo::DecodeGCHdrInfo(hdrInfo ** infoPtr) +PTR_CBYTE EECodeInfo::DecodeGCHdrInfoHelper(hdrInfo ** infoPtr) { - if (m_hdrInfoTable == NULL) - { - GCInfoToken gcInfoToken = GetGCInfoToken(); - DWORD hdrInfoSize = (DWORD)::DecodeGCHdrInfo(gcInfoToken, m_relOffset, &m_hdrInfoBody); - _ASSERTE(hdrInfoSize != 0); - m_hdrInfoTable = (PTR_CBYTE)gcInfoToken.Info + hdrInfoSize; - } + GCInfoToken gcInfoToken = GetGCInfoToken(); + DWORD hdrInfoSize = (DWORD)::DecodeGCHdrInfo(gcInfoToken, m_relOffset, &m_hdrInfoBody); + _ASSERTE(hdrInfoSize != 0); + m_hdrInfoTable = (PTR_CBYTE)gcInfoToken.Info + hdrInfoSize; *infoPtr = &m_hdrInfoBody; return m_hdrInfoTable; diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index 302111f97d5210..40b2382dbd9d29 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1867,7 +1867,7 @@ void ExceptionObject::SetStackTrace(OBJECTREF stackTrace) // that both of these arrays are consistent. That means that the stack trace doesn't contain // frames that need keep alive objects and that are not protected by entries in the keep alive // array. -void ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outKeepAliveArray /*= NULL*/) const +void ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outKeepAliveArray, Thread *pCurrentThread) const { CONTRACTL { @@ -1882,9 +1882,16 @@ void ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * ExceptionObject::GetStackTraceParts(_stackTrace, stackTrace, outKeepAliveArray); #ifndef DACCESS_COMPILE - Thread *pThread = GetThread(); + if ((stackTrace.Get() != NULL) && (stackTrace.GetObjectThread() != pCurrentThread)) + { + GetStackTraceClone(stackTrace, outKeepAliveArray); + } +#endif // DACCESS_COMPILE +} - if ((stackTrace.Get() != NULL) && (stackTrace.GetObjectThread() != pThread)) +#ifndef DACCESS_COMPILE +void ExceptionObject::GetStackTraceClone(StackTraceArray & stackTrace, PTRARRAYREF * outKeepAliveArray) +{ { struct { @@ -1964,8 +1971,8 @@ void ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * } GCPROTECT_END(); } -#endif // DACCESS_COMPILE } +#endif // DACCESS_COMPILE // Get the stack trace and the dynamic method array from the stack trace object. // If the stack trace was created by another thread, it returns clones of both arrays. @@ -1985,7 +1992,7 @@ void ExceptionObject::GetStackTraceParts(OBJECTREF stackTraceObj, StackTraceArra PTRARRAYREF keepAliveArray = NULL; // Extract the stack trace and keepAlive arrays from the stack trace object. - if ((stackTraceObj != NULL) && ((dac_cast(OBJECTREFToObject(stackTraceObj)))->GetArrayElementType() != ELEMENT_TYPE_I1)) + if ((stackTraceObj != NULL) && ((dac_cast(OBJECTREFToObject(stackTraceObj)))->GetMethodTable()->ContainsGCPointers())) { // The stack trace object is the dynamic methods array with its first slot set to the stack trace I1Array. PTR_PTRArray combinedArray = dac_cast(OBJECTREFToObject(stackTraceObj)); diff --git a/src/coreclr/vm/object.h b/src/coreclr/vm/object.h index 1c4384e7771ef1..18b1a6ed6c0705 100644 --- a/src/coreclr/vm/object.h +++ b/src/coreclr/vm/object.h @@ -2268,7 +2268,15 @@ class ExceptionObject : public Object void SetStackTrace(OBJECTREF stackTrace); - void GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outKeepaliveArray = NULL) const; + void GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outKeepaliveArray = NULL) const + { + return GetStackTrace(stackTrace, outKeepaliveArray, GetThread()); + } + +private: + static void GetStackTraceClone(StackTraceArray & stackTrace, PTRARRAYREF * outKeepAliveArray); +public: + void GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outKeepaliveArray, Thread *pCurrentThread) const; static void GetStackTraceParts(OBJECTREF stackTraceObj, StackTraceArray & stackTrace, PTRARRAYREF * outKeepaliveArray); diff --git a/src/coreclr/vm/stackwalk.h b/src/coreclr/vm/stackwalk.h index 1ee3c19f5196e1..99c54be22b035e 100644 --- a/src/coreclr/vm/stackwalk.h +++ b/src/coreclr/vm/stackwalk.h @@ -619,7 +619,7 @@ class StackFrameIterator CONTRACTL_END #if defined(FEATURE_EH_FUNCLETS) && !defined(DACCESS_COMPILE) - m_isRuntimeWrappedExceptions = (m_crawl.pFunc != NULL) && m_crawl.pFunc->GetModule()->IsRuntimeWrapExceptions(); + m_isRuntimeWrappedExceptions = (m_crawl.pFunc != NULL) && m_crawl.pFunc->GetModule()->IsRuntimeWrapExceptionsDuringEH(); #endif // FEATURE_EH_FUNCLETS && !DACCESS_COMPILE } From 72e890dec446850f4518cd1921d452471b19ae7e Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Mon, 14 Apr 2025 13:15:21 -0700 Subject: [PATCH 2/5] Address feedback, and fix a few of the issues found in the tests --- src/coreclr/vm/ceeload.cpp | 3 +++ src/coreclr/vm/clrex.h | 6 ++++++ src/coreclr/vm/exceptionhandling.cpp | 2 ++ src/coreclr/vm/exstate.cpp | 4 ++-- src/coreclr/vm/jitinterface.cpp | 1 + src/coreclr/vm/stackwalk.h | 2 +- 6 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/coreclr/vm/ceeload.cpp b/src/coreclr/vm/ceeload.cpp index 60a9373cb44cbe..2c9182f4fc51b0 100644 --- a/src/coreclr/vm/ceeload.cpp +++ b/src/coreclr/vm/ceeload.cpp @@ -1076,6 +1076,9 @@ BOOL Module::IsRuntimeWrapExceptionsDuringEH() } CONTRACTL_END + // This method assumes that the runtime wrap exceptions status has already been computed. + // IsRuntimeWrapExceptionsStatusComputed() returns TRUE before calling this method, but + // that should be done as part of Module activation, so we shouldn't need to worry about that. _ASSERTE(IsRuntimeWrapExceptionsStatusComputed()); return (m_dwPersistedFlags & WRAP_EXCEPTIONS) != 0; } diff --git a/src/coreclr/vm/clrex.h b/src/coreclr/vm/clrex.h index 8cd79a8ed954d2..71fc4726010947 100644 --- a/src/coreclr/vm/clrex.h +++ b/src/coreclr/vm/clrex.h @@ -58,7 +58,12 @@ class StackTraceInfo { struct StackTraceArrayProtect { + // Stores the current stack trace array. This array may be accessed by multiple threads + // during exception handling, and needs to be protected from concurrent modifications. StackTraceArray m_pStackTraceArray; + + // Used as a temporary buffer when resizing the stack trace array. + // This allows atomic replacement of the original array with the newly sized array. StackTraceArray m_pStackTraceArrayNew; }; static OBJECTREF GetKeepAliveObject(MethodDesc* pMethod); @@ -1168,4 +1173,5 @@ class CLRLastThrownObjectException : public CLRException bool IsHRESULTForExceptionKind(HRESULT hr, RuntimeExceptionKind kind); #endif // _CLREX_H_ +``` diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index 22fbe6865e7d95..61154a3e4d539e 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -3677,6 +3677,7 @@ static void NotifyExceptionPassStarted(StackFrameIterator *pThis, Thread *pThrea } else { + BEGIN_PROFILER_CALLBACK(CORProfilerTrackExceptions()); _ASSERTE(pExInfo->m_pMDToReportFunctionLeave != NULL); EEToProfilerExceptionInterfaceWrapper::ExceptionSearchCatcherFound(pMD); if (pExInfo->m_pMDToReportFunctionLeave != NULL) @@ -3684,6 +3685,7 @@ static void NotifyExceptionPassStarted(StackFrameIterator *pThis, Thread *pThrea EEToProfilerExceptionInterfaceWrapper::ExceptionSearchFunctionLeave(pExInfo->m_pMDToReportFunctionLeave); pExInfo->m_pMDToReportFunctionLeave = NULL; } + END_PROFILER_CALLBACK(); // We don't need to do anything special for continuable exceptions after calling // this callback. We are going to start unwinding anyway. diff --git a/src/coreclr/vm/exstate.cpp b/src/coreclr/vm/exstate.cpp index e70c6c9c6edf6f..0326e274f6d31b 100644 --- a/src/coreclr/vm/exstate.cpp +++ b/src/coreclr/vm/exstate.cpp @@ -291,7 +291,7 @@ ExceptionFlags* ThreadExceptionState::GetFlags() #if !defined(DACCESS_COMPILE) #ifdef DEBUGGING_SUPPORTED -static DebuggerExState m_emptyDebuggerExState; +static DebuggerExState s_emptyDebuggerExState; DebuggerExState* ThreadExceptionState::GetDebuggerState() { @@ -303,7 +303,7 @@ DebuggerExState* ThreadExceptionState::GetDebuggerState() else { _ASSERTE(!"unexpected use of GetDebuggerState() when no exception in flight"); - return &m_emptyDebuggerExState; + return &s_emptyDebuggerExState; } #else // FEATURE_EH_FUNCLETS return &(m_currentExInfo.m_DebuggerExState); diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 46879c15c8d308..53ea7b045ff52a 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -14927,6 +14927,7 @@ BOOL EECodeInfo::HasFrameRegister() PTR_CBYTE EECodeInfo::DecodeGCHdrInfoHelper(hdrInfo ** infoPtr) { GCInfoToken gcInfoToken = GetGCInfoToken(); + _ASSERTE(m_hdrInfoTable == NULL); DWORD hdrInfoSize = (DWORD)::DecodeGCHdrInfo(gcInfoToken, m_relOffset, &m_hdrInfoBody); _ASSERTE(hdrInfoSize != 0); m_hdrInfoTable = (PTR_CBYTE)gcInfoToken.Info + hdrInfoSize; diff --git a/src/coreclr/vm/stackwalk.h b/src/coreclr/vm/stackwalk.h index 99c54be22b035e..12094594907fc0 100644 --- a/src/coreclr/vm/stackwalk.h +++ b/src/coreclr/vm/stackwalk.h @@ -613,7 +613,7 @@ class StackFrameIterator CONTRACTL { MODE_ANY; - GC_TRIGGERS; + GC_NOTRIGGER; NOTHROW; } CONTRACTL_END From d899e0b3ecf977dabe86fdbbdc0dd2a4ab84bd9c Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Mon, 14 Apr 2025 13:25:41 -0700 Subject: [PATCH 3/5] Update src/coreclr/vm/clrex.h Co-authored-by: Filip Navara --- src/coreclr/vm/clrex.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/coreclr/vm/clrex.h b/src/coreclr/vm/clrex.h index 71fc4726010947..007c049b35ff9b 100644 --- a/src/coreclr/vm/clrex.h +++ b/src/coreclr/vm/clrex.h @@ -1173,5 +1173,4 @@ class CLRLastThrownObjectException : public CLRException bool IsHRESULTForExceptionKind(HRESULT hr, RuntimeExceptionKind kind); #endif // _CLREX_H_ -``` From cc6d9b531c53504d622e2c483503e47e6eac89b1 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Mon, 21 Apr 2025 10:28:51 -0700 Subject: [PATCH 4/5] Attempt to address issue with reflection emit modules --- src/coreclr/inc/corhdr.h | 2 ++ src/coreclr/vm/ceeload.cpp | 61 +++++++++++++++++++---------------- src/coreclr/vm/ceeload.h | 11 +++++++ src/coreclr/vm/comdynamic.cpp | 6 ++++ 4 files changed, 52 insertions(+), 28 deletions(-) diff --git a/src/coreclr/inc/corhdr.h b/src/coreclr/inc/corhdr.h index 84f7ebcf428b75..9f79288b2b4fc5 100644 --- a/src/coreclr/inc/corhdr.h +++ b/src/coreclr/inc/corhdr.h @@ -1680,6 +1680,8 @@ typedef enum CorAttributeTargets // Keep in sync with RuntimeCompatibilityAttribute.cs #define RUNTIMECOMPATIBILITY_TYPE_W W("System.Runtime.CompilerServices.RuntimeCompatibilityAttribute") #define RUNTIMECOMPATIBILITY_TYPE "System.Runtime.CompilerServices.RuntimeCompatibilityAttribute" +#define RUNTIMECOMPATIBILITY_TYPE_NAMESPACE "System.Runtime.CompilerServices" +#define RUNTIMECOMPATIBILITY_TYPE_NAME "RuntimeCompatibilityAttribute" // Keep in sync with AssemblySettingAttributes.cs diff --git a/src/coreclr/vm/ceeload.cpp b/src/coreclr/vm/ceeload.cpp index 60a9373cb44cbe..19fef34460d3d0 100644 --- a/src/coreclr/vm/ceeload.cpp +++ b/src/coreclr/vm/ceeload.cpp @@ -1092,44 +1092,48 @@ BOOL Module::IsRuntimeWrapExceptions() if (!(IsRuntimeWrapExceptionsStatusComputed())) { - HRESULT hr; - BOOL fRuntimeWrapExceptions = FALSE; + UpdateCachedIsRuntimeWrapExceptions(); + } + return !!(m_dwPersistedFlags & WRAP_EXCEPTIONS); +} - IMDInternalImport *mdImport = GetAssembly()->GetMDImport(); +void Module::UpdateCachedIsRuntimeWrapExceptions() +{ + HRESULT hr; + BOOL fRuntimeWrapExceptions = FALSE; - mdToken token; - IfFailGo(mdImport->GetAssemblyFromScope(&token)); + IMDInternalImport *mdImport = GetAssembly()->GetMDImport(); - const BYTE *pVal; - ULONG cbVal; + mdToken token; + IfFailGo(mdImport->GetAssemblyFromScope(&token)); - hr = mdImport->GetCustomAttributeByName(token, - RUNTIMECOMPATIBILITY_TYPE, - (const void**)&pVal, &cbVal); + const BYTE *pVal; + ULONG cbVal; - // Parse the attribute - if (hr == S_OK) - { - CustomAttributeParser ca(pVal, cbVal); - CaNamedArg namedArgs[1] = {{0}}; + hr = mdImport->GetCustomAttributeByName(token, + RUNTIMECOMPATIBILITY_TYPE, + (const void**)&pVal, &cbVal); - // First, the void constructor: - IfFailGo(ParseKnownCaArgs(ca, NULL, 0)); + // Parse the attribute + if (hr == S_OK) + { + CustomAttributeParser ca(pVal, cbVal); + CaNamedArg namedArgs[1] = {{0}}; - // Then, find the named argument - namedArgs[0].InitBoolField("WrapNonExceptionThrows"); + // First, the void constructor: + IfFailGo(ParseKnownCaArgs(ca, NULL, 0)); - IfFailGo(ParseKnownCaNamedArgs(ca, namedArgs, ARRAY_SIZE(namedArgs))); + // Then, find the named argument + namedArgs[0].InitBoolField("WrapNonExceptionThrows"); - if (namedArgs[0].val.boolean) - fRuntimeWrapExceptions = TRUE; - } -ErrExit: - InterlockedOr((LONG*)&m_dwPersistedFlags, COMPUTED_WRAP_EXCEPTIONS | - (fRuntimeWrapExceptions ? WRAP_EXCEPTIONS : 0)); - } + IfFailGo(ParseKnownCaNamedArgs(ca, namedArgs, ARRAY_SIZE(namedArgs))); - return !!(m_dwPersistedFlags & WRAP_EXCEPTIONS); + if (namedArgs[0].val.boolean) + fRuntimeWrapExceptions = TRUE; + } +ErrExit: + InterlockedOr((LONG*)&m_dwPersistedFlags, COMPUTED_WRAP_EXCEPTIONS | + (fRuntimeWrapExceptions ? WRAP_EXCEPTIONS : 0)); } BOOL Module::IsRuntimeMarshallingEnabled() @@ -3756,6 +3760,7 @@ ReflectionModule *ReflectionModule::Create(Assembly *pAssembly, PEAssembly *pPEA ReflectionModuleHolder pModule(new (pMemory) ReflectionModule(pAssembly, pPEAssembly)); pModule->DoInit(pamTracker, szName); + pModule->SetRuntimeMarshallingEnabledCached_ForReflectionEmitModules(); RETURN pModule.Extract(); } diff --git a/src/coreclr/vm/ceeload.h b/src/coreclr/vm/ceeload.h index 07a7c24fb46742..4064ff463e3b26 100644 --- a/src/coreclr/vm/ceeload.h +++ b/src/coreclr/vm/ceeload.h @@ -1554,6 +1554,7 @@ class Module : public ModuleBase // words, they become compliant //----------------------------------------------------------------------------------------- BOOL IsRuntimeWrapExceptions(); + void UpdateCachedIsRuntimeWrapExceptions(); BOOL IsRuntimeWrapExceptionsDuringEH(); //----------------------------------------------------------------------------------------- @@ -1568,6 +1569,16 @@ class Module : public ModuleBase return (m_dwPersistedFlags & RUNTIME_MARSHALLING_ENABLED_IS_CACHED); } +protected: + // For reflection emit modules we set this flag when we emit the attribute, and always consider + // the current setting of the flag to be set. + void SetRuntimeMarshallingEnabledCached_ForReflectionEmitModules() + { + LIMITED_METHOD_CONTRACT; + m_dwPersistedFlags |= RUNTIME_MARSHALLING_ENABLED_IS_CACHED; + } +public: + BOOL HasDefaultDllImportSearchPathsAttribute(); BOOL IsDefaultDllImportSearchPathsAttributeCached() diff --git a/src/coreclr/vm/comdynamic.cpp b/src/coreclr/vm/comdynamic.cpp index fa40c57dbfe2f9..d0b07f6697c8e2 100644 --- a/src/coreclr/vm/comdynamic.cpp +++ b/src/coreclr/vm/comdynamic.cpp @@ -908,6 +908,12 @@ void UpdateRuntimeStateForAssemblyCustomAttribute(Module* pModule, mdToken tkCus Assembly* pAssembly = pModule->GetAssembly(); pAssembly->UpdateCachedFriendAssemblyInfo(); } + + // System.Runtime.CompilerServices.RuntimeCompatibilityAttribute processing + if (((strcmp(szNamespace, RUNTIMECOMPATIBILITY_TYPE_NAMESPACE) == 0) && (strcmp(szName, RUNTIMECOMPATIBILITY_TYPE_NAME) == 0))) + { + pModule->UpdateCachedIsRuntimeWrapExceptions(); + } } extern "C" void QCALLTYPE TypeBuilder_DefineCustomAttribute(QCall::ModuleHandle pModule, INT32 token, INT32 conTok, LPCBYTE pBlob, INT32 cbBlob) From 8548bd662edc19da42abe2d14dce3ad9922f9ed2 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Mon, 21 Apr 2025 13:41:41 -0700 Subject: [PATCH 5/5] I really should be more careful about my AI generated code completion... --- src/coreclr/vm/ceeload.cpp | 2 +- src/coreclr/vm/ceeload.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/vm/ceeload.cpp b/src/coreclr/vm/ceeload.cpp index afee5306666124..b9552d1c98277a 100644 --- a/src/coreclr/vm/ceeload.cpp +++ b/src/coreclr/vm/ceeload.cpp @@ -3763,7 +3763,7 @@ ReflectionModule *ReflectionModule::Create(Assembly *pAssembly, PEAssembly *pPEA ReflectionModuleHolder pModule(new (pMemory) ReflectionModule(pAssembly, pPEAssembly)); pModule->DoInit(pamTracker, szName); - pModule->SetRuntimeMarshallingEnabledCached_ForReflectionEmitModules(); + pModule->SetIsRuntimeWrapExceptionsCached_ForReflectionEmitModules(); RETURN pModule.Extract(); } diff --git a/src/coreclr/vm/ceeload.h b/src/coreclr/vm/ceeload.h index 4064ff463e3b26..ac0ea11cd9ca6a 100644 --- a/src/coreclr/vm/ceeload.h +++ b/src/coreclr/vm/ceeload.h @@ -1572,10 +1572,10 @@ class Module : public ModuleBase protected: // For reflection emit modules we set this flag when we emit the attribute, and always consider // the current setting of the flag to be set. - void SetRuntimeMarshallingEnabledCached_ForReflectionEmitModules() + void SetIsRuntimeWrapExceptionsCached_ForReflectionEmitModules() { LIMITED_METHOD_CONTRACT; - m_dwPersistedFlags |= RUNTIME_MARSHALLING_ENABLED_IS_CACHED; + m_dwPersistedFlags |= COMPUTED_WRAP_EXCEPTIONS; } public: