Remove global spinlock for EH stacktrace

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

@@ -128,10 +128,10 @@ internal void InternalPreserveStackTrace()
         private static extern void PrepareForForeignExceptionRaise();
 
         [MethodImpl(MethodImplOptions.InternalCall)]
-        private static extern void GetStackTracesDeepCopy(Exception exception, out byte[]? currentStackTrace, out object[]? dynamicMethodArray);
+        private static extern void GetStackTracesDeepCopy(Exception exception, out object? currentStackTrace);
 
         [MethodImpl(MethodImplOptions.InternalCall)]
-        internal static extern void SaveStackTracesFromDeepCopy(Exception exception, byte[]? currentStackTrace, object[]? dynamicMethodArray);
+        internal static extern void SaveStackTracesFromDeepCopy(Exception exception, object? currentStackTrace);
 
         [MethodImpl(MethodImplOptions.InternalCall)]
         internal static extern uint GetExceptionCount();
@@ -147,17 +147,14 @@ 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);
+                SaveStackTracesFromDeepCopy(this, dispatchState.StackTrace);
 
                 _stackTraceString = null;
 
@@ -172,7 +169,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 +178,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 +223,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 +243,9 @@ public DispatchState(
 
         internal DispatchState CaptureDispatchState()
         {
-            GetStackTracesDeepCopy(this, out byte[]? stackTrace, out object[]? dynamicMethods);
+            GetStackTracesDeepCopy(this, out object? stackTrace);
 
-            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

@@ -51,28 +51,16 @@ 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_keepaliveItemsCount = -1; // -1 indicates the count is not initialized yet
 
+    static OBJECTREF GetKeepaliveObject(MethodDesc* pMethod);
+    static int GetKeepaliveItemsCount(StackTraceArray *pStackTrace);
+    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);
+    BOOL AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, UINT_PTR currentSP, MethodDesc* pFunc, CrawlFrame* pCf);
 };
 
 

src/coreclr/vm/comutilnative.cpp

@@ -76,8 +76,44 @@ FCIMPL0(VOID, ExceptionNative::PrepareForForeignExceptionRaise)
 }
 FCIMPLEND
 
+void DeepCopyStackTrace(StackTraceArray &stackTrace, StackTraceArray &stackTraceCopy, PTRARRAYREF *pKeepaliveArray, PTRARRAYREF *pKeepaliveArrayCopy)
+{
+    CONTRACTL
+    {
+        THROWS;
+        GC_TRIGGERS;
+        MODE_COOPERATIVE;
+    }
+    CONTRACTL_END;
+
+    if (stackTrace.Get() != NULL)
+    {
+        stackTraceCopy.CopyFrom(stackTrace);
+    }
+    else
+    {
+        stackTraceCopy.Set(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 cOrigKeepalive = (*pKeepaliveArray)->GetNumComponents();
+
+        // ..and allocate a new array. This can trigger GC or throw under OOM.
+        *pKeepaliveArrayCopy = (PTRARRAYREF)AllocateObjectArray(cOrigKeepalive, g_pObjectClass);
+
+        // Deepcopy references to the new array we just allocated
+        memmoveGCRefs((*pKeepaliveArrayCopy)->GetDataPtr(), (*pKeepaliveArray)->GetDataPtr(),
+                      cOrigKeepalive * sizeof(Object *));
+
+        (*pKeepaliveArrayCopy)->SetAt(0, (PTRARRAYREF)stackTraceCopy.Get());
+    }
+}
+
 // 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);
+FCIMPL2(VOID, ExceptionNative::GetStackTracesDeepCopy, Object* pExceptionObjectUnsafe, Object **pStackTraceUnsafe);
 {
     CONTRACTL
     {
@@ -87,111 +123,83 @@ FCIMPL3(VOID, ExceptionNative::GetStackTracesDeepCopy, Object* pExceptionObjectU
 
     ASSERT(pExceptionObjectUnsafe != NULL);
     ASSERT(pStackTraceUnsafe != NULL);
-    ASSERT(pDynamicMethodsUnsafe != NULL);
 
     struct
     {
         StackTraceArray stackTrace;
         StackTraceArray stackTraceCopy;
-        EXCEPTIONREF refException;
-        PTRARRAYREF dynamicMethodsArray; // Object array of Managed Resolvers
-        PTRARRAYREF dynamicMethodsArrayCopy; // Copy of the object array of Managed Resolvers
+        EXCEPTIONREF refException = NULL;
+        PTRARRAYREF keepaliveArray = NULL; // Object array of Managed Resolvers
+        PTRARRAYREF keepaliveArrayCopy = NULL; // 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;
+    gc.refException->GetStackTrace(gc.stackTrace, &gc.keepaliveArray);
+    DeepCopyStackTrace(gc.stackTrace, gc.stackTraceCopy, &gc.keepaliveArray, &gc.keepaliveArrayCopy);
 
-    if ((unsigned)gc.stackTrace.Size() > 0)
+    if (gc.keepaliveArrayCopy != NULL)
     {
-        // Deepcopy the array
-        gc.stackTraceCopy.CopyFrom(gc.stackTrace);
-        fHaveStackTrace = true;
+        *pStackTraceUnsafe = OBJECTREFToObject(gc.keepaliveArrayCopy);
     }
-
-    if (gc.dynamicMethodsArray != NULL)
+    else if (gc.stackTrace.Get() != NULL)
     {
-        // Get the number of elements in the dynamic methods array
-        unsigned   cOrigDynamic = gc.dynamicMethodsArray->GetNumComponents();
-
-        // ..and allocate a new array. This can trigger GC or throw under OOM.
-        gc.dynamicMethodsArrayCopy = (PTRARRAYREF)AllocateObjectArray(cOrigDynamic, g_pObjectClass);
-
-        // Deepcopy references to the new array we just allocated
-        memmoveGCRefs(gc.dynamicMethodsArrayCopy->GetDataPtr(), gc.dynamicMethodsArray->GetDataPtr(),
-                                                  cOrigDynamic * sizeof(Object *));
-
-        fHaveDynamicMethodArray = true;
+        *pStackTraceUnsafe = OBJECTREFToObject(gc.stackTraceCopy.Get());
+    }
+    else
+    {
+        *pStackTraceUnsafe = NULL;
     }
-
-    // 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);
+FCIMPL2(VOID, ExceptionNative::SaveStackTracesFromDeepCopy, Object* pExceptionObjectUnsafe, Object *pStackTraceUnsafe);
 {
     CONTRACTL
     {
         FCALL_CHECK;
     }
     CONTRACTL_END;
-
+    
     ASSERT(pExceptionObjectUnsafe != NULL);
 
     struct
     {
         StackTraceArray stackTrace;
-        EXCEPTIONREF refException;
-        PTRARRAYREF dynamicMethodsArray; // Object array of Managed Resolvers
+        StackTraceArray stackTraceCopy;
+        OBJECTREF refStackTrace = NULL;
+        PTRARRAYREF refKeepaliveArray = NULL;
+        PTRARRAYREF refKeepaliveArrayCopy = NULL;
+        EXCEPTIONREF refException = NULL;
     } gc;
-    gc.refException = NULL;
-    gc.dynamicMethodsArray = NULL;
 
     // GC protect the array reference
     HELPER_METHOD_FRAME_BEGIN_PROTECT(gc);
 
     // Get the exception object reference
     gc.refException = (EXCEPTIONREF)(ObjectToOBJECTREF(pExceptionObjectUnsafe));
+    gc.refStackTrace = (OBJECTREF)(ObjectToOBJECTREF(pStackTraceUnsafe));
 
-    if (pStackTraceUnsafe != NULL)
-    {
-        // Copy the stacktrace
-        StackTraceArray stackTraceArray((I1ARRAYREF)ObjectToOBJECTREF(pStackTraceUnsafe));
-        gc.stackTrace.Swap(stackTraceArray);
-    }
-
-    gc.dynamicMethodsArray = NULL;
-    if (pDynamicMethodsUnsafe != NULL)
-    {
-        gc.dynamicMethodsArray = (PTRARRAYREF)ObjectToOBJECTREF(pDynamicMethodsUnsafe);
-    }
+    ExceptionObject::GetStackTraceParts(gc.refStackTrace, gc.stackTrace, &gc.refKeepaliveArray);
+    DeepCopyStackTrace(gc.stackTrace, gc.stackTraceCopy, &gc.refKeepaliveArray, &gc.refKeepaliveArrayCopy);
 
-    // 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)
+    if (gc.refKeepaliveArray != NULL)
     {
-        // Save the stacktrace details in the exception under a lock
-        gc.refException->SetStackTrace(gc.stackTrace.Get(), gc.dynamicMethodsArray);
+        // The stack trace object is the keepalive array with its first slot set to the stack trace I1Array.
+        // Save the stacktrace details in the exception
+        gc.refException->SetStackTrace(gc.refKeepaliveArrayCopy);
     }
     else
     {
-        gc.refException->SetStackTrace(NULL, NULL);
+        // The stack trace object is the stack trace I1Array.
+        // Save the stacktrace details in the exception
+        gc.refException->SetStackTrace(gc.stackTraceCopy.Get());
     }
 
     HELPER_METHOD_FRAME_END();