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

@@ -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);
 };