Remove global spinlock for EH stacktrace

src/coreclr/System.Private.CoreLib/src/System/Exception.CoreCLR.cs

@@ -128,10 +128,7 @@ internal void InternalPreserveStackTrace()
         private static extern void PrepareForForeignExceptionRaise();
 
         [MethodImpl(MethodImplOptions.InternalCall)]
-        private static extern void GetStackTracesDeepCopy(Exception exception, out byte[]? currentStackTrace, out object[]? dynamicMethodArray);
-
-        [MethodImpl(MethodImplOptions.InternalCall)]
-        internal static extern void SaveStackTracesFromDeepCopy(Exception exception, byte[]? currentStackTrace, object[]? dynamicMethodArray);
+        private static extern object? GetFrozenStackTrace(Exception exception);
 
         [MethodImpl(MethodImplOptions.InternalCall)]
         internal static extern uint GetExceptionCount();
@@ -147,18 +144,13 @@ internal void RestoreDispatchState(in DispatchState dispatchState)
                 // in the exception object. This will ensure that when this exception is thrown and these
                 // fields are modified, then EDI's references remain intact.
                 //
-                byte[]? stackTraceCopy = (byte[]?)dispatchState.StackTrace?.Clone();
-                object[]? dynamicMethodsCopy = (object[]?)dispatchState.DynamicMethods?.Clone();
 
                 // Watson buckets and remoteStackTraceString fields are captured and restored without any locks. It is possible for them to
                 // get out of sync without violating overall integrity of the system.
                 _watsonBuckets = dispatchState.WatsonBuckets;
                 _ipForWatsonBuckets = dispatchState.IpForWatsonBuckets;
                 _remoteStackTraceString = dispatchState.RemoteStackTrace;
-
-                // The binary stack trace and references to dynamic methods have to be restored under a lock to guarantee integrity of the system.
-                SaveStackTracesFromDeepCopy(this, stackTraceCopy, dynamicMethodsCopy);
-
+                _stackTrace = dispatchState.StackTrace;
                 _stackTraceString = null;
 
                 // Marks the TES state to indicate we have restored foreign exception
@@ -172,7 +164,7 @@ internal void RestoreDispatchState(in DispatchState dispatchState)
         private IDictionary? _data;
         private readonly Exception? _innerException;
         private string? _helpURL;
-        private byte[]? _stackTrace;
+        private object? _stackTrace;
         private byte[]? _watsonBuckets;
         private string? _stackTraceString; // Needed for serialization.
         private string? _remoteStackTraceString;
@@ -181,7 +173,6 @@ internal void RestoreDispatchState(in DispatchState dispatchState)
         // DynamicMethodDescs alive for the lifetime of the exception. We do this because
         // the _stackTrace field holds MethodDescs, and a DynamicMethodDesc can be destroyed
         // unless a System.Resolver object roots it.
-        private readonly object[]? _dynamicMethods;
         private string? _source;         // Mainly used by VB.
         private UIntPtr _ipForWatsonBuckets; // Used to persist the IP for Watson Bucketing
         private readonly IntPtr _xptrs;             // Internal EE stuff
@@ -227,21 +218,18 @@ internal static string GetMessageFromNativeResources(ExceptionMessageKind kind)
 
         internal readonly struct DispatchState
         {
-            public readonly byte[]? StackTrace;
-            public readonly object[]? DynamicMethods;
+            public readonly object? StackTrace;
             public readonly string? RemoteStackTrace;
             public readonly UIntPtr IpForWatsonBuckets;
             public readonly byte[]? WatsonBuckets;
 
             public DispatchState(
-                byte[]? stackTrace,
-                object[]? dynamicMethods,
+                object? stackTrace,
                 string? remoteStackTrace,
                 UIntPtr ipForWatsonBuckets,
                 byte[]? watsonBuckets)
             {
                 StackTrace = stackTrace;
-                DynamicMethods = dynamicMethods;
                 RemoteStackTrace = remoteStackTrace;
                 IpForWatsonBuckets = ipForWatsonBuckets;
                 WatsonBuckets = watsonBuckets;
@@ -250,9 +238,9 @@ public DispatchState(
 
         internal DispatchState CaptureDispatchState()
         {
-            GetStackTracesDeepCopy(this, out byte[]? stackTrace, out object[]? dynamicMethods);
+            object? stackTrace = GetFrozenStackTrace(this);
 
-            return new DispatchState(stackTrace, dynamicMethods,
+            return new DispatchState(stackTrace,
                 _remoteStackTraceString, _ipForWatsonBuckets, _watsonBuckets);
         }
 

src/coreclr/System.Private.CoreLib/src/System/Runtime/ExceptionServices/AsmOffsets.cs

@@ -150,23 +150,23 @@ class AsmOffsets
     public const int SIZEOF__EHEnum = 0x20;
     public const int OFFSETOF__StackFrameIterator__m_pRegDisplay = 0x228;
     public const int OFFSETOF__ExInfo__m_pPrevExInfo = 0;
-    public const int OFFSETOF__ExInfo__m_pExContext = 0xc0;
-    public const int OFFSETOF__ExInfo__m_exception = 0xc8;
-    public const int OFFSETOF__ExInfo__m_kind = 0xd0;
-    public const int OFFSETOF__ExInfo__m_passNumber = 0xd1;
-    public const int OFFSETOF__ExInfo__m_idxCurClause = 0xd4;
-    public const int OFFSETOF__ExInfo__m_frameIter = 0xd8;
+    public const int OFFSETOF__ExInfo__m_pExContext = 0xb0;
+    public const int OFFSETOF__ExInfo__m_exception = 0xb8;
+    public const int OFFSETOF__ExInfo__m_kind = 0xc0;
+    public const int OFFSETOF__ExInfo__m_passNumber = 0xc1;
+    public const int OFFSETOF__ExInfo__m_idxCurClause = 0xc4;
+    public const int OFFSETOF__ExInfo__m_frameIter = 0xc8;
     public const int OFFSETOF__ExInfo__m_notifyDebuggerSP = OFFSETOF__ExInfo__m_frameIter + SIZEOF__StackFrameIterator;
 #else // TARGET_64BIT
     public const int SIZEOF__EHEnum = 0x10;
     public const int OFFSETOF__StackFrameIterator__m_pRegDisplay = 0x218;
     public const int OFFSETOF__ExInfo__m_pPrevExInfo = 0;
-    public const int OFFSETOF__ExInfo__m_pExContext = 0x70;
-    public const int OFFSETOF__ExInfo__m_exception = 0x74;
-    public const int OFFSETOF__ExInfo__m_kind = 0x78;
-    public const int OFFSETOF__ExInfo__m_passNumber = 0x79;
-    public const int OFFSETOF__ExInfo__m_idxCurClause = 0x7C;
-    public const int OFFSETOF__ExInfo__m_frameIter = 0x80;
+    public const int OFFSETOF__ExInfo__m_pExContext = 0x60;
+    public const int OFFSETOF__ExInfo__m_exception = 0x64;
+    public const int OFFSETOF__ExInfo__m_kind = 0x68;
+    public const int OFFSETOF__ExInfo__m_passNumber = 0x69;
+    public const int OFFSETOF__ExInfo__m_idxCurClause = 0x6c;
+    public const int OFFSETOF__ExInfo__m_frameIter = 0x70;
     public const int OFFSETOF__ExInfo__m_notifyDebuggerSP = OFFSETOF__ExInfo__m_frameIter + SIZEOF__StackFrameIterator;
 #endif // TARGET_64BIT
 

src/coreclr/classlibnative/bcltype/system.cpp

@@ -93,6 +93,13 @@ FCIMPL1(ReflectMethodObject*, SystemNative::GetMethodFromStackTrace, ArrayBase*
 {
     FCALL_CONTRACT;
 
+    // The pStackTraceUNSAFE can be either I1Array or Object[]. In the latter case, the first entry is the actual stack trace I1Array,
+    // the rest are pointers to the method info objects. We only care about the first entry here.
+    if (pStackTraceUNSAFE->GetArrayElementType() != ELEMENT_TYPE_I1)
+    {
+        PtrArray *combinedArray = (PtrArray*)pStackTraceUNSAFE;
+        pStackTraceUNSAFE = (ArrayBase*)OBJECTREFToObject(combinedArray->GetAt(0));
+    }
     I1ARRAYREF pArray(static_cast<I1Array *>(pStackTraceUNSAFE));
     StackTraceArray stackArray(pArray);
 

src/coreclr/debug/daccess/enummem.cpp

@@ -533,7 +533,7 @@ HRESULT ClrDataAccess::DumpManagedExcepObject(CLRDataEnumMemoryFlags flags, OBJE
     DumpManagedExcepObject(flags, exceptRef->GetInnerException());
 
     // Dump the stack trace array object and its underlying type
-    I1ARRAYREF stackTraceArrayObj = exceptRef->GetStackTraceArrayObject();
+    OBJECTREF stackTraceArrayObj = exceptRef->GetStackTraceArrayObject();
 
     // There are cases where a managed exception does not have a stack trace.
     // These cases are:
@@ -558,6 +558,22 @@ HRESULT ClrDataAccess::DumpManagedExcepObject(CLRDataEnumMemoryFlags flags, OBJE
     // MD this happens.
     StackTraceArray stackTrace;
     exceptRef->GetStackTrace(stackTrace);
+
+    // The stackTraceArrayObj can be either a byte[] with the actual stack trace array or an object[] where the first element is the actual stack trace array.
+    // In case it was the latter, we need to dump the actual stack trace array object here too.
+    OBJECTREF actualStackTraceArrayObj = (OBJECTREF)stackTrace.Get();
+    if (actualStackTraceArrayObj != stackTraceArrayObj)
+    {
+        // first dump the array's element type
+        TypeHandle arrayTypeHandle = actualStackTraceArrayObj->GetTypeHandle();
+        TypeHandle elementTypeHandle = arrayTypeHandle.GetArrayElementTypeHandle();
+        elementTypeHandle.AsMethodTable()->EnumMemoryRegions(flags);
+        elementTypeHandle.AsMethodTable()->GetClass()->EnumMemoryRegions(flags, elementTypeHandle.AsMethodTable());
+
+        // now dump the actual stack trace array object
+        DumpManagedObject(flags, actualStackTraceArrayObj);
+    }
+
     for(size_t i = 0; i < stackTrace.Size(); i++)
     {
         MethodDesc* pMD = stackTrace[i].pFunc;

src/coreclr/inc/sospriv.idl

@@ -444,7 +444,7 @@ interface ISOSDacInterface8 : IUnknown
 // Increment anytime there is a change in the data structures that SOS depends on like
 // stress log structs (StressMsg, StressLogChunck, ThreadStressLog, etc), exception
 // stack traces (StackTraceElement), the PredefinedTlsSlots enums, etc.
-cpp_quote("#define SOS_BREAKING_CHANGE_VERSION 4")
+cpp_quote("#define SOS_BREAKING_CHANGE_VERSION 5")
 
 [
     object,

src/coreclr/vm/clrex.h

@@ -27,6 +27,9 @@ enum StackTraceElementFlags
 
     // Set if the "ip" field has already been adjusted (decremented)
     STEF_IP_ADJUSTED = 0x0002,
+
+    // Set if the element references a method that needs a keep alive object
+    STEF_KEEPALIVE = 0x0004,
 };
 
 // This struct is used by SOS in the diagnostic repo.
@@ -51,28 +54,15 @@ struct StackTraceElement
     }
 };
 
-// This struct is used by SOS in the diagnostic repo.
-// See: https://github.com/dotnet/diagnostics/blob/9ff35f13af2f03a68a166cfd53f1a4bb32425f2f/src/SOS/Strike/strike.cpp#L2669
 class StackTraceInfo
 {
-private:
-    // for building stack trace info
-    StackTraceElement*  m_pStackTrace;      // pointer to stack trace storage
-    unsigned            m_cStackTrace;      // size of stack trace storage
-    unsigned            m_dFrameCount;      // current frame in stack trace
-    unsigned            m_cDynamicMethodItems; // number of items in the Dynamic Method array
-    unsigned            m_dCurrentDynamicIndex; // index of the next location where the resolver object will be stored
+    int m_dummy; // remove this
 
+    static OBJECTREF GetKeepAliveObject(MethodDesc* pMethod);
+    static void EnsureStackTraceArray(StackTraceArray *pStackTrace, size_t neededSize);
+    static void EnsureKeepAliveArray(PTRARRAYREF *ppKeepAliveArray, size_t neededSize);
 public:
-    void Init();
-    BOOL IsEmpty();
-    void AllocateStackTrace();
-    void ClearStackTrace();
-    void FreeStackTrace();
-    void SaveStackTrace(BOOL bAllowAllocMem, OBJECTHANDLE hThrowable, BOOL bReplaceStack, BOOL bSkipLastElement);
-    BOOL AppendElement(BOOL bAllowAllocMem, UINT_PTR currentIP, UINT_PTR currentSP, MethodDesc* pFunc, CrawlFrame* pCf);
-
-    void GetLeafFrameInfo(StackTraceElement* pStackTraceElement);
+    static BOOL AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, UINT_PTR currentSP, MethodDesc* pFunc, CrawlFrame* pCf);
 };
 
 

src/coreclr/vm/comutilnative.cpp

@@ -76,75 +76,49 @@ FCIMPL0(VOID, ExceptionNative::PrepareForForeignExceptionRaise)
 }
 FCIMPLEND
 
-// Given an exception object, this method will extract the stacktrace and dynamic method array and set them up for return to the caller.
-FCIMPL3(VOID, ExceptionNative::GetStackTracesDeepCopy, Object* pExceptionObjectUnsafe, Object **pStackTraceUnsafe, Object **pDynamicMethodsUnsafe);
+void DeepCopyStackTrace(StackTraceArray &stackTrace, StackTraceArray &stackTraceCopy, PTRARRAYREF *pKeepAliveArray, PTRARRAYREF *pKeepAliveArrayCopy)
 {
     CONTRACTL
     {
-        FCALL_CHECK;
+        THROWS;
+        GC_TRIGGERS;
+        MODE_COOPERATIVE;
+        PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)&stackTrace));
+        PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)&stackTraceCopy));
+        PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)pKeepAliveArray));
+        PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)pKeepAliveArrayCopy));
     }
     CONTRACTL_END;
 
-    ASSERT(pExceptionObjectUnsafe != NULL);
-    ASSERT(pStackTraceUnsafe != NULL);
-    ASSERT(pDynamicMethodsUnsafe != NULL);
-
-    struct
+    if (stackTrace.Get() != NULL)
     {
-        StackTraceArray stackTrace;
-        StackTraceArray stackTraceCopy;
-        EXCEPTIONREF refException;
-        PTRARRAYREF dynamicMethodsArray; // Object array of Managed Resolvers
-        PTRARRAYREF dynamicMethodsArrayCopy; // Copy of the object array of Managed Resolvers
-    } gc;
-    gc.refException = NULL;
-    gc.dynamicMethodsArray = NULL;
-    gc.dynamicMethodsArrayCopy = NULL;
-
-    // GC protect the array reference
-    HELPER_METHOD_FRAME_BEGIN_PROTECT(gc);
-
-    // Get the exception object reference
-    gc.refException = (EXCEPTIONREF)(ObjectToOBJECTREF(pExceptionObjectUnsafe));
-
-    // Fetch the stacktrace details from the exception under a lock
-    gc.refException->GetStackTrace(gc.stackTrace, &gc.dynamicMethodsArray);
-
-    bool fHaveStackTrace = false;
-    bool fHaveDynamicMethodArray = false;
-
-    if ((unsigned)gc.stackTrace.Size() > 0)
+        stackTraceCopy.CopyFrom(stackTrace);
+    }
+    else
     {
-        // Deepcopy the array
-        gc.stackTraceCopy.CopyFrom(gc.stackTrace);
-        fHaveStackTrace = true;
+        stackTraceCopy.Set(NULL);
     }
 
-    if (gc.dynamicMethodsArray != NULL)
+    if (*pKeepAliveArray != NULL)
     {
+        // The stack trace object is the keepAlive array with its first slot set to the stack trace I1Array.
         // Get the number of elements in the dynamic methods array
-        unsigned   cOrigDynamic = gc.dynamicMethodsArray->GetNumComponents();
+        unsigned cOrigKeepAlive = (*pKeepAliveArray)->GetNumComponents();
 
         // ..and allocate a new array. This can trigger GC or throw under OOM.
-        gc.dynamicMethodsArrayCopy = (PTRARRAYREF)AllocateObjectArray(cOrigDynamic, g_pObjectClass);
+        *pKeepAliveArrayCopy = (PTRARRAYREF)AllocateObjectArray(cOrigKeepAlive, g_pObjectClass);
 
         // Deepcopy references to the new array we just allocated
-        memmoveGCRefs(gc.dynamicMethodsArrayCopy->GetDataPtr(), gc.dynamicMethodsArray->GetDataPtr(),
-                                                  cOrigDynamic * sizeof(Object *));
+        memmoveGCRefs((*pKeepAliveArrayCopy)->GetDataPtr(), (*pKeepAliveArray)->GetDataPtr(),
+                      cOrigKeepAlive * sizeof(Object *));
 
-        fHaveDynamicMethodArray = true;
+        (*pKeepAliveArrayCopy)->SetAt(0, (PTRARRAYREF)stackTraceCopy.Get());
     }
-
-    // Prep to return
-    *pStackTraceUnsafe = fHaveStackTrace?OBJECTREFToObject(gc.stackTraceCopy.Get()):NULL;
-    *pDynamicMethodsUnsafe = fHaveDynamicMethodArray?OBJECTREFToObject(gc.dynamicMethodsArrayCopy):NULL;
-
-    HELPER_METHOD_FRAME_END();
 }
-FCIMPLEND
 
-// Given an exception object and deep copied instances of a stacktrace and/or dynamic method array, this method will set the latter in the exception object instance.
-FCIMPL3(VOID, ExceptionNative::SaveStackTracesFromDeepCopy, Object* pExceptionObjectUnsafe, Object *pStackTraceUnsafe, Object *pDynamicMethodsUnsafe);
+// Given an exception object, this method will mark its stack trace as frozen and return it to the caller.
+// Frozen stack traces are immutable, when a thread attempts to add a frame to it, the stack trace is cloned first.
+FCIMPL1(Object *, ExceptionNative::GetFrozenStackTrace, Object* pExceptionObjectUnsafe);
 {
     CONTRACTL
     {
@@ -157,44 +131,39 @@ FCIMPL3(VOID, ExceptionNative::SaveStackTracesFromDeepCopy, Object* pExceptionOb
     struct
     {
         StackTraceArray stackTrace;
-        EXCEPTIONREF refException;
-        PTRARRAYREF dynamicMethodsArray; // Object array of Managed Resolvers
+        EXCEPTIONREF refException = NULL;
+        PTRARRAYREF keepAliveArray = NULL; // Object array of Managed Resolvers / AssemblyLoadContexts
+        OBJECTREF result = NULL;
     } gc;
-    gc.refException = NULL;
-    gc.dynamicMethodsArray = NULL;
 
     // GC protect the array reference
-    HELPER_METHOD_FRAME_BEGIN_PROTECT(gc);
+    HELPER_METHOD_FRAME_BEGIN_RET_PROTECT(gc);
 
     // Get the exception object reference
     gc.refException = (EXCEPTIONREF)(ObjectToOBJECTREF(pExceptionObjectUnsafe));
 
-    if (pStackTraceUnsafe != NULL)
-    {
-        // Copy the stacktrace
-        StackTraceArray stackTraceArray((I1ARRAYREF)ObjectToOBJECTREF(pStackTraceUnsafe));
-        gc.stackTrace.Swap(stackTraceArray);
-    }
+    gc.refException->GetStackTrace(gc.stackTrace, &gc.keepAliveArray);
 
-    gc.dynamicMethodsArray = NULL;
-    if (pDynamicMethodsUnsafe != NULL)
+    gc.stackTrace.MarkAsFrozen();
+
+    if (gc.keepAliveArray != NULL)
     {
-        gc.dynamicMethodsArray = (PTRARRAYREF)ObjectToOBJECTREF(pDynamicMethodsUnsafe);
+        gc.result = gc.keepAliveArray;
     }
-
-    // If there is no stacktrace, then there cannot be any dynamic method array. Thus,
-    // save stacktrace only when we have it.
-    if (gc.stackTrace.Size() > 0)
+    else if (gc.stackTrace.Get() != NULL)
     {
-        // Save the stacktrace details in the exception under a lock
-        gc.refException->SetStackTrace(gc.stackTrace.Get(), gc.dynamicMethodsArray);
+        gc.result = gc.stackTrace.Get();
     }
     else
     {
-        gc.refException->SetStackTrace(NULL, NULL);
+        gc.result = NULL;
     }
+
+    // There can be no GC after setting the frozenStackTrace until the Object is returned.
 
     HELPER_METHOD_FRAME_END();
+
+    return OBJECTREFToObject(gc.result);
 }
 FCIMPLEND
 

src/coreclr/vm/comutilnative.h

@@ -42,9 +42,7 @@ class ExceptionNative
     static FCDECL1(FC_BOOL_RET, IsTransient, INT32 hresult);
     static FCDECL3(StringObject *, StripFileInfo, Object *orefExcepUNSAFE, StringObject *orefStrUNSAFE, CLR_BOOL isRemoteStackTrace);
     static FCDECL0(VOID, PrepareForForeignExceptionRaise);
-    static FCDECL3(VOID, GetStackTracesDeepCopy, Object* pExceptionObjectUnsafe, Object **pStackTraceUnsafe, Object **pDynamicMethodsUnsafe);
-    static FCDECL3(VOID, SaveStackTracesFromDeepCopy, Object* pExceptionObjectUnsafe, Object *pStackTraceUnsafe, Object *pDynamicMethodsUnsafe);
-
+    static FCDECL1(Object *, GetFrozenStackTrace, Object* pExceptionObjectUnsafe);
 
 #ifdef FEATURE_COMINTEROP
     // NOTE: caller cleans up any partially initialized BSTRs in pED

src/coreclr/vm/corelib.h

@@ -311,7 +311,6 @@ DEFINE_FIELD_U(_stackTrace,        ExceptionObject,    _stackTrace)
 DEFINE_FIELD_U(_watsonBuckets,     ExceptionObject,    _watsonBuckets)
 DEFINE_FIELD_U(_stackTraceString,  ExceptionObject,    _stackTraceString)
 DEFINE_FIELD_U(_remoteStackTraceString, ExceptionObject, _remoteStackTraceString)
-DEFINE_FIELD_U(_dynamicMethods,    ExceptionObject,    _dynamicMethods)
 DEFINE_FIELD_U(_source,            ExceptionObject,    _source)
 DEFINE_FIELD_U(_ipForWatsonBuckets,ExceptionObject,    _ipForWatsonBuckets)
 DEFINE_FIELD_U(_xptrs,             ExceptionObject,    _xptrs)