From 718f6384240f0a8261bd1513865e79da67ede755 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 28 Mar 2024 15:03:37 +0100 Subject: [PATCH 01/15] Remove global spinlock for EH stacktrace The global spinlock that was used to ensure that stack trace and the associated dynamic methods array were updated and read atomically. However, for the new EH, it has shown to cause a high contention in case many threads were handling exceptions at the same time. This change replaces the two arrays by one object member in the exception class. It contains reference to either the byte[] of the stack trace (when there are no dynamic methods on the stack trace) or an object[] where the first element contains the stack trace byte[] reference and the following elements contain what used to be in the dynamic array. That allows atomic updates and reads of the stack trace and dynamic method keepalive references without a need of a lock. The original code was quite convoluted, it was difficult to reason about and it had some races in it that were hidden behind the global lock. So I have decided to rewrite the whole thing from scratch. The way it ensures that it is race free is that whenever it updates the exception stack trace and the one that's on the exception was created by a different thread, it creates a deep copy of both the stack trace and the keepalive array. When making the copy, it also handles a case when a frame that needs a keepalive entry is on the stack trace part, but the keepalive array extracted from the exception is stale (the other thread needed to resize the keepalive array, but not the stack trace). In that case, the stack trace is trimmed at first such entry found. Since the case when multiple threads are throwing the same exception and so they are modifying its stack trace in parallel is pathological anyways, I believe the extra work spent on creating the clones of the arrays is a good tradeoff for ensuring easy to reason about thread safety. I have also removed a dead code path from the StackTraceInfo::SaveStackTrace. Finally, since with the previous iteration of this change, a bug in building the stack trace was found, I have added a coreclr test to verify stack trace for an exception matches the expectations. --- .../src/System/Exception.CoreCLR.cs | 23 +- .../Runtime/ExceptionServices/AsmOffsets.cs | 24 +- src/coreclr/classlibnative/bcltype/system.cpp | 7 + src/coreclr/debug/daccess/enummem.cpp | 18 +- src/coreclr/inc/sospriv.idl | 2 +- src/coreclr/vm/clrex.h | 24 +- src/coreclr/vm/comutilnative.cpp | 126 +-- src/coreclr/vm/comutilnative.h | 5 +- src/coreclr/vm/corelib.h | 1 - src/coreclr/vm/excep.cpp | 730 ++++++------------ src/coreclr/vm/excep.h | 8 +- src/coreclr/vm/exceptionhandling.cpp | 66 +- src/coreclr/vm/exceptionhandling.h | 5 - src/coreclr/vm/exinfo.cpp | 7 - src/coreclr/vm/exinfo.h | 6 + src/coreclr/vm/exstate.cpp | 18 - src/coreclr/vm/exstate.h | 2 - src/coreclr/vm/frames.h | 1 + src/coreclr/vm/i386/excepx86.cpp | 40 +- src/coreclr/vm/object.cpp | 202 +++-- src/coreclr/vm/object.h | 58 +- src/coreclr/vm/threads.cpp | 1 - .../exceptionstacktrace.cs | 251 ++++++ .../exceptionstacktrace.csproj | 8 + 24 files changed, 831 insertions(+), 802 deletions(-) create mode 100644 src/tests/baseservices/exceptions/exceptionstacktrace/exceptionstacktrace.cs create mode 100644 src/tests/baseservices/exceptions/exceptionstacktrace/exceptionstacktrace.csproj diff --git a/src/coreclr/System.Private.CoreLib/src/System/Exception.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Exception.CoreCLR.cs index 5fb81316b93ea..112fd7036cafb 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Exception.CoreCLR.cs +++ b/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,8 +147,6 @@ 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. @@ -156,8 +154,7 @@ internal void RestoreDispatchState(in DispatchState dispatchState) _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); } diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ExceptionServices/AsmOffsets.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ExceptionServices/AsmOffsets.cs index 7db188808e26a..4de704e77cd46 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ExceptionServices/AsmOffsets.cs +++ b/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 diff --git a/src/coreclr/classlibnative/bcltype/system.cpp b/src/coreclr/classlibnative/bcltype/system.cpp index 5d2f00cd849db..fdca0027e4cbb 100644 --- a/src/coreclr/classlibnative/bcltype/system.cpp +++ b/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(pStackTraceUNSAFE)); StackTraceArray stackArray(pArray); diff --git a/src/coreclr/debug/daccess/enummem.cpp b/src/coreclr/debug/daccess/enummem.cpp index 9c713aebfefd9..be3c1123ea416 100644 --- a/src/coreclr/debug/daccess/enummem.cpp +++ b/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; diff --git a/src/coreclr/inc/sospriv.idl b/src/coreclr/inc/sospriv.idl index c377df57a1530..98cfa0afe9a51 100644 --- a/src/coreclr/inc/sospriv.idl +++ b/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, diff --git a/src/coreclr/vm/clrex.h b/src/coreclr/vm/clrex.h index 586798f397fab..4cf163354fd92 100644 --- a/src/coreclr/vm/clrex.h +++ b/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); }; diff --git a/src/coreclr/vm/comutilnative.cpp b/src/coreclr/vm/comutilnative.cpp index a3c9d0a848cdf..73659df912c4c 100644 --- a/src/coreclr/vm/comutilnative.cpp +++ b/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,19 +123,15 @@ 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); @@ -107,91 +139,67 @@ FCIMPL3(VOID, ExceptionNative::GetStackTracesDeepCopy, Object* pExceptionObjectU // 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(); diff --git a/src/coreclr/vm/comutilnative.h b/src/coreclr/vm/comutilnative.h index 3e64207564c84..ea30a2c6a7101 100644 --- a/src/coreclr/vm/comutilnative.h +++ b/src/coreclr/vm/comutilnative.h @@ -42,9 +42,8 @@ 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 FCDECL2(VOID, GetStackTracesDeepCopy, Object* pExceptionObjectUnsafe, Object **pStackTraceUnsafe); + static FCDECL2(VOID, SaveStackTracesFromDeepCopy, Object* pExceptionObjectUnsafe, Object *pStackTraceUnsafe); #ifdef FEATURE_COMINTEROP // NOTE: caller cleans up any partially initialized BSTRs in pED diff --git a/src/coreclr/vm/corelib.h b/src/coreclr/vm/corelib.h index 17f74503bdc28..032de211d2298 100644 --- a/src/coreclr/vm/corelib.h +++ b/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) diff --git a/src/coreclr/vm/excep.cpp b/src/coreclr/vm/excep.cpp index cb1fe1e30a449..30b2e189592cf 100644 --- a/src/coreclr/vm/excep.cpp +++ b/src/coreclr/vm/excep.cpp @@ -2125,374 +2125,6 @@ void UnwindFrames( // No return value. #endif // !defined(FEATURE_EH_FUNCLETS) -void StackTraceInfo::SaveStackTrace(BOOL bAllowAllocMem, OBJECTHANDLE hThrowable, BOOL bReplaceStack, BOOL bSkipLastElement) -{ - CONTRACTL - { - NOTHROW; - GC_TRIGGERS; - MODE_COOPERATIVE; - } - CONTRACTL_END; - - // Do not save stacktrace to preallocated exception. These are shared. - if (CLRException::IsPreallocatedExceptionHandle(hThrowable)) - { - // Preallocated exceptions will never have this flag set. However, its possible - // that after this flag is set for a regular exception but before we throw, we have an async - // exception like a RudeThreadAbort, which will replace the exception - // containing the restored stack trace. - // - // In such a case, we should clear the flag as the throwable representing the - // preallocated exception will not have the restored (or any) stack trace. - PTR_ThreadExceptionState pCurTES = GetThread()->GetExceptionState(); - pCurTES->ResetRaisingForeignException(); - - return; - } - - LOG((LF_EH, LL_INFO1000, "StackTraceInfo::SaveStackTrace (%p), alloc = %d, replace = %d, skiplast = %d\n", this, bAllowAllocMem, bReplaceStack, bSkipLastElement)); - - // if have bSkipLastElement, must also keep the stack - _ASSERTE(! bSkipLastElement || ! bReplaceStack); - - bool fSuccess = false; - MethodTable* pMT = ObjectFromHandle(hThrowable)->GetMethodTable(); - - // Check if the flag indicating foreign exception raise has been setup or not, - // and then reset it so that subsequent processing of managed frames proceeds - // normally. - PTR_ThreadExceptionState pCurTES = GetThread()->GetExceptionState(); - BOOL fRaisingForeignException = pCurTES->IsRaisingForeignException(); - pCurTES->ResetRaisingForeignException(); - - if (bAllowAllocMem && m_dFrameCount != 0) - { - EX_TRY - { - // Only save stack trace info on exceptions - _ASSERTE(IsException(pMT)); // what is the pathway here? - if (!IsException(pMT)) - { - fSuccess = true; - } - else - { - // If the stack trace contains DynamicMethodDescs, we need to save the corrosponding - // System.Resolver objects in the Exception._dynamicMethods field. Failing to do that - // will cause an AV in the runtime when we try to visit those MethodDescs in the - // Exception._stackTrace field, because they have been recycled or destroyed. - unsigned iNumDynamics = 0; - - // How many DynamicMethodDescs do we need to keep alive? - for (unsigned iElement=0; iElement < m_dFrameCount; iElement++) - { - MethodDesc *pMethod = m_pStackTrace[iElement].pFunc; - _ASSERTE(pMethod); - - if (pMethod->IsLCGMethod()) - { - // Increment the number of new dynamic methods we have found - iNumDynamics++; - } - else - if (pMethod->GetMethodTable()->Collectible()) - { - iNumDynamics++; - } - } - - struct _gc - { - StackTraceArray stackTrace; - StackTraceArray stackTraceTemp; - PTRARRAYREF dynamicMethodsArrayTemp; - PTRARRAYREF dynamicMethodsArray; // Object array of Managed Resolvers - PTRARRAYREF pOrigDynamicArray; - - _gc() - : stackTrace() - , stackTraceTemp() - , dynamicMethodsArrayTemp(static_cast(NULL)) - , dynamicMethodsArray(static_cast(NULL)) - , pOrigDynamicArray(static_cast(NULL)) - {} - }; - - _gc gc; - GCPROTECT_BEGIN(gc); - - // If the flag indicating foreign exception raise has been setup, then check - // if the exception object has stacktrace or not. If we have an async non-preallocated - // exception after setting this flag but before we throw, then the new - // exception will not have any stack trace set and thus, we should behave as if - // the flag was not setup. - if (fRaisingForeignException) - { - // Get the reference to stack trace and reset our flag if applicable. - ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->GetStackTrace(gc.stackTraceTemp); - if (gc.stackTraceTemp.Size() == 0) - { - fRaisingForeignException = FALSE; - } - } - - // Replace stack (i.e. build a new stack trace) only if we are not raising a foreign exception. - // If we are, then we will continue to extend the existing stack trace. - if (bReplaceStack - && (!fRaisingForeignException) - ) - { - // Cleanup previous info - gc.stackTrace.Append(m_pStackTrace, m_pStackTrace + m_dFrameCount); - - if (iNumDynamics) - { - // Adjust the allocation size of the array, if required - if (iNumDynamics > m_cDynamicMethodItems) - { - S_UINT32 cNewSize = S_UINT32(2) * S_UINT32(iNumDynamics); - if (cNewSize.IsOverflow()) - { - // Overflow here implies we cannot allocate memory anymore - LOG((LF_EH, LL_INFO100, "StackTraceInfo::SaveStackTrace - Cannot calculate initial resolver array size due to overflow!\n")); - COMPlusThrowOM(); - } - - m_cDynamicMethodItems = cNewSize.Value(); - } - - gc.dynamicMethodsArray = (PTRARRAYREF)AllocateObjectArray(m_cDynamicMethodItems, g_pObjectClass); - LOG((LF_EH, LL_INFO100, "StackTraceInfo::SaveStackTrace - allocated dynamic array for first frame of size %lu\n", - m_cDynamicMethodItems)); - } - - m_dCurrentDynamicIndex = 0; - } - else - { - // Fetch the stacktrace and the dynamic method array - ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->GetStackTrace(gc.stackTrace, &gc.pOrigDynamicArray); - - 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(); - if (numCurrentFrames > 0) - { - // "numCurrentFrames" can be zero if the user created an EDI using - // an unthrown exception. - StackTraceElement & refLastElementFromForeignStackTrace = gc.stackTrace[numCurrentFrames - 1]; - refLastElementFromForeignStackTrace.flags |= STEF_LAST_FRAME_FROM_FOREIGN_STACK_TRACE; - } - } - - if (!bSkipLastElement) - gc.stackTrace.Append(m_pStackTrace, m_pStackTrace + m_dFrameCount); - - ////////////////////////////// - - unsigned cOrigDynamic = 0; // number of objects in the old array - if (gc.pOrigDynamicArray != NULL) - { - cOrigDynamic = gc.pOrigDynamicArray->GetNumComponents(); - } - else - { - // Since there is no dynamic method array, reset the corresponding state variables - m_dCurrentDynamicIndex = 0; - m_cDynamicMethodItems = 0; - } - - if ((gc.pOrigDynamicArray != NULL) - || (fRaisingForeignException) - ) - { - // Since we have just restored the dynamic method array as well, - // calculate the dynamic array index which would be the total - // number of dynamic methods present in the stack trace. - // - // In addition to the ForeignException scenario, we need to reset these - // values incase the exception object in question is being thrown by - // multiple threads in parallel and thus, could have potentially different - // dynamic method array contents/size as opposed to the current state of - // StackTraceInfo. - - unsigned iStackTraceElements = (unsigned)gc.stackTrace.Size(); - m_dCurrentDynamicIndex = 0; - for (unsigned iIndex = 0; iIndex < iStackTraceElements; iIndex++) - { - MethodDesc *pMethod = gc.stackTrace[iIndex].pFunc; - if (pMethod) - { - if ((pMethod->IsLCGMethod()) || (pMethod->GetMethodTable()->Collectible())) - { - // Increment the number of new dynamic methods we have found - m_dCurrentDynamicIndex++; - } - } - } - - // Total number of elements in the dynamic method array should also be - // reset based upon the restored array size. - m_cDynamicMethodItems = cOrigDynamic; - } - - // Make the dynamic Array field reference the original array we got from the - // Exception object. If, below, we have to add new entries, we will add it to the - // array if it is allocated, or else, we will allocate it before doing so. - gc.dynamicMethodsArray = gc.pOrigDynamicArray; - - // Create an object array if we have new dynamic method entries AND - // if we are at the (or went past) the current size limit - if (iNumDynamics > 0) - { - // Reallocate the array if we are at the (or went past) the current size limit - unsigned cTotalDynamicMethodCount = m_dCurrentDynamicIndex; - - S_UINT32 cNewSum = S_UINT32(cTotalDynamicMethodCount) + S_UINT32(iNumDynamics); - if (cNewSum.IsOverflow()) - { - // If the current size is already the UINT32 max size, then we - // cannot go further. Overflow here implies we cannot allocate memory anymore. - LOG((LF_EH, LL_INFO100, "StackTraceInfo::SaveStackTrace - Cannot calculate resolver array size due to overflow!\n")); - COMPlusThrowOM(); - } - - cTotalDynamicMethodCount = cNewSum.Value(); - - if (cTotalDynamicMethodCount > m_cDynamicMethodItems) - { - // Double the current limit of the array. - S_UINT32 cNewSize = S_UINT32(2) * S_UINT32(cTotalDynamicMethodCount); - if (cNewSize.IsOverflow()) - { - // Overflow here implies that we cannot allocate any more memory - LOG((LF_EH, LL_INFO100, "StackTraceInfo::SaveStackTrace - Cannot resize resolver array beyond max size due to overflow!\n")); - COMPlusThrowOM(); - } - - m_cDynamicMethodItems = cNewSize.Value(); - gc.dynamicMethodsArray = (PTRARRAYREF)AllocateObjectArray(m_cDynamicMethodItems, - g_pObjectClass); - - _ASSERTE(!(cOrigDynamic && !gc.pOrigDynamicArray)); - - LOG((LF_EH, LL_INFO100, "StackTraceInfo::SaveStackTrace - resized dynamic array to size %lu\n", - m_cDynamicMethodItems)); - - // Copy previous entries if there are any, and update iCurDynamic to point - // to the following index. - if (cOrigDynamic && (gc.pOrigDynamicArray != NULL)) - { - memmoveGCRefs(gc.dynamicMethodsArray->GetDataPtr(), - gc.pOrigDynamicArray->GetDataPtr(), - cOrigDynamic * sizeof(Object *)); - - // m_dCurrentDynamicIndex is already referring to the correct index - // at which the next resolver object will be saved - } - } - else - { - // We are adding objects to the existing array. - // - // We have new dynamic method entries for which - // resolver objects need to be saved. Ensure - // that we have the array to store them - if (gc.dynamicMethodsArray == NULL) - { - _ASSERTE(m_cDynamicMethodItems > 0); - - gc.dynamicMethodsArray = (PTRARRAYREF)AllocateObjectArray(m_cDynamicMethodItems, - g_pObjectClass); - m_dCurrentDynamicIndex = 0; - LOG((LF_EH, LL_INFO100, "StackTraceInfo::SaveStackTrace - allocated dynamic array of size %lu\n", - m_cDynamicMethodItems)); - } - else - { - // The array exists for storing resolver objects. - // Simply set the index at which the next resolver - // will be stored in it. - } - } - } - } - - // Update _dynamicMethods field - if (iNumDynamics) - { - // At this point, we should be having a valid array for storage - _ASSERTE(gc.dynamicMethodsArray != NULL); - - // Assert that we are in valid range of the array in which resolver objects will be saved. - // We subtract 1 below since storage will start from m_dCurrentDynamicIndex onwards and not - // from (m_dCurrentDynamicIndex + 1). - _ASSERTE((m_dCurrentDynamicIndex + iNumDynamics - 1) < gc.dynamicMethodsArray->GetNumComponents()); - - for (unsigned i=0; i < m_dFrameCount; i++) - { - MethodDesc *pMethod = m_pStackTrace[i].pFunc; - _ASSERTE(pMethod); - - if (pMethod->IsLCGMethod()) - { - // We need to append the corresponding System.Resolver for - // this DynamicMethodDesc to keep it alive. - DynamicMethodDesc *pDMD = (DynamicMethodDesc *) pMethod; - OBJECTREF pResolver = pDMD->GetLCGMethodResolver()->GetManagedResolver(); - - _ASSERTE(pResolver != NULL); - - // Store Resolver information in the array - gc.dynamicMethodsArray->SetAt(m_dCurrentDynamicIndex++, pResolver); - } - else - if (pMethod->GetMethodTable()->Collectible()) - { - OBJECTREF pLoaderAllocator = pMethod->GetMethodTable()->GetLoaderAllocator()->GetExposedObject(); - _ASSERTE(pLoaderAllocator != NULL); - gc.dynamicMethodsArray->SetAt (m_dCurrentDynamicIndex++, pLoaderAllocator); - } - } - } - - ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->SetStackTrace(gc.stackTrace.Get(), gc.dynamicMethodsArray); - - // Update _stackTraceString field. - ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->SetStackTraceString(NULL); - fSuccess = true; - - GCPROTECT_END(); // gc - } - } - EX_CATCH - { - } - EX_END_CATCH(SwallowAllExceptions) - } - - ClearStackTrace(); - - if (!fSuccess) - { - EX_TRY - { - _ASSERTE(IsException(pMT)); // what is the pathway here? - if (bReplaceStack && IsException(pMT)) - ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->ClearStackTraceForThrow(); - } - EX_CATCH - { - // Do nothing - } - EX_END_CATCH(SwallowAllExceptions); - } -} - // Copy a context record, being careful about whether or not the target // is large enough to support CONTEXT_EXTENDED_REGISTERS. // @@ -3226,196 +2858,326 @@ void COMPlusCooperativeTransitionHandler(Frame* pFrame) -void StackTraceInfo::Init() +#ifndef TARGET_UNIX // Watson is supported on Windows only +void SetupWatsonBucket(UINT_PTR currentIP, CrawlFrame* pCf) +{ + Thread *pThread = GetThread(); + + if (pThread && (currentIP != 0)) + { + // Setup the watson bucketing details for the initial throw + // callback only if we dont already have them. + ThreadExceptionState *pExState = pThread->GetExceptionState(); + if (!pExState->GetFlags()->GotWatsonBucketDetails()) + { + // Adjust the IP if necessary. + UINT_PTR adjustedIp = currentIP; + // This is a workaround copied from above. + if (!(pCf->HasFaulted() || pCf->IsIPadjusted()) && adjustedIp != 0) + { + adjustedIp -= 1; + } + + // Setup the bucketing details for the initial throw + SetupInitialThrowBucketDetails(adjustedIp); + } + } +} +#endif // !TARGET_UNIX + +// Ensure that there is space for one more element in the stack trace array. +void StackTraceInfo::EnsureStackTraceArray(StackTraceArray *pStackTrace, size_t neededSize) { CONTRACTL { - NOTHROW; - GC_NOTRIGGER; - MODE_ANY; - FORBID_FAULT; + GC_TRIGGERS; + THROWS; + PRECONDITION(CheckPointer(pStackTrace)); } CONTRACTL_END; - LOG((LF_EH, LL_INFO10000, "StackTraceInfo::Init (%p)\n", this)); + StackTraceArray newStackTrace; + GCPROTECT_BEGIN(newStackTrace); - m_pStackTrace = NULL; - m_cStackTrace = 0; - m_dFrameCount = 0; - m_cDynamicMethodItems = 0; - m_dCurrentDynamicIndex = 0; + size_t stackTraceCapacity = pStackTrace->Capacity(); + if (neededSize > stackTraceCapacity) + { + S_SIZE_T newCapacity = S_SIZE_T(stackTraceCapacity) * S_SIZE_T(2); + if (newCapacity.IsOverflow() || (neededSize > newCapacity.Value())) + { + newCapacity = S_SIZE_T(neededSize); + } + + stackTraceCapacity = newCapacity.Value(); + + // Allocate a new array with the needed size + newStackTrace.Allocate(stackTraceCapacity); + if (pStackTrace->Get() != NULL) + { + STRESS_LOG3(LF_EH, LL_INFO1000, "EnsureStackTraceArray copying from stackTrace=%p to stackTrace=%p, owner thread was %04x\n", OBJECTREFToObject(pStackTrace->Get()), OBJECTREFToObject(newStackTrace.Get()), pStackTrace->GetObjectThread()->GetOSThreadId()); + // Copy the original array to the new one + newStackTrace.CopyDataFrom(*pStackTrace); + // TODO: cleanup this - make the method have delta as the parameter instead + newStackTrace.SetSize(neededSize - 1); + } + else + { + STRESS_LOG1(LF_EH, LL_INFO1000, "EnsureStackTraceArray created a new clean array %p\n", OBJECTREFToObject(newStackTrace.Get())); + } + // Update the stack trace array + pStackTrace->Set(newStackTrace.Get()); + } + GCPROTECT_END(); } -void StackTraceInfo::FreeStackTrace() +// Ensure that there is space for the neededSize elements in the keepalive array. +void StackTraceInfo::EnsureKeepaliveArray(PTRARRAYREF *ppKeepaliveArray, size_t neededSize) { CONTRACTL { - NOTHROW; - GC_NOTRIGGER; - MODE_ANY; - FORBID_FAULT; + GC_TRIGGERS; + THROWS; + PRECONDITION(CheckPointer(ppKeepaliveArray)); } CONTRACTL_END; - if (m_pStackTrace) + PTRARRAYREF pNewKeepaliveArray = NULL; + GCPROTECT_BEGIN(pNewKeepaliveArray); + + size_t keepaliveArrayCapacity = (*ppKeepaliveArray != NULL) ? (*ppKeepaliveArray)->GetNumComponents() : 0; + if (neededSize > keepaliveArrayCapacity) { - delete [] m_pStackTrace; - m_pStackTrace = NULL; - m_cStackTrace = 0; - m_dFrameCount = 0; - m_cDynamicMethodItems = 0; - m_dCurrentDynamicIndex = 0; + S_SIZE_T newCapacity = S_SIZE_T(keepaliveArrayCapacity) * S_SIZE_T(2); + if (newCapacity.IsOverflow() || (neededSize > newCapacity.Value())) + { + newCapacity = S_SIZE_T(neededSize); + } + + keepaliveArrayCapacity = newCapacity.Value(); + + if (!FitsIn(keepaliveArrayCapacity)) + { + EX_THROW(EEMessageException, (kOverflowException, IDS_EE_ARRAY_DIMENSIONS_EXCEEDED)); + } + + // Allocate a new array with the needed size + pNewKeepaliveArray = (PTRARRAYREF)AllocateObjectArray(static_cast(keepaliveArrayCapacity), g_pObjectClass); + if ((*ppKeepaliveArray) != NULL) + { + memmoveGCRefs(pNewKeepaliveArray->GetDataPtr(), + (*ppKeepaliveArray)->GetDataPtr(), + neededSize * sizeof(Object *)); + } + // Update the keepalive array + *ppKeepaliveArray = pNewKeepaliveArray; } + + GCPROTECT_END(); } -BOOL StackTraceInfo::IsEmpty() +// Get a keepalive object for the given method. The keepalive object is either a +// Resolver object for DynamicMethodDesc or a LoaderAllocator object for methods in +// collectible assemblies. +// Returns NULL if the method code cannot be destroyed. +OBJECTREF StackTraceInfo::GetKeepaliveObject(MethodDesc* pMethod) { LIMITED_METHOD_CONTRACT; - return 0 == m_dFrameCount; -} + if (pMethod->IsLCGMethod()) + { + // We need to append the corresponding System.Resolver for + // this DynamicMethodDesc to keep it alive. + DynamicMethodDesc *pDMD = (DynamicMethodDesc *) pMethod; + OBJECTREF pResolver = pDMD->GetLCGMethodResolver()->GetManagedResolver(); -void StackTraceInfo::ClearStackTrace() -{ - LIMITED_METHOD_CONTRACT; + _ASSERTE(pResolver != NULL); + + // Store Resolver information in the array + return pResolver; + } + else if (pMethod->GetMethodTable()->Collectible()) + { + OBJECTREF pLoaderAllocator = pMethod->GetMethodTable()->GetLoaderAllocator()->GetExposedObject(); + _ASSERTE(pLoaderAllocator != NULL); + return pLoaderAllocator; + } - LOG((LF_EH, LL_INFO1000, "StackTraceInfo::ClearStackTrace (%p)\n", this)); - m_dFrameCount = 0; + return NULL; } -// allocate stack trace info. As each function is found in the stack crawl, it will be added -// to this list. If the list is too small, it is reallocated. -void StackTraceInfo::AllocateStackTrace() +// Get number of methods in the stack trace that can be collected. We need to store keepalive +// objects (Resolver / LoaderAllocator) for these methods. +int StackTraceInfo::GetKeepaliveItemsCount(StackTraceArray *pStackTrace) { - STATIC_CONTRACT_NOTHROW; - STATIC_CONTRACT_GC_NOTRIGGER; - STATIC_CONTRACT_MODE_ANY; - STATIC_CONTRACT_FORBID_FAULT; - - LOG((LF_EH, LL_INFO1000, "StackTraceInfo::AllocateStackTrace (%p)\n", this)); + LIMITED_METHOD_CONTRACT; - if (!m_pStackTrace) + int count = 0; + for (size_t i = 0; i < pStackTrace->Size(); i++) { -#ifdef _DEBUG - unsigned int allocSize = 2; // make small to exercise realloc -#else - unsigned int allocSize = 30; -#endif - - SCAN_IGNORE_FAULT; // A fault of new is okay here. The rest of the system is cool if we don't have enough - // memory to remember the stack as we run our first pass. - m_pStackTrace = new (nothrow) StackTraceElement[allocSize]; - - if (m_pStackTrace != NULL) + MethodDesc *pMethod = (*pStackTrace)[i].pFunc; + if (pMethod->IsLCGMethod() || pMethod->GetMethodTable()->Collectible()) { - // Remember how much we allocated. - m_cStackTrace = allocSize; - m_cDynamicMethodItems = allocSize; - } - else - { - m_cStackTrace = 0; - m_cDynamicMethodItems = 0; + count++; } } + + return count; } // +// Append stack frame to an exception stack trace. // Returns true if it appended the element, false otherwise. // -BOOL StackTraceInfo::AppendElement(BOOL bAllowAllocMem, UINT_PTR currentIP, UINT_PTR currentSP, MethodDesc* pFunc, CrawlFrame* pCf) +BOOL StackTraceInfo::AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, UINT_PTR currentSP, MethodDesc* pFunc, CrawlFrame* pCf) { CONTRACTL { GC_TRIGGERS; NOTHROW; + MODE_COOPERATIVE; } CONTRACTL_END - LOG((LF_EH, LL_INFO10000, "StackTraceInfo::AppendElement (%p), IP = %p, SP = %p, %s::%s\n", this, currentIP, currentSP, pFunc ? pFunc->m_pszDebugClassName : "", pFunc ? pFunc->m_pszDebugMethodName : "" )); BOOL bRetVal = FALSE; + Thread *pThread = GetThread(); + MethodTable* pMT = ObjectFromHandle(hThrowable)->GetMethodTable(); + _ASSERTE(IsException(pMT)); + + PTR_ThreadExceptionState pCurTES = pThread->GetExceptionState(); + // Check if the flag indicating foreign exception raise has been setup or not, + // and then reset it so that subsequent processing of managed frames proceeds + // normally. + BOOL fRaisingForeignException = pCurTES->IsRaisingForeignException(); + pCurTES->ResetRaisingForeignException(); + + LOG((LF_EH, LL_INFO10000, "StackTraceInfo::AppendElement (%p), IP = %p, SP = %p, %s::%s\n", this, currentIP, currentSP, pFunc ? pFunc->m_pszDebugClassName : "", pFunc ? pFunc->m_pszDebugMethodName : "" )); if (pFunc != NULL && pFunc->IsILStub()) return FALSE; - // Save this function in the stack trace array, which we only build on the first pass. We'll try to expand the - // stack trace array if we don't have enough room. Note that we only try to expand if we're allowed to allocate - // memory (bAllowAllocMem). - if (bAllowAllocMem && (m_dFrameCount >= m_cStackTrace)) + // Do not save stacktrace to preallocated exception. These are shared. + if (CLRException::IsPreallocatedExceptionHandle(hThrowable)) { - StackTraceElement* pTempElement = new (nothrow) StackTraceElement[m_cStackTrace*2]; + // Preallocated exceptions will never have this flag set. However, its possible + // that after this flag is set for a regular exception but before we throw, we have an async + // exception like a RudeThreadAbort, which will replace the exception + // containing the restored stack trace. - if (pTempElement != NULL) - { - memcpy(pTempElement, m_pStackTrace, m_cStackTrace * sizeof(StackTraceElement)); - delete [] m_pStackTrace; - m_pStackTrace = pTempElement; - m_cStackTrace *= 2; - } + return FALSE; } - // Add the function to the stack trace array if there's room. - if (m_dFrameCount < m_cStackTrace) + StackTraceElement stackTraceElem; + + stackTraceElem.pFunc = pFunc; + + stackTraceElem.ip = currentIP; + stackTraceElem.sp = currentSP; + + // When we are building stack trace as we encounter managed frames during exception dispatch, + // then none of those frames represent a stack trace from a foreign exception (as they represent + // the current exception). Hence, set the corresponding flag to FALSE. + stackTraceElem.flags = 0; + + // This is a workaround to fix the generation of stack traces from exception objects so that + // they point to the line that actually generated the exception instead of the line + // following. + if (pCf->IsIPadjusted()) + { + stackTraceElem.flags |= STEF_IP_ADJUSTED; + } + else if (!pCf->HasFaulted() && stackTraceElem.ip != 0) { - StackTraceElement* pStackTraceElem; + stackTraceElem.ip -= STACKWALK_CONTROLPC_ADJUST_OFFSET; + stackTraceElem.flags |= STEF_IP_ADJUSTED; + } - // If we get in here, we'd better have a stack trace array. - CONSISTENCY_CHECK(m_pStackTrace != NULL); +#ifndef TARGET_UNIX // Watson is supported on Windows only + SetupWatsonBucket(currentIP, pCf); +#endif // !TARGET_UNIX - pStackTraceElem = &(m_pStackTrace[m_dFrameCount]); + EX_TRY + { + struct + { + StackTraceArray stackTrace; + PTRARRAYREF pKeepaliveArray = NULL; // Object array of Managed Resolvers / Loader Allocators of methods that can be collected + OBJECTREF keepaliveObject = NULL; + } gc; + + GCPROTECT_BEGIN_THREAD(pThread, gc); - pStackTraceElem->pFunc = pFunc; + // 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. + bool wasCreatedByForeignThread = ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->GetStackTrace(gc.stackTrace, &gc.pKeepaliveArray); - pStackTraceElem->ip = currentIP; - pStackTraceElem->sp = currentSP; + // 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)); - // When we are building stack trace as we encounter managed frames during exception dispatch, - // then none of those frames represent a stack trace from a foreign exception (as they represent - // the current exception). Hence, set the corresponding flag to FALSE. - pStackTraceElem->flags = 0; + EnsureStackTraceArray(&gc.stackTrace, gc.stackTrace.Size() + 1); - // This is a workaround to fix the generation of stack traces from exception objects so that - // they point to the line that actually generated the exception instead of the line - // following. - if (pCf->IsIPadjusted()) + if (wasCreatedByForeignThread) { - pStackTraceElem->flags |= STEF_IP_ADJUSTED; + // If the stack trace was created by a foreign thread, we need to update the cached keepalive items count. + m_keepaliveItemsCount = GetKeepaliveItemsCount(&gc.stackTrace); } - else if (!pCf->HasFaulted() && pStackTraceElem->ip != 0) + else { - pStackTraceElem->ip -= STACKWALK_CONTROLPC_ADJUST_OFFSET; - pStackTraceElem->flags |= STEF_IP_ADJUSTED; + if (m_keepaliveItemsCount == -1) + { + // The m_keepaliveItemsCount was not initialized yet, so we need to calculate it. + m_keepaliveItemsCount = GetKeepaliveItemsCount(&gc.stackTrace); + } + _ASSERTE(m_keepaliveItemsCount == GetKeepaliveItemsCount(&gc.stackTrace)); } - ++m_dFrameCount; - bRetVal = TRUE; - } - -#ifndef TARGET_UNIX // Watson is supported on Windows only - Thread *pThread = GetThread(); + gc.keepaliveObject = GetKeepaliveObject(pFunc); + if (gc.keepaliveObject != NULL) + { + // The new frame to be added is a method that can be collected, so we need to update the keepalive items count. + m_keepaliveItemsCount++; + } - if (pThread && (currentIP != 0)) - { - // Setup the watson bucketing details for the initial throw - // callback only if we dont already have them. - ThreadExceptionState *pExState = pThread->GetExceptionState(); - if (!pExState->GetFlags()->GotWatsonBucketDetails()) + if (m_keepaliveItemsCount != 0) { - // Adjust the IP if necessary. - UINT_PTR adjustedIp = currentIP; - // This is a workaround copied from above. - if (!(pCf->HasFaulted() || pCf->IsIPadjusted()) && adjustedIp != 0) + // One extra slot is added for the stack trace array + EnsureKeepaliveArray(&gc.pKeepaliveArray, m_keepaliveItemsCount + 1); + if (gc.keepaliveObject != NULL) { - adjustedIp -= 1; + // Add the method to the keepalive array + gc.pKeepaliveArray->SetAt(m_keepaliveItemsCount, gc.keepaliveObject); } + } + else + { + // There are no methods that can be collected, so we don't need the keepalive array + gc.pKeepaliveArray = NULL; + } - // Setup the bucketing details for the initial throw - SetupInitialThrowBucketDetails(adjustedIp); + gc.stackTrace.Append(&stackTraceElem); + + if (gc.pKeepaliveArray != NULL) + { + _ASSERTE(m_keepaliveItemsCount > 0); + gc.pKeepaliveArray->SetAt(0, gc.stackTrace.Get()); + ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->SetStackTrace(dac_cast(gc.pKeepaliveArray)); + } + else + { + _ASSERTE(m_keepaliveItemsCount == 0); + ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->SetStackTrace(dac_cast(gc.stackTrace.Get())); } + + // Clear the _stackTraceString field as it no longer matches the stack trace + ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->SetStackTraceString(NULL); + + GCPROTECT_END(); // gc } -#endif // !TARGET_UNIX + EX_CATCH + { + } + EX_END_CATCH(SwallowAllExceptions) - return bRetVal; + return TRUE; } void UnwindFrameChain(Thread* pThread, LPVOID pvLimitSP) diff --git a/src/coreclr/vm/excep.h b/src/coreclr/vm/excep.h index 8e8ddacabe6b7..9b54fd7ce6936 100644 --- a/src/coreclr/vm/excep.h +++ b/src/coreclr/vm/excep.h @@ -65,14 +65,13 @@ struct ThrowCallbackType int dHandler; // the index of the handler whose filter returned catch indication BOOL bIsUnwind; // are we currently unwinding an exception BOOL bUnwindStack; // reset the stack before calling the handler? (Stack overflow only) - BOOL bAllowAllocMem; // are we allowed to allocate memory? BOOL bDontCatch; // can we catch this exception? BYTE *pStack; Frame * pTopFrame; Frame * pBottomFrame; MethodDesc * pProfilerNotify; // Context for profiler callbacks -- see COMPlusFrameHandler(). - BOOL bReplaceStack; // Used to pass info to SaveStackTrace call - BOOL bSkipLastElement;// Used to pass info to SaveStackTrace call + BOOL bIsNewException; + BOOL bSkipLastElement;// Used to skip calling StackTraceInfo::AppendElement #ifdef _DEBUG void * pCurrentExceptionRecord; void * pPrevExceptionRecord; @@ -86,13 +85,12 @@ struct ThrowCallbackType dHandler = 0; bIsUnwind = FALSE; bUnwindStack = FALSE; - bAllowAllocMem = TRUE; bDontCatch = FALSE; pStack = NULL; pTopFrame = (Frame *)-1; pBottomFrame = (Frame *)-1; pProfilerNotify = NULL; - bReplaceStack = FALSE; + bIsNewException = FALSE; bSkipLastElement = FALSE; #ifdef _DEBUG pCurrentExceptionRecord = 0; diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index 6033dd4ad02f2..71dd3507f43cc 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -234,9 +234,6 @@ void InitializeExceptionHandling() g_theTrackerAllocator.Init(); - // Initialize the lock used for synchronizing access to the stacktrace in the exception object - g_StackTraceArrayLock.Init(LOCK_TYPE_DEFAULT, TRUE); - g_isNewExceptionHandlingEnabled = Configuration::GetKnobBooleanValue(W("System.Runtime.LegacyExceptionHandling"), CLRConfig::EXTERNAL_LegacyExceptionHandling ) == 0; #ifdef TARGET_UNIX @@ -2436,7 +2433,7 @@ CLRUnwindStatus ExceptionTracker::ProcessExplicitFrame( // update our exception stacktrace // - BOOL bReplaceStack = FALSE; + BOOL bIsNewException = FALSE; BOOL bSkipLastElement = FALSE; if (STS_FirstRethrowFrame == STState) @@ -2446,7 +2443,7 @@ CLRUnwindStatus ExceptionTracker::ProcessExplicitFrame( else if (STS_NewException == STState) { - bReplaceStack = TRUE; + bIsNewException = TRUE; } // Normally, we need to notify the profiler in two cases: @@ -2466,18 +2463,20 @@ CLRUnwindStatus ExceptionTracker::ProcessExplicitFrame( // // notify profiler of new/rethrown exception // - if (bSkipLastElement || bReplaceStack) + if (bSkipLastElement || bIsNewException) { GCX_COOP(); EEToProfilerExceptionInterfaceWrapper::ExceptionThrown(pThread); - UpdatePerformanceMetrics(pcfThisFrame, bSkipLastElement, bReplaceStack); + UpdatePerformanceMetrics(pcfThisFrame, bSkipLastElement, bIsNewException); } // // Update stack trace // - m_StackTraceInfo.AppendElement(CanAllocateMemory(), 0, sf.SP, pMD, pcfThisFrame); - m_StackTraceInfo.SaveStackTrace(CanAllocateMemory(), m_hThrowable, bReplaceStack, bSkipLastElement); + if (!bSkipLastElement) + { + m_StackTraceInfo.AppendElement(m_hThrowable, NULL, sf.SP, pMD, pcfThisFrame); + } // // make callback to debugger and/or profiler @@ -2725,7 +2724,7 @@ CLRUnwindStatus ExceptionTracker::ProcessManagedCallFrame( BOOL fFoundHandler = FALSE; DWORD_PTR dwHandlerStartPC = 0; - BOOL bReplaceStack = FALSE; + BOOL bIsNewException = FALSE; BOOL bSkipLastElement = FALSE; bool fUnwindFinished = false; @@ -2736,17 +2735,17 @@ CLRUnwindStatus ExceptionTracker::ProcessManagedCallFrame( else if (STS_NewException == STState) { - bReplaceStack = TRUE; + bIsNewException = TRUE; } // We need to notify the profiler on the first pass in two cases: // 1) a brand new exception is thrown, and // 2) an exception is rethrown. - if (fIsFirstPass && (bSkipLastElement || bReplaceStack)) + if (fIsFirstPass && (bSkipLastElement || bIsNewException)) { GCX_COOP(); EEToProfilerExceptionInterfaceWrapper::ExceptionThrown(pThread); - UpdatePerformanceMetrics(pcfThisFrame, bSkipLastElement, bReplaceStack); + UpdatePerformanceMetrics(pcfThisFrame, bSkipLastElement, bIsNewException); } if (!fUnwindingToFindResumeFrame) @@ -2758,8 +2757,10 @@ CLRUnwindStatus ExceptionTracker::ProcessManagedCallFrame( { GCX_COOP(); - m_StackTraceInfo.AppendElement(CanAllocateMemory(), uControlPC, sf.SP, pMD, pcfThisFrame); - m_StackTraceInfo.SaveStackTrace(CanAllocateMemory(), m_hThrowable, bReplaceStack, bSkipLastElement); + if (!bSkipLastElement) + { + m_StackTraceInfo.AppendElement(m_hThrowable, uControlPC, sf.SP, pMD, pcfThisFrame); + } } // @@ -4053,11 +4054,6 @@ ExceptionTracker* ExceptionTracker::GetOrCreateTracker( CONSISTENCY_CHECK(NULL == pTracker->m_hThrowable); pThread->SafeSetThrowables(oThrowable); - - if (pTracker->CanAllocateMemory()) - { - pTracker->m_StackTraceInfo.AllocateStackTrace(); - } } INDEBUG(oThrowable = NULL); @@ -7501,7 +7497,6 @@ void ExceptionTracker::ReleaseResources() } m_hThrowable = NULL; } - m_StackTraceInfo.FreeStackTrace(); #ifndef TARGET_UNIX // Clear any held Watson Bucketing details @@ -7626,18 +7621,17 @@ extern "C" void QCALLTYPE AppendExceptionStackFrame(QCall::ObjectHandleOnStack e Frame* pFrame = pThread->GetFrame(); MarkInlinedCallFrameAsEHHelperCall(pFrame); - bool canAllocateMemory = !(exceptionObj.Get() == CLRException::GetPreallocatedOutOfMemoryException()) && - !(exceptionObj.Get() == CLRException::GetPreallocatedStackOverflowException()); - - MethodDesc *pMD = pExInfo->m_frameIter.m_crawl.GetFunction(); + if ((flags & RH_EH_FIRST_RETHROW_FRAME) == 0) + { + MethodDesc *pMD = pExInfo->m_frameIter.m_crawl.GetFunction(); #if _DEBUG - EECodeInfo codeInfo(ip); - _ASSERTE(codeInfo.IsValid()); - _ASSERTE(pMD == codeInfo.GetMethodDesc()); + EECodeInfo codeInfo(ip); + _ASSERTE(codeInfo.IsValid()); + _ASSERTE(pMD == codeInfo.GetMethodDesc()); #endif // _DEBUG - pExInfo->m_StackTraceInfo.AppendElement(canAllocateMemory, ip, sp, pMD, &pExInfo->m_frameIter.m_crawl); - pExInfo->m_StackTraceInfo.SaveStackTrace(canAllocateMemory, pExInfo->m_hThrowable, /*bReplaceStack*/FALSE, /*bSkipLastElement*/FALSE); + pExInfo->m_StackTraceInfo.AppendElement(pExInfo->m_hThrowable, ip, sp, pMD, &pExInfo->m_frameIter.m_crawl); + } } // Notify the debugger that we are on the first pass for a managed exception. @@ -8310,11 +8304,7 @@ extern "C" bool QCALLTYPE SfiInit(StackFrameIterator* pThis, CONTEXT* pStackwalk if (pMD != NULL) { GCX_COOP(); - bool canAllocateMemory = !(pExInfo->m_exception == CLRException::GetPreallocatedOutOfMemoryException()) && - !(pExInfo->m_exception == CLRException::GetPreallocatedStackOverflowException()); - - pExInfo->m_StackTraceInfo.AppendElement(canAllocateMemory, 0, GetRegdisplaySP(pExInfo->m_frameIter.m_crawl.GetRegisterSet()), pMD, &pExInfo->m_frameIter.m_crawl); - pExInfo->m_StackTraceInfo.SaveStackTrace(canAllocateMemory, pExInfo->m_hThrowable, /*bReplaceStack*/FALSE, /*bSkipLastElement*/FALSE); + pExInfo->m_StackTraceInfo.AppendElement(pExInfo->m_hThrowable, NULL, GetRegdisplaySP(pExInfo->m_frameIter.m_crawl.GetRegisterSet()), pMD, &pExInfo->m_frameIter.m_crawl); #if defined(DEBUGGING_SUPPORTED) if (NotifyDebuggerOfStub(pThread, pFrame)) @@ -8580,11 +8570,7 @@ extern "C" bool QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollideCla if (pMD != NULL) { GCX_COOP(); - bool canAllocateMemory = !(pTopExInfo->m_exception == CLRException::GetPreallocatedOutOfMemoryException()) && - !(pTopExInfo->m_exception == CLRException::GetPreallocatedStackOverflowException()); - - pTopExInfo->m_StackTraceInfo.AppendElement(canAllocateMemory, 0, GetRegdisplaySP(pTopExInfo->m_frameIter.m_crawl.GetRegisterSet()), pMD, &pTopExInfo->m_frameIter.m_crawl); - pTopExInfo->m_StackTraceInfo.SaveStackTrace(canAllocateMemory, pTopExInfo->m_hThrowable, /*bReplaceStack*/FALSE, /*bSkipLastElement*/FALSE); + pTopExInfo->m_StackTraceInfo.AppendElement(pTopExInfo->m_hThrowable, NULL, GetRegdisplaySP(pTopExInfo->m_frameIter.m_crawl.GetRegisterSet()), pMD, &pTopExInfo->m_frameIter.m_crawl); #if defined(DEBUGGING_SUPPORTED) if (NotifyDebuggerOfStub(pThread, pFrame)) diff --git a/src/coreclr/vm/exceptionhandling.h b/src/coreclr/vm/exceptionhandling.h index 3977587a76b9f..41a4055b68cb7 100644 --- a/src/coreclr/vm/exceptionhandling.h +++ b/src/coreclr/vm/exceptionhandling.h @@ -137,9 +137,6 @@ struct ExceptionTrackerBase m_fDeliveredFirstChanceNotification(FALSE), m_ExceptionCode((pExceptionRecord != PTR_NULL) ? pExceptionRecord->ExceptionCode : 0) { -#ifndef DACCESS_COMPILE - m_StackTraceInfo.Init(); -#endif // DACCESS_COMPILE #ifndef TARGET_UNIX // Init the WatsonBucketTracker m_WatsonBucketTracker.Init(); @@ -516,8 +513,6 @@ class ExceptionTracker : public ExceptionTrackerBase static bool IsFilterStartOffset(EE_ILEXCEPTION_CLAUSE* pEHClause, DWORD_PTR dwHandlerStartPC); - void SaveStackTrace(); - inline BOOL CanAllocateMemory() { CONTRACTL diff --git a/src/coreclr/vm/exinfo.cpp b/src/coreclr/vm/exinfo.cpp index 49287d3957e9d..3542a8b194763 100644 --- a/src/coreclr/vm/exinfo.cpp +++ b/src/coreclr/vm/exinfo.cpp @@ -211,8 +211,6 @@ void ExInfo::UnwindExInfo(VOID* limit) pPrevNestedInfo->GetWatsonBucketTracker()->ClearWatsonBucketDetails(); #endif // TARGET_UNIX - pPrevNestedInfo->m_StackTraceInfo.FreeStackTrace(); - #ifdef DEBUGGING_SUPPORTED if (g_pDebugInterface != NULL) { @@ -231,9 +229,6 @@ void ExInfo::UnwindExInfo(VOID* limit) pPrevNestedInfo = pPrev; } - // either clear the one we're about to copy over or the topmost one - m_StackTraceInfo.FreeStackTrace(); - if (pPrevNestedInfo) { // found nested handler info that is above the esp restore point so successfully caught nested @@ -329,7 +324,6 @@ ExInfo::ExInfo(Thread *pThread, EXCEPTION_RECORD *pExceptionRecord, CONTEXT *pEx m_pMDToReportFunctionLeave(NULL), m_lastReportedFunclet({0, 0, 0}) { - m_StackTraceInfo.AllocateStackTrace(); pThread->GetExceptionState()->m_pCurrentTracker = this; m_pInitialFrame = pThread->GetFrame(); if (exceptionKind == ExKind::HardwareFault) @@ -367,7 +361,6 @@ void ExInfo::ReleaseResources() } m_hThrowable = NULL; } - m_StackTraceInfo.FreeStackTrace(); #ifndef TARGET_UNIX // Clear any held Watson Bucketing details diff --git a/src/coreclr/vm/exinfo.h b/src/coreclr/vm/exinfo.h index fc9f134dfeb49..86a31166ac18d 100644 --- a/src/coreclr/vm/exinfo.h +++ b/src/coreclr/vm/exinfo.h @@ -169,6 +169,12 @@ enum RhEHClauseKind RH_EH_CLAUSE_UNUSED = 3, }; +enum RhEHFrameType +{ + RH_EH_FIRST_FRAME = 1, + RH_EH_FIRST_RETHROW_FRAME = 2, +}; + struct RhEHClause { RhEHClauseKind _clauseKind; diff --git a/src/coreclr/vm/exstate.cpp b/src/coreclr/vm/exstate.cpp index 1f245eedd802b..a285d85458e6c 100644 --- a/src/coreclr/vm/exstate.cpp +++ b/src/coreclr/vm/exstate.cpp @@ -76,24 +76,6 @@ Thread* ThreadExceptionState::GetMyThread() } -void ThreadExceptionState::FreeAllStackTraces() -{ - WRAPPER_NO_CONTRACT; - -#ifdef FEATURE_EH_FUNCLETS - ExceptionTrackerBase* pNode = m_pCurrentTracker; -#else // FEATURE_EH_FUNCLETS - ExInfo* pNode = &m_currentExInfo; -#endif // FEATURE_EH_FUNCLETS - - for ( ; - pNode != NULL; - pNode = pNode->m_pPrevNestedInfo) - { - pNode->m_StackTraceInfo.FreeStackTrace(); - } -} - OBJECTREF ThreadExceptionState::GetThrowable() { CONTRACTL diff --git a/src/coreclr/vm/exstate.h b/src/coreclr/vm/exstate.h index 3ef7cb95fed1d..2d60a52f2bb50 100644 --- a/src/coreclr/vm/exstate.h +++ b/src/coreclr/vm/exstate.h @@ -59,8 +59,6 @@ class ThreadExceptionState public: - void FreeAllStackTraces(); - #ifdef _DEBUG typedef enum { diff --git a/src/coreclr/vm/frames.h b/src/coreclr/vm/frames.h index 68706cd52304d..a56e04a8828e6 100644 --- a/src/coreclr/vm/frames.h +++ b/src/coreclr/vm/frames.h @@ -3388,6 +3388,7 @@ class FrameWithCookie #else // #ifndef DACCESS_COMPILE #define GCPROTECT_BEGIN(ObjRefStruct) +#define GCPROTECT_BEGIN_THREAD(pThread, ObjRefStruct) #define GCPROTECT_ARRAY_BEGIN(ObjRefArray,cnt) #define GCPROTECT_BEGININTERIOR(ObjRefStruct) #define GCPROTECT_END() diff --git a/src/coreclr/vm/i386/excepx86.cpp b/src/coreclr/vm/i386/excepx86.cpp index 13590fb23708f..7cc6ebc4c8f0b 100644 --- a/src/coreclr/vm/i386/excepx86.cpp +++ b/src/coreclr/vm/i386/excepx86.cpp @@ -1045,19 +1045,6 @@ CPFH_RealFirstPassHandler( // ExceptionContinueSearch, etc. EEPOLICY_HANDLE_FATAL_ERROR(exceptionCode); } - // If we're out of memory, then we figure there's probably not memory to maintain a stack trace, so we skip it. - // If we've got a stack overflow, then we figure the stack will be so huge as to make tracking the stack trace - // impracticle, so we skip it. - if ((throwable == CLRException::GetPreallocatedOutOfMemoryException()) || - (throwable == CLRException::GetPreallocatedStackOverflowException())) - { - tct.bAllowAllocMem = FALSE; - } - else - { - pExInfo->m_StackTraceInfo.AllocateStackTrace(); - } - GCPROTECT_END(); } @@ -1098,7 +1085,7 @@ CPFH_RealFirstPassHandler( // ExceptionContinueSearch, etc. LOG((LF_EH, LL_INFO100, "CPFH_RealFirstPassHandler: looking for handler bottom %x, top %x\n", tct.pBottomFrame, tct.pTopFrame)); - tct.bReplaceStack = pExInfo->m_pBottomMostHandler == pEstablisherFrame && !bRethrownException; + tct.bIsNewException = pExInfo->m_pBottomMostHandler == pEstablisherFrame && !bRethrownException; tct.bSkipLastElement = bRethrownException && bNestedException; found = LookForHandler(&exceptionPointers, pThread, @@ -1225,9 +1212,6 @@ void InitializeExceptionHandling() WRAPPER_NO_CONTRACT; CLRAddVectoredHandlers(); - - // Initialize the lock used for synchronizing access to the stacktrace in the exception object - g_StackTraceArrayLock.Init(LOCK_TYPE_DEFAULT, TRUE); } //****************************************************************************** @@ -2211,27 +2195,25 @@ StackWalkAction COMPlusThrowCallback( // SWA value if (!pFunc->IsILStub()) { - // Append the current frame to the stack trace and save the save trace to the managed Exception object. - pExInfo->m_StackTraceInfo.AppendElement(pData->bAllowAllocMem, currentIP, currentSP, pFunc, pCf); - - pExInfo->m_StackTraceInfo.SaveStackTrace(pData->bAllowAllocMem, - pThread->GetThrowableAsHandle(), - pData->bReplaceStack, - pData->bSkipLastElement); + if (!pData->bSkipLastElement) + { + // Append the current frame to the stack trace and save the save trace to the managed Exception object. + pExInfo->m_StackTraceInfo.AppendElement(pThread->GetThrowableAsHandle(), currentIP, currentSP, pFunc, pCf); + } } else { - LOG((LF_EH, LL_INFO1000, "COMPlusThrowCallback: Skipping AppendElement/SaveStackTrace for IL stub MD %p\n", pFunc)); + LOG((LF_EH, LL_INFO1000, "COMPlusThrowCallback: Skipping AppendElement for IL stub MD %p\n", pFunc)); } // Fire an exception thrown ETW event when an exception occurs - ETW::ExceptionLog::ExceptionThrown(pCf, pData->bSkipLastElement, pData->bReplaceStack); + ETW::ExceptionLog::ExceptionThrown(pCf, pData->bSkipLastElement, pData->bIsNewException); // Reset the flags. These flags are set only once before each stack walk done by LookForHandler(), and // they apply only to the first frame we append to the stack trace. Subsequent frames are always appended. - if (pData->bReplaceStack) + if (pData->bIsNewException) { - pData->bReplaceStack = FALSE; + pData->bIsNewException = FALSE; } if (pData->bSkipLastElement) { @@ -2985,7 +2967,6 @@ void ResumeAtJitEH(CrawlFrame* pCf, LOG((LF_EH, LL_INFO1000, "ResumeAtJitEH: popping nested ExInfo at 0x%p\n", pPrevNestedInfo->m_StackAddress)); pPrevNestedInfo->DestroyExceptionHandle(); - pPrevNestedInfo->m_StackTraceInfo.FreeStackTrace(); #ifdef DEBUGGING_SUPPORTED if (g_pDebugInterface != NULL) @@ -3246,7 +3227,6 @@ EXCEPTION_HANDLER_IMPL(COMPlusNestedExceptionHandler) LOG((LF_EH, LL_INFO100, "COMPlusNestedExceptionHandler: PopExInfo(): popping nested ExInfo at 0x%p\n", pPrevNestedInfo)); pPrevNestedInfo->DestroyExceptionHandle(); - pPrevNestedInfo->m_StackTraceInfo.FreeStackTrace(); #ifdef DEBUGGING_SUPPORTED if (g_pDebugInterface != NULL) diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index cf41103441844..a4c22a279adad 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1426,7 +1426,7 @@ void __fastcall ZeroMemoryInGCHeap(void* mem, size_t size) *memBytes++ = 0; } -void StackTraceArray::Append(StackTraceElement const * begin, StackTraceElement const * end) +void StackTraceArray::Append(StackTraceElement const * elem) { CONTRACTL { @@ -1437,12 +1437,12 @@ void StackTraceArray::Append(StackTraceElement const * begin, StackTraceElement } CONTRACTL_END; - // ensure that only one thread can write to the array - EnsureThreadAffinity(); + // Only the thread that has created the array can append to it + assert(GetObjectThread() == GetThreadNULLOk()); - size_t newsize = Size() + (end - begin); - Grow(newsize); - memcpyNoGCRefs(GetData() + Size(), begin, (end - begin) * sizeof(StackTraceElement)); + size_t newsize = Size() + 1; + assert(newsize <= Capacity()); + memcpyNoGCRefs(GetData() + Size(), elem, sizeof(StackTraceElement)); MemoryBarrier(); // prevent the newsize from being reordered with the array copy SetSize(newsize); @@ -1473,62 +1473,38 @@ void StackTraceArray::CheckState() const assert(p[i].pFunc != NULL); } -void StackTraceArray::Grow(size_t grow_size) +void StackTraceArray::Allocate(size_t size) { CONTRACTL { THROWS; GC_TRIGGERS; MODE_COOPERATIVE; - INJECT_FAULT(ThrowOutOfMemory();); PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)this)); } CONTRACTL_END; - size_t raw_size = grow_size * sizeof(StackTraceElement) + sizeof(ArrayHeader); + size_t raw_size = size * sizeof(StackTraceElement) + sizeof(ArrayHeader); - if (!m_array) - { - SetArray(I1ARRAYREF(AllocatePrimitiveArray(ELEMENT_TYPE_I1, static_cast(raw_size)))); - SetSize(0); - SetObjectThread(); - } - else + if (!FitsIn(raw_size)) { - if (Capacity() >= raw_size) - return; - - // allocate a new array, copy the data - size_t new_capacity = Max(Capacity() * 2, raw_size); - - _ASSERTE(new_capacity >= grow_size * sizeof(StackTraceElement) + sizeof(ArrayHeader)); - - I1ARRAYREF newarr = (I1ARRAYREF) AllocatePrimitiveArray(ELEMENT_TYPE_I1, static_cast(new_capacity)); - memcpyNoGCRefs(newarr->GetDirectPointerToNonObjectElements(), - GetRaw(), - Size() * sizeof(StackTraceElement) + sizeof(ArrayHeader)); - - SetArray(newarr); + EX_THROW(EEMessageException, (kOverflowException, IDS_EE_ARRAY_DIMENSIONS_EXCEEDED)); } + + SetArray(I1ARRAYREF(AllocatePrimitiveArray(ELEMENT_TYPE_I1, static_cast(raw_size)))); + SetSize(0); + SetObjectThread(); } -void StackTraceArray::EnsureThreadAffinity() +size_t StackTraceArray::Capacity() const { WRAPPER_NO_CONTRACT; - if (!m_array) - return; - - if (GetObjectThread() != GetThreadNULLOk()) { - // object is being changed by a thread different from the one which created it - // make a copy of the array to prevent a race condition when two different threads try to change it - StackTraceArray copy; - GCPROTECT_BEGIN(copy); - copy.CopyFrom(*this); - this->Swap(copy); - GCPROTECT_END(); + return 0; } + + return (m_array->GetNumComponents() - sizeof(ArrayHeader)) / sizeof(StackTraceElement); } // Deep copies the stack trace array @@ -1545,7 +1521,23 @@ void StackTraceArray::CopyFrom(StackTraceArray const & src) } CONTRACTL_END; - m_array = (I1ARRAYREF) AllocatePrimitiveArray(ELEMENT_TYPE_I1, static_cast(src.Capacity())); + m_array = (I1ARRAYREF) AllocatePrimitiveArray(ELEMENT_TYPE_I1, static_cast(src.Get()->GetNumComponents())); + + CopyDataFrom(src); +} + +void StackTraceArray::CopyDataFrom(StackTraceArray const & src) +{ + CONTRACTL + { + THROWS; + GC_TRIGGERS; + MODE_COOPERATIVE; + INJECT_FAULT(ThrowOutOfMemory();); + PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)this)); + PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)&src)); + } + CONTRACTL_END; Volatile size = src.Size(); memcpyNoGCRefs(GetRaw(), src.GetRaw(), size * sizeof(StackTraceElement) + sizeof(ArrayHeader)); @@ -1886,10 +1878,11 @@ StackTraceElement & StackTraceArray::operator[](size_t index) } #if !defined(DACCESS_COMPILE) -// Define the lock used to access stacktrace from an exception object -SpinLock g_StackTraceArrayLock; -void ExceptionObject::SetStackTrace(I1ARRAYREF stackTrace, PTRARRAYREF dynamicMethodArray) +// If there are any dynamic methods, the stack trace object is the dynamic methods array with its first +// slot set to the stack trace I1Array. +// Otherwise the stack trace object is directly the stacktrace I1Array +void ExceptionObject::SetStackTrace(OBJECTREF stackTrace) { CONTRACTL { @@ -1906,40 +1899,119 @@ void ExceptionObject::SetStackTrace(I1ARRAYREF stackTrace, PTRARRAYREF dynamicMe } #endif - SpinLock::AcquireLock(&g_StackTraceArrayLock); - SetObjectReference((OBJECTREF*)&_stackTrace, (OBJECTREF)stackTrace); - SetObjectReference((OBJECTREF*)&_dynamicMethods, (OBJECTREF)dynamicMethodArray); - - SpinLock::ReleaseLock(&g_StackTraceArrayLock); - } #endif // !defined(DACCESS_COMPILE) -void ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outDynamicMethodArray /*= NULL*/) const +bool ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outKeepaliveArray /*= NULL*/) const { CONTRACTL { - GC_NOTRIGGER; - NOTHROW; + GC_TRIGGERS; + THROWS; MODE_COOPERATIVE; } CONTRACTL_END; -#if !defined(DACCESS_COMPILE) - SpinLock::AcquireLock(&g_StackTraceArrayLock); -#endif // !defined(DACCESS_COMPILE) + return ExceptionObject::GetStackTraceParts(_stackTrace, stackTrace, outKeepaliveArray); +} - StackTraceArray temp(_stackTrace); - stackTrace.Swap(temp); +// 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. +/* static */ +bool ExceptionObject::GetStackTraceParts(OBJECTREF stackTraceObj, StackTraceArray & stackTrace, PTRARRAYREF * outKeepaliveArray /*= NULL*/) +{ + CONTRACTL + { + GC_TRIGGERS; + THROWS; + MODE_COOPERATIVE; + } + CONTRACTL_END; - if (outDynamicMethodArray != NULL) + struct { - *outDynamicMethodArray = _dynamicMethods; + StackTraceArray newStackTrace; + PTRARRAYREF keepaliveArray = NULL; + PTRARRAYREF newKeepaliveArray = NULL; + PTR_PTRArray combinedArray = NULL; + } gc; + + bool wasCreatedByForeignThread = false; + + GCPROTECT_BEGIN(gc); + + // Extract the stack trace and keepalive arrays from the stack trace object. + if ((stackTraceObj != NULL) && ((dac_cast(OBJECTREFToObject(stackTraceObj)))->GetArrayElementType() != ELEMENT_TYPE_I1)) + { + // The stack trace object is the dynamic methods array with its first slot set to the stack trace I1Array. + gc.combinedArray = dac_cast(OBJECTREFToObject(stackTraceObj)); + stackTrace.Set(dac_cast(gc.combinedArray->GetAt(0))); + gc.keepaliveArray = dac_cast(ObjectToOBJECTREF(gc.combinedArray)); + } + else + { + stackTrace.Set(dac_cast(stackTraceObj)); } -#if !defined(DACCESS_COMPILE) - SpinLock::ReleaseLock(&g_StackTraceArrayLock); -#endif // !defined(DACCESS_COMPILE) +#ifndef DACCESS_COMPILE + Thread *pThread = GetThread(); + + if ((stackTrace.Get() != NULL) && (stackTrace.GetObjectThread() != pThread)) + { + // When the stack trace was created by other thread than the current one, we create a copy of both the stack trace and the keepalive arrays to make sure + // they are not changing while the caller is accessing them. + wasCreatedByForeignThread = true; + gc.newStackTrace.Allocate(stackTrace.Capacity()); + gc.newStackTrace.CopyDataFrom(stackTrace); + stackTrace.Set(gc.newStackTrace.Get()); + + int keepaliveArrayCapacity = (gc.keepaliveArray == NULL) ? 0 : gc.keepaliveArray->GetNumComponents(); + + int j = 0; + size_t count = stackTrace.Size(); + for (size_t i = 0; i < count; i++) + { + MethodDesc *pMethod = stackTrace[i].pFunc; + if (pMethod->IsLCGMethod() || pMethod->GetMethodTable()->Collectible()) + { + OBJECTREF keepaliveObject = NULL; + if ((j + 1) < keepaliveArrayCapacity) + { + keepaliveObject = gc.keepaliveArray->GetAt(j + 1); + } + if (keepaliveObject == NULL) + { + // Trim the stack trace at a point where a dynamic or collectible method is found without a corresponding keepalive object. + stackTrace.SetSize(i); + break; + } + j++; + } + } + + if (keepaliveArrayCapacity != 0) + { + gc.newKeepaliveArray = (PTRARRAYREF)AllocateObjectArray(static_cast(keepaliveArrayCapacity), g_pObjectClass); + _ASSERTE(gc.keepaliveArray != NULL); + memmoveGCRefs(gc.newKeepaliveArray->GetDataPtr() + 1, + gc.keepaliveArray->GetDataPtr() + 1, + (keepaliveArrayCapacity - 1) * sizeof(Object *)); + gc.newKeepaliveArray->SetAt(0, stackTrace.Get()); + gc.keepaliveArray = gc.newKeepaliveArray; + } + else + { + gc.keepaliveArray = NULL; + } + } +#endif // DACCESS_COMPILE + + if (outKeepaliveArray != NULL) + { + *outKeepaliveArray = gc.keepaliveArray; + } + GCPROTECT_END(); + return wasCreatedByForeignThread; } diff --git a/src/coreclr/vm/object.h b/src/coreclr/vm/object.h index c486177ee3187..0fe725f6e018d 100644 --- a/src/coreclr/vm/object.h +++ b/src/coreclr/vm/object.h @@ -1932,7 +1932,7 @@ class StackTraceArray LIMITED_METHOD_CONTRACT; } - void Swap(StackTraceArray & rhs) + void Set(I1ARRAYREF array) { CONTRACTL { @@ -1942,9 +1942,7 @@ class StackTraceArray } CONTRACTL_END; SUPPORTS_DAC; - I1ARRAYREF t = m_array; - m_array = rhs.m_array; - rhs.m_array = t; + m_array = array; } size_t Size() const @@ -1959,7 +1957,9 @@ class StackTraceArray StackTraceElement const & operator[](size_t index) const; StackTraceElement & operator[](size_t index); - void Append(StackTraceElement const * begin, StackTraceElement const * end); + size_t Capacity() const; + void Append(StackTraceElement const * element); + void Allocate(size_t size); I1ARRAYREF Get() const { @@ -1970,27 +1970,12 @@ class StackTraceArray // Deep copies the array void CopyFrom(StackTraceArray const & src); -private: - StackTraceArray(StackTraceArray const & rhs) = delete; - - StackTraceArray & operator=(StackTraceArray const & rhs) = delete; + void CopyDataFrom(StackTraceArray const & src); - void Grow(size_t size); - void EnsureThreadAffinity(); - void CheckState() const; - - size_t Capacity() const - { - WRAPPER_NO_CONTRACT; - assert(!!m_array); - - return m_array->GetNumComponents(); - } - - size_t GetSize() const + Thread * GetObjectThread() const { WRAPPER_NO_CONTRACT; - return GetHeader()->m_size; + return GetHeader()->m_thread; } void SetSize(size_t size) @@ -1999,10 +1984,17 @@ class StackTraceArray GetHeader()->m_size = size; } - Thread * GetObjectThread() const +private: + StackTraceArray(StackTraceArray const & rhs) = delete; + + StackTraceArray & operator=(StackTraceArray const & rhs) = delete; + + void CheckState() const; + + size_t GetSize() const { WRAPPER_NO_CONTRACT; - return GetHeader()->m_thread; + return GetHeader()->m_size; } void SetObjectThread() @@ -2138,11 +2130,6 @@ typedef PTR_LoaderAllocatorObject LOADERALLOCATORREF; #endif // FEATURE_COLLECTIBLE_TYPES -#if !defined(DACCESS_COMPILE) -// Define the lock used to access stacktrace from an exception object -EXTERN_C SpinLock g_StackTraceArrayLock; -#endif // !defined(DACCESS_COMPILE) - // This class corresponds to Exception on the managed side. typedef DPTR(class ExceptionObject) PTR_ExceptionObject; #include "pshpack4.h" @@ -2187,11 +2174,13 @@ class ExceptionObject : public Object return _xptrs; } - void SetStackTrace(I1ARRAYREF stackTrace, PTRARRAYREF dynamicMethodArray); + void SetStackTrace(OBJECTREF stackTrace); + + bool GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outKeepaliveArray = NULL) const; - void GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outDynamicMethodArray = NULL) const; + static bool GetStackTraceParts(OBJECTREF stackTraceObj, StackTraceArray & stackTrace, PTRARRAYREF * outKeepaliveArray); - I1ARRAYREF GetStackTraceArrayObject() const + OBJECTREF GetStackTraceArrayObject() const { LIMITED_METHOD_DAC_CONTRACT; return _stackTrace; @@ -2345,11 +2334,10 @@ class ExceptionObject : public Object OBJECTREF _data; OBJECTREF _innerException; STRINGREF _helpURL; - I1ARRAYREF _stackTrace; + OBJECTREF _stackTrace; U1ARRAYREF _watsonBuckets; STRINGREF _stackTraceString; //Needed for serialization. STRINGREF _remoteStackTraceString; - PTRARRAYREF _dynamicMethods; STRINGREF _source; // Mainly used by VB. UINT_PTR _ipForWatsonBuckets; // Contains the IP of exception for watson bucketing diff --git a/src/coreclr/vm/threads.cpp b/src/coreclr/vm/threads.cpp index 7411d62a285f8..3598882fa4c11 100644 --- a/src/coreclr/vm/threads.cpp +++ b/src/coreclr/vm/threads.cpp @@ -2392,7 +2392,6 @@ int Thread::DecExternalCount(BOOL holdingLock) if (!HasValidThreadHandle()) { SelfDelete = this == pCurThread; - m_ExceptionState.FreeAllStackTraces(); if (SelfDelete) { SetThread(NULL); } diff --git a/src/tests/baseservices/exceptions/exceptionstacktrace/exceptionstacktrace.cs b/src/tests/baseservices/exceptions/exceptionstacktrace/exceptionstacktrace.cs new file mode 100644 index 0000000000000..e39f343335387 --- /dev/null +++ b/src/tests/baseservices/exceptions/exceptionstacktrace/exceptionstacktrace.cs @@ -0,0 +1,251 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Diagnostics; +using System.Reflection; +using System.Reflection.Emit; +using System.Runtime.CompilerServices; +using System.Threading; +using Xunit; + +public static class Program +{ + static MethodInfo throwHelper = typeof(Program).GetMethod("ThrowHelper"); + + static Exception theException = new Exception("My exception"); + static int recursion; + + private static void VerifyStackTrace(Exception e, string[] templateStackTrace) + { + string[] stackTrace = e.StackTrace.Split(Environment.NewLine, StringSplitOptions.RemoveEmptyEntries); + Assert.Equal(templateStackTrace.Length, stackTrace.Length); + + for (int i = 0; i < stackTrace.Length; i++) + { + Assert.Contains(templateStackTrace[i], stackTrace[i]); + } + } + + [Fact] + public static void DynamicMethodsWithRethrow() + { + try + { + ThrowHelper(); + } + catch (Exception e) + { + Console.WriteLine(e.ToString()); + + VerifyStackTrace(e, new string[] + { + "Program.ThrowHelper()", + "MyMethod4()", + "Program.AppendDynamicFrame()", + "Program.ThrowHelper()", + "MyMethod3()", + "Program.AppendDynamicFrame()", + "Program.ThrowHelper()", + "MyMethod2()", + "Program.AppendDynamicFrame()", + "Program.ThrowHelper()", + "MyMethod1()", + "Program.AppendDynamicFrame()", + "Program.ThrowHelper()", + "MyMethod0()", + "Program.AppendDynamicFrame()", + "Program.ThrowHelper()", + "Program.DynamicMethodsWithRethrow()" + }); + } + } + + [Fact] + public static void ThrowExistingExceptionAgain() + { + try + { + F1(); + } + catch (Exception e) + { + Console.WriteLine(e.ToString()); + VerifyStackTrace(e, new string[] + { + "Program.E1()", + "Program.F1()", + "Program.ThrowExistingExceptionAgain()" + }); + } + } + + [Fact] + public static void RethrowViaThrow() + { + try + { + F(); + } + catch (Exception e) + { + Console.WriteLine(e.ToString()); + VerifyStackTrace(e, new string[] + { + "Program.A()", + "Program.B()", + "Program.C()", + "Program.D()", + "Program.E()", + "Program.F()", + "Program.RethrowViaThrow()" + }); + } + } + + [Fact] + public static void RethrowViaThrowException() + { + try + { + F2(); + } + catch (Exception e) + { + Console.WriteLine(e.ToString()); + VerifyStackTrace(e, new string[] + { + "Program.D2()", + "Program.E2()", + "Program.F2()", + "Program.RethrowViaThrowException()" + }); + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void A() + { + throw new Exception("A"); + } + + + [MethodImpl(MethodImplOptions.NoInlining)] + static void B() + { + A(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void C() + { + B(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void D() + { + try + { + C(); + } + catch (Exception e) + { + throw; + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void E() + { + D(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void F() + { + E(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void D2() + { + try + { + C(); + } + catch (Exception e) + { + throw e; + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void E2() + { + D2(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void F2() + { + E2(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void D1(out Exception e) + { + e = null; + try + { + C(); + } + catch (Exception e2) + { + e = e2; + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void E1() + { + D1(out Exception e); + throw e; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void F1() + { + E1(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static void ThrowHelper() + { + if (recursion++ < 5) + AppendDynamicFrame(); + + throw theException; + } + + static int counter; + + static void AppendDynamicFrame() + { + var dm = new DynamicMethod($"MyMethod{counter++}", null, null); + var il = dm.GetILGenerator(); + il.Emit(OpCodes.Call, throwHelper); + il.Emit(OpCodes.Call, throwHelper); + il.Emit(OpCodes.Ret); + + var d = dm.CreateDelegate(); + try + { + d(); + } + catch (Exception e) + { + throw; + } + } +} diff --git a/src/tests/baseservices/exceptions/exceptionstacktrace/exceptionstacktrace.csproj b/src/tests/baseservices/exceptions/exceptionstacktrace/exceptionstacktrace.csproj new file mode 100644 index 0000000000000..e3323b349c398 --- /dev/null +++ b/src/tests/baseservices/exceptions/exceptionstacktrace/exceptionstacktrace.csproj @@ -0,0 +1,8 @@ + + + 1 + + + + + From 96da965c2b7a9477245d4265301b0e62b3462bad Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Wed, 5 Jun 2024 17:33:12 +0200 Subject: [PATCH 02/15] Fix MUSL build --- src/coreclr/vm/exceptionhandling.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index 71dd3507f43cc..d02653728a8b3 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -2475,7 +2475,7 @@ CLRUnwindStatus ExceptionTracker::ProcessExplicitFrame( // if (!bSkipLastElement) { - m_StackTraceInfo.AppendElement(m_hThrowable, NULL, sf.SP, pMD, pcfThisFrame); + m_StackTraceInfo.AppendElement(m_hThrowable, 0, sf.SP, pMD, pcfThisFrame); } // @@ -8304,7 +8304,7 @@ extern "C" bool QCALLTYPE SfiInit(StackFrameIterator* pThis, CONTEXT* pStackwalk if (pMD != NULL) { GCX_COOP(); - pExInfo->m_StackTraceInfo.AppendElement(pExInfo->m_hThrowable, NULL, GetRegdisplaySP(pExInfo->m_frameIter.m_crawl.GetRegisterSet()), pMD, &pExInfo->m_frameIter.m_crawl); + pExInfo->m_StackTraceInfo.AppendElement(pExInfo->m_hThrowable, 0, GetRegdisplaySP(pExInfo->m_frameIter.m_crawl.GetRegisterSet()), pMD, &pExInfo->m_frameIter.m_crawl); #if defined(DEBUGGING_SUPPORTED) if (NotifyDebuggerOfStub(pThread, pFrame)) @@ -8570,7 +8570,7 @@ extern "C" bool QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollideCla if (pMD != NULL) { GCX_COOP(); - pTopExInfo->m_StackTraceInfo.AppendElement(pTopExInfo->m_hThrowable, NULL, GetRegdisplaySP(pTopExInfo->m_frameIter.m_crawl.GetRegisterSet()), pMD, &pTopExInfo->m_frameIter.m_crawl); + pTopExInfo->m_StackTraceInfo.AppendElement(pTopExInfo->m_hThrowable, 0, GetRegdisplaySP(pTopExInfo->m_frameIter.m_crawl.GetRegisterSet()), pMD, &pTopExInfo->m_frameIter.m_crawl); #if defined(DEBUGGING_SUPPORTED) if (NotifyDebuggerOfStub(pThread, pFrame)) From fb8cc6b890eee4acf244f329a05d2cece4ad14ea Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Wed, 5 Jun 2024 17:34:41 +0200 Subject: [PATCH 03/15] Fix x86 build --- src/coreclr/vm/exinfo.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/coreclr/vm/exinfo.cpp b/src/coreclr/vm/exinfo.cpp index 3542a8b194763..c0c7a9c0c1268 100644 --- a/src/coreclr/vm/exinfo.cpp +++ b/src/coreclr/vm/exinfo.cpp @@ -98,7 +98,6 @@ void ExInfo::Init() CONTRACTL_END; m_ExceptionFlags.Init(); - m_StackTraceInfo.Init(); m_DebuggerExState.Init(); m_pSearchBoundary = NULL; From 2d6378165474393bed65134602cf04fed6b9651a Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 6 Jun 2024 18:44:32 +0200 Subject: [PATCH 04/15] Fix several issues * Missing calls to IsOverflow at few places * Added a flag on StackTraceElement to indicate that the element needs a keepalive entry. It removes the need to call IsLCGMethod / Collectible check on the method table stored in the element and eliminates a possible problem with the method being collected in one place. * Returned missing call to StackFrameInfo::Init to the x86 code path * Removed obsolete comment and code line --- src/coreclr/vm/clrex.h | 6 +++++- src/coreclr/vm/excep.cpp | 33 ++++++++++++++++++++++-------- src/coreclr/vm/exceptionhandling.h | 3 +++ src/coreclr/vm/exinfo.cpp | 1 + src/coreclr/vm/object.cpp | 3 +-- 5 files changed, 34 insertions(+), 12 deletions(-) diff --git a/src/coreclr/vm/clrex.h b/src/coreclr/vm/clrex.h index 4cf163354fd92..a050d4381500a 100644 --- a/src/coreclr/vm/clrex.h +++ b/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. @@ -53,13 +56,14 @@ struct StackTraceElement class StackTraceInfo { - int m_keepaliveItemsCount = -1; // -1 indicates the count is not initialized yet + int m_keepaliveItemsCount; 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 AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, UINT_PTR currentSP, MethodDesc* pFunc, CrawlFrame* pCf); }; diff --git a/src/coreclr/vm/excep.cpp b/src/coreclr/vm/excep.cpp index 30b2e189592cf..98e02508a2553 100644 --- a/src/coreclr/vm/excep.cpp +++ b/src/coreclr/vm/excep.cpp @@ -2856,7 +2856,20 @@ void COMPlusCooperativeTransitionHandler(Frame* pFrame) GCX_PREEMP_NO_DTOR(); } +void StackTraceInfo::Init() +{ + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + MODE_ANY; + FORBID_FAULT; + } + CONTRACTL_END; + LOG((LF_EH, LL_INFO10000, "StackTraceInfo::Init (%p)\n", this)); + m_keepaliveItemsCount = -1; // -1 indicates the count is not initialized yet +} #ifndef TARGET_UNIX // Watson is supported on Windows only void SetupWatsonBucket(UINT_PTR currentIP, CrawlFrame* pCf) @@ -2906,6 +2919,10 @@ void StackTraceInfo::EnsureStackTraceArray(StackTraceArray *pStackTrace, size_t if (newCapacity.IsOverflow() || (neededSize > newCapacity.Value())) { newCapacity = S_SIZE_T(neededSize); + if (newCapacity.IsOverflow()) + { + EX_THROW(EEMessageException, (kOverflowException, IDS_EE_ARRAY_DIMENSIONS_EXCEEDED)); + } } stackTraceCapacity = newCapacity.Value(); @@ -2914,15 +2931,9 @@ void StackTraceInfo::EnsureStackTraceArray(StackTraceArray *pStackTrace, size_t newStackTrace.Allocate(stackTraceCapacity); if (pStackTrace->Get() != NULL) { - STRESS_LOG3(LF_EH, LL_INFO1000, "EnsureStackTraceArray copying from stackTrace=%p to stackTrace=%p, owner thread was %04x\n", OBJECTREFToObject(pStackTrace->Get()), OBJECTREFToObject(newStackTrace.Get()), pStackTrace->GetObjectThread()->GetOSThreadId()); // Copy the original array to the new one newStackTrace.CopyDataFrom(*pStackTrace); - // TODO: cleanup this - make the method have delta as the parameter instead - newStackTrace.SetSize(neededSize - 1); - } - else - { - STRESS_LOG1(LF_EH, LL_INFO1000, "EnsureStackTraceArray created a new clean array %p\n", OBJECTREFToObject(newStackTrace.Get())); + _ASSERTE(newStackTrace.Size() == (neededSize - 1)); } // Update the stack trace array pStackTrace->Set(newStackTrace.Get()); @@ -2951,6 +2962,10 @@ void StackTraceInfo::EnsureKeepaliveArray(PTRARRAYREF *ppKeepaliveArray, size_t if (newCapacity.IsOverflow() || (neededSize > newCapacity.Value())) { newCapacity = S_SIZE_T(neededSize); + if (newCapacity.IsOverflow()) + { + EX_THROW(EEMessageException, (kOverflowException, IDS_EE_ARRAY_DIMENSIONS_EXCEEDED)); + } } keepaliveArrayCapacity = newCapacity.Value(); @@ -3014,8 +3029,7 @@ int StackTraceInfo::GetKeepaliveItemsCount(StackTraceArray *pStackTrace) int count = 0; for (size_t i = 0; i < pStackTrace->Size(); i++) { - MethodDesc *pMethod = (*pStackTrace)[i].pFunc; - if (pMethod->IsLCGMethod() || pMethod->GetMethodTable()->Collectible()) + if ((*pStackTrace)[i].flags & STEF_KEEPALIVE) { count++; } @@ -3135,6 +3149,7 @@ BOOL StackTraceInfo::AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, { // The new frame to be added is a method that can be collected, so we need to update the keepalive items count. m_keepaliveItemsCount++; + stackTraceElem.flags |= STEF_KEEPALIVE; } if (m_keepaliveItemsCount != 0) diff --git a/src/coreclr/vm/exceptionhandling.h b/src/coreclr/vm/exceptionhandling.h index 41a4055b68cb7..8c3f66d874be4 100644 --- a/src/coreclr/vm/exceptionhandling.h +++ b/src/coreclr/vm/exceptionhandling.h @@ -137,6 +137,9 @@ struct ExceptionTrackerBase m_fDeliveredFirstChanceNotification(FALSE), m_ExceptionCode((pExceptionRecord != PTR_NULL) ? pExceptionRecord->ExceptionCode : 0) { +#ifndef DACCESS_COMPILE + m_StackTraceInfo.Init(); +#endif // DACCESS_COMPILE #ifndef TARGET_UNIX // Init the WatsonBucketTracker m_WatsonBucketTracker.Init(); diff --git a/src/coreclr/vm/exinfo.cpp b/src/coreclr/vm/exinfo.cpp index c0c7a9c0c1268..3542a8b194763 100644 --- a/src/coreclr/vm/exinfo.cpp +++ b/src/coreclr/vm/exinfo.cpp @@ -98,6 +98,7 @@ void ExInfo::Init() CONTRACTL_END; m_ExceptionFlags.Init(); + m_StackTraceInfo.Init(); m_DebuggerExState.Init(); m_pSearchBoundary = NULL; diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index a4c22a279adad..682e5f584f3a7 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1972,8 +1972,7 @@ bool ExceptionObject::GetStackTraceParts(OBJECTREF stackTraceObj, StackTraceArra size_t count = stackTrace.Size(); for (size_t i = 0; i < count; i++) { - MethodDesc *pMethod = stackTrace[i].pFunc; - if (pMethod->IsLCGMethod() || pMethod->GetMethodTable()->Collectible()) + if (stackTrace[i].flags & STEF_KEEPALIVE) { OBJECTREF keepaliveObject = NULL; if ((j + 1) < keepaliveArrayCapacity) From 8eccdbc8203e82448ec13c9b7464205d0c84bcf1 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Mon, 10 Jun 2024 20:02:01 +0200 Subject: [PATCH 05/15] Few changes based on feedback * Add keep alive items count to the stack trace header. * Implement the concept of frozen stack traces to eliminate copies in the ExceptionDispatchInfo storing / restoring exceptions. --- src/coreclr/vm/clrex.h | 6 +- src/coreclr/vm/comutilnative.cpp | 23 +++--- src/coreclr/vm/debugdebugger.cpp | 2 +- src/coreclr/vm/excep.cpp | 61 +++++++--------- src/coreclr/vm/exceptionhandling.h | 3 - src/coreclr/vm/exinfo.cpp | 1 - src/coreclr/vm/object.cpp | 112 ++++++++++++++++------------- src/coreclr/vm/object.h | 32 ++++++++- 8 files changed, 135 insertions(+), 105 deletions(-) diff --git a/src/coreclr/vm/clrex.h b/src/coreclr/vm/clrex.h index a050d4381500a..0a2cb72de8df4 100644 --- a/src/coreclr/vm/clrex.h +++ b/src/coreclr/vm/clrex.h @@ -56,15 +56,13 @@ struct StackTraceElement class StackTraceInfo { - int m_keepaliveItemsCount; + int m_dummy; // remove this 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 AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, UINT_PTR currentSP, MethodDesc* pFunc, CrawlFrame* pCf); + static BOOL AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, UINT_PTR currentSP, MethodDesc* pFunc, CrawlFrame* pCf); }; diff --git a/src/coreclr/vm/comutilnative.cpp b/src/coreclr/vm/comutilnative.cpp index 73659df912c4c..f1977b19855be 100644 --- a/src/coreclr/vm/comutilnative.cpp +++ b/src/coreclr/vm/comutilnative.cpp @@ -83,6 +83,10 @@ void DeepCopyStackTrace(StackTraceArray &stackTrace, StackTraceArray &stackTrace THROWS; GC_TRIGGERS; MODE_COOPERATIVE; + PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)&stackTrace)); + PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)&stackTraceCopy)); + PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)pKeepaliveArray)); + PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)pKeepaliveArrayCopy)); } CONTRACTL_END; @@ -127,10 +131,8 @@ FCIMPL2(VOID, ExceptionNative::GetStackTracesDeepCopy, Object* pExceptionObjectU struct { StackTraceArray stackTrace; - StackTraceArray stackTraceCopy; EXCEPTIONREF refException = NULL; PTRARRAYREF keepaliveArray = NULL; // Object array of Managed Resolvers - PTRARRAYREF keepaliveArrayCopy = NULL; // Copy of the object array of Managed Resolvers } gc; // GC protect the array reference @@ -140,15 +142,16 @@ FCIMPL2(VOID, ExceptionNative::GetStackTracesDeepCopy, Object* pExceptionObjectU gc.refException = (EXCEPTIONREF)(ObjectToOBJECTREF(pExceptionObjectUnsafe)); gc.refException->GetStackTrace(gc.stackTrace, &gc.keepaliveArray); - DeepCopyStackTrace(gc.stackTrace, gc.stackTraceCopy, &gc.keepaliveArray, &gc.keepaliveArrayCopy); - if (gc.keepaliveArrayCopy != NULL) + gc.stackTrace.MarkAsFrozen(); + + if (gc.keepaliveArray != NULL) { - *pStackTraceUnsafe = OBJECTREFToObject(gc.keepaliveArrayCopy); + *pStackTraceUnsafe = OBJECTREFToObject(gc.keepaliveArray); } else if (gc.stackTrace.Get() != NULL) { - *pStackTraceUnsafe = OBJECTREFToObject(gc.stackTraceCopy.Get()); + *pStackTraceUnsafe = OBJECTREFToObject(gc.stackTrace.Get()); } else { @@ -172,10 +175,8 @@ FCIMPL2(VOID, ExceptionNative::SaveStackTracesFromDeepCopy, Object* pExceptionOb struct { StackTraceArray stackTrace; - StackTraceArray stackTraceCopy; OBJECTREF refStackTrace = NULL; PTRARRAYREF refKeepaliveArray = NULL; - PTRARRAYREF refKeepaliveArrayCopy = NULL; EXCEPTIONREF refException = NULL; } gc; @@ -187,19 +188,19 @@ FCIMPL2(VOID, ExceptionNative::SaveStackTracesFromDeepCopy, Object* pExceptionOb gc.refStackTrace = (OBJECTREF)(ObjectToOBJECTREF(pStackTraceUnsafe)); ExceptionObject::GetStackTraceParts(gc.refStackTrace, gc.stackTrace, &gc.refKeepaliveArray); - DeepCopyStackTrace(gc.stackTrace, gc.stackTraceCopy, &gc.refKeepaliveArray, &gc.refKeepaliveArrayCopy); + _ASSERTE((gc.stackTrace.Get() == NULL) || gc.stackTrace.IsFrozen()); if (gc.refKeepaliveArray != NULL) { // 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); + gc.refException->SetStackTrace(gc.refKeepaliveArray); } else { // The stack trace object is the stack trace I1Array. // Save the stacktrace details in the exception - gc.refException->SetStackTrace(gc.stackTraceCopy.Get()); + gc.refException->SetStackTrace(gc.stackTrace.Get()); } HELPER_METHOD_FRAME_END(); diff --git a/src/coreclr/vm/debugdebugger.cpp b/src/coreclr/vm/debugdebugger.cpp index 96cc3d1f2043c..044c75240147c 100644 --- a/src/coreclr/vm/debugdebugger.cpp +++ b/src/coreclr/vm/debugdebugger.cpp @@ -1044,9 +1044,9 @@ void DebugStackTrace::GetStackFramesFromException(OBJECTREF * e, // Now get the _stackTrace reference StackTraceArray traceData; - EXCEPTIONREF(*e)->GetStackTrace(traceData, pDynamicMethodArray); GCPROTECT_BEGIN(traceData); + EXCEPTIONREF(*e)->GetStackTrace(traceData, pDynamicMethodArray); // The number of frame info elements in the stack trace info pData->cElements = static_cast(traceData.Size()); diff --git a/src/coreclr/vm/excep.cpp b/src/coreclr/vm/excep.cpp index 98e02508a2553..811a0a2661e1a 100644 --- a/src/coreclr/vm/excep.cpp +++ b/src/coreclr/vm/excep.cpp @@ -2856,21 +2856,6 @@ void COMPlusCooperativeTransitionHandler(Frame* pFrame) GCX_PREEMP_NO_DTOR(); } -void StackTraceInfo::Init() -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - MODE_ANY; - FORBID_FAULT; - } - CONTRACTL_END; - - LOG((LF_EH, LL_INFO10000, "StackTraceInfo::Init (%p)\n", this)); - m_keepaliveItemsCount = -1; // -1 indicates the count is not initialized yet -} - #ifndef TARGET_UNIX // Watson is supported on Windows only void SetupWatsonBucket(UINT_PTR currentIP, CrawlFrame* pCf) { @@ -2898,7 +2883,7 @@ void SetupWatsonBucket(UINT_PTR currentIP, CrawlFrame* pCf) } #endif // !TARGET_UNIX -// Ensure that there is space for one more element in the stack trace array. +// Ensure that there is space for neededSize elements in the stack trace array. void StackTraceInfo::EnsureStackTraceArray(StackTraceArray *pStackTrace, size_t neededSize) { CONTRACTL @@ -3020,9 +3005,10 @@ OBJECTREF StackTraceInfo::GetKeepaliveObject(MethodDesc* pMethod) return NULL; } +#ifdef _DEBUG // Get number of methods in the stack trace that can be collected. We need to store keepalive // objects (Resolver / LoaderAllocator) for these methods. -int StackTraceInfo::GetKeepaliveItemsCount(StackTraceArray *pStackTrace) +int GetKeepaliveItemsCount(StackTraceArray *pStackTrace) { LIMITED_METHOD_CONTRACT; @@ -3037,6 +3023,7 @@ int StackTraceInfo::GetKeepaliveItemsCount(StackTraceArray *pStackTrace) return count; } +#endif // _DEBUG // // Append stack frame to an exception stack trace. @@ -3064,7 +3051,7 @@ BOOL StackTraceInfo::AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, BOOL fRaisingForeignException = pCurTES->IsRaisingForeignException(); pCurTES->ResetRaisingForeignException(); - LOG((LF_EH, LL_INFO10000, "StackTraceInfo::AppendElement (%p), IP = %p, SP = %p, %s::%s\n", this, currentIP, currentSP, pFunc ? pFunc->m_pszDebugClassName : "", pFunc ? pFunc->m_pszDebugMethodName : "" )); + LOG((LF_EH, LL_INFO10000, "StackTraceInfo::AppendElement IP = %p, SP = %p, %s::%s\n", currentIP, currentSP, pFunc ? pFunc->m_pszDebugClassName : "", pFunc ? pFunc->m_pszDebugMethodName : "" )); if (pFunc != NULL && pFunc->IsILStub()) return FALSE; @@ -3129,37 +3116,40 @@ BOOL StackTraceInfo::AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, EnsureStackTraceArray(&gc.stackTrace, gc.stackTrace.Size() + 1); - if (wasCreatedByForeignThread) + if (fRaisingForeignException) { - // If the stack trace was created by a foreign thread, we need to update the cached keepalive items count. - m_keepaliveItemsCount = GetKeepaliveItemsCount(&gc.stackTrace); - } - else - { - if (m_keepaliveItemsCount == -1) + // 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(); + if (numCurrentFrames > 0) { - // The m_keepaliveItemsCount was not initialized yet, so we need to calculate it. - m_keepaliveItemsCount = GetKeepaliveItemsCount(&gc.stackTrace); + // "numCurrentFrames" can be zero if the user created an EDI using + // an unthrown exception. + StackTraceElement & refLastElementFromForeignStackTrace = gc.stackTrace[numCurrentFrames - 1]; + refLastElementFromForeignStackTrace.flags |= STEF_LAST_FRAME_FROM_FOREIGN_STACK_TRACE; } - _ASSERTE(m_keepaliveItemsCount == GetKeepaliveItemsCount(&gc.stackTrace)); } + size_t keepaliveItemsCount = gc.stackTrace.GetKeepAliveItemsCount(); + _ASSERTE(keepaliveItemsCount == GetKeepaliveItemsCount(&gc.stackTrace)); + gc.keepaliveObject = GetKeepaliveObject(pFunc); if (gc.keepaliveObject != NULL) { // The new frame to be added is a method that can be collected, so we need to update the keepalive items count. - m_keepaliveItemsCount++; + keepaliveItemsCount++; stackTraceElem.flags |= STEF_KEEPALIVE; } - if (m_keepaliveItemsCount != 0) + if (keepaliveItemsCount != 0) { // One extra slot is added for the stack trace array - EnsureKeepaliveArray(&gc.pKeepaliveArray, m_keepaliveItemsCount + 1); + EnsureKeepaliveArray(&gc.pKeepaliveArray, keepaliveItemsCount + 1); if (gc.keepaliveObject != NULL) { // Add the method to the keepalive array - gc.pKeepaliveArray->SetAt(m_keepaliveItemsCount, gc.keepaliveObject); + gc.pKeepaliveArray->SetAt(keepaliveItemsCount, gc.keepaliveObject); } } else @@ -3168,17 +3158,20 @@ BOOL StackTraceInfo::AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, gc.pKeepaliveArray = NULL; } + gc.stackTrace.SetKeepAliveItemsCount(keepaliveItemsCount); + gc.stackTrace.Append(&stackTraceElem); + _ASSERTE(GetKeepaliveItemsCount(&gc.stackTrace) == keepaliveItemsCount); if (gc.pKeepaliveArray != NULL) { - _ASSERTE(m_keepaliveItemsCount > 0); + _ASSERTE(keepaliveItemsCount > 0); gc.pKeepaliveArray->SetAt(0, gc.stackTrace.Get()); ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->SetStackTrace(dac_cast(gc.pKeepaliveArray)); } else { - _ASSERTE(m_keepaliveItemsCount == 0); + _ASSERTE(keepaliveItemsCount == 0); ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->SetStackTrace(dac_cast(gc.stackTrace.Get())); } diff --git a/src/coreclr/vm/exceptionhandling.h b/src/coreclr/vm/exceptionhandling.h index 8c3f66d874be4..41a4055b68cb7 100644 --- a/src/coreclr/vm/exceptionhandling.h +++ b/src/coreclr/vm/exceptionhandling.h @@ -137,9 +137,6 @@ struct ExceptionTrackerBase m_fDeliveredFirstChanceNotification(FALSE), m_ExceptionCode((pExceptionRecord != PTR_NULL) ? pExceptionRecord->ExceptionCode : 0) { -#ifndef DACCESS_COMPILE - m_StackTraceInfo.Init(); -#endif // DACCESS_COMPILE #ifndef TARGET_UNIX // Init the WatsonBucketTracker m_WatsonBucketTracker.Init(); diff --git a/src/coreclr/vm/exinfo.cpp b/src/coreclr/vm/exinfo.cpp index 3542a8b194763..c0c7a9c0c1268 100644 --- a/src/coreclr/vm/exinfo.cpp +++ b/src/coreclr/vm/exinfo.cpp @@ -98,7 +98,6 @@ void ExInfo::Init() CONTRACTL_END; m_ExceptionFlags.Init(); - m_StackTraceInfo.Init(); m_DebuggerExState.Init(); m_pSearchBoundary = NULL; diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index 682e5f584f3a7..f4821cc52343b 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1493,6 +1493,7 @@ void StackTraceArray::Allocate(size_t size) SetArray(I1ARRAYREF(AllocatePrimitiveArray(ELEMENT_TYPE_I1, static_cast(raw_size)))); SetSize(0); + SetKeepAliveItemsCount(0); SetObjectThread(); } @@ -1526,6 +1527,9 @@ void StackTraceArray::CopyFrom(StackTraceArray const & src) CopyDataFrom(src); } +int GetKeepaliveItemsCount(StackTraceArray *pStackTrace); + + void StackTraceArray::CopyDataFrom(StackTraceArray const & src) { CONTRACTL @@ -1541,9 +1545,9 @@ void StackTraceArray::CopyDataFrom(StackTraceArray const & src) Volatile size = src.Size(); memcpyNoGCRefs(GetRaw(), src.GetRaw(), size * sizeof(StackTraceElement) + sizeof(ArrayHeader)); - SetSize(size); // set size to the exact value which was used when we copied the data // another thread might have changed it at the time of copying + SetKeepAliveItemsCount(::GetKeepaliveItemsCount(this)); SetObjectThread(); // affinitize the newly created array with the current thread } @@ -1910,55 +1914,27 @@ bool ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * GC_TRIGGERS; THROWS; MODE_COOPERATIVE; + PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)&stackTrace)); + PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)outKeepaliveArray)); } CONTRACTL_END; - return ExceptionObject::GetStackTraceParts(_stackTrace, stackTrace, outKeepaliveArray); -} - -// 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. -/* static */ -bool ExceptionObject::GetStackTraceParts(OBJECTREF stackTraceObj, StackTraceArray & stackTrace, PTRARRAYREF * outKeepaliveArray /*= NULL*/) -{ - CONTRACTL - { - GC_TRIGGERS; - THROWS; - MODE_COOPERATIVE; - } - CONTRACTL_END; - - struct - { - StackTraceArray newStackTrace; - PTRARRAYREF keepaliveArray = NULL; - PTRARRAYREF newKeepaliveArray = NULL; - PTR_PTRArray combinedArray = NULL; - } gc; - bool wasCreatedByForeignThread = false; - - GCPROTECT_BEGIN(gc); - - // Extract the stack trace and keepalive arrays from the stack trace object. - if ((stackTraceObj != NULL) && ((dac_cast(OBJECTREFToObject(stackTraceObj)))->GetArrayElementType() != ELEMENT_TYPE_I1)) - { - // The stack trace object is the dynamic methods array with its first slot set to the stack trace I1Array. - gc.combinedArray = dac_cast(OBJECTREFToObject(stackTraceObj)); - stackTrace.Set(dac_cast(gc.combinedArray->GetAt(0))); - gc.keepaliveArray = dac_cast(ObjectToOBJECTREF(gc.combinedArray)); - } - else - { - stackTrace.Set(dac_cast(stackTraceObj)); - } + ExceptionObject::GetStackTraceParts(_stackTrace, stackTrace, outKeepaliveArray); #ifndef DACCESS_COMPILE Thread *pThread = GetThread(); if ((stackTrace.Get() != NULL) && (stackTrace.GetObjectThread() != pThread)) { + struct + { + StackTraceArray newStackTrace; + PTRARRAYREF newKeepaliveArray = NULL; + } gc; + + GCPROTECT_BEGIN(gc); + // When the stack trace was created by other thread than the current one, we create a copy of both the stack trace and the keepalive arrays to make sure // they are not changing while the caller is accessing them. wasCreatedByForeignThread = true; @@ -1966,7 +1942,7 @@ bool ExceptionObject::GetStackTraceParts(OBJECTREF stackTraceObj, StackTraceArra gc.newStackTrace.CopyDataFrom(stackTrace); stackTrace.Set(gc.newStackTrace.Get()); - int keepaliveArrayCapacity = (gc.keepaliveArray == NULL) ? 0 : gc.keepaliveArray->GetNumComponents(); + int keepaliveArrayCapacity = ((*outKeepaliveArray) == NULL) ? 0 : (*outKeepaliveArray)->GetNumComponents(); int j = 0; size_t count = stackTrace.Size(); @@ -1977,12 +1953,14 @@ bool ExceptionObject::GetStackTraceParts(OBJECTREF stackTraceObj, StackTraceArra OBJECTREF keepaliveObject = NULL; if ((j + 1) < keepaliveArrayCapacity) { - keepaliveObject = gc.keepaliveArray->GetAt(j + 1); + keepaliveObject = (*outKeepaliveArray)->GetAt(j + 1); } if (keepaliveObject == NULL) { // Trim the stack trace at a point where a dynamic or collectible method is found without a corresponding keepalive object. stackTrace.SetSize(i); + stackTrace.SetKeepAliveItemsCount(j); + _ASSERTE(GetKeepaliveItemsCount(&stackTrace) == j); break; } j++; @@ -1992,25 +1970,63 @@ bool ExceptionObject::GetStackTraceParts(OBJECTREF stackTraceObj, StackTraceArra if (keepaliveArrayCapacity != 0) { gc.newKeepaliveArray = (PTRARRAYREF)AllocateObjectArray(static_cast(keepaliveArrayCapacity), g_pObjectClass); - _ASSERTE(gc.keepaliveArray != NULL); + _ASSERTE((*outKeepaliveArray) != NULL); memmoveGCRefs(gc.newKeepaliveArray->GetDataPtr() + 1, - gc.keepaliveArray->GetDataPtr() + 1, + (*outKeepaliveArray)->GetDataPtr() + 1, (keepaliveArrayCapacity - 1) * sizeof(Object *)); gc.newKeepaliveArray->SetAt(0, stackTrace.Get()); - gc.keepaliveArray = gc.newKeepaliveArray; + *outKeepaliveArray = gc.newKeepaliveArray; } else { - gc.keepaliveArray = NULL; + *outKeepaliveArray = NULL; } + GCPROTECT_END(); } #endif // DACCESS_COMPILE + return wasCreatedByForeignThread; +} + +// 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. +/* static */ +void ExceptionObject::GetStackTraceParts(OBJECTREF stackTraceObj, StackTraceArray & stackTrace, PTRARRAYREF * outKeepaliveArray /*= NULL*/) +{ + CONTRACTL + { + GC_TRIGGERS; + THROWS; + MODE_COOPERATIVE; + PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)&stackTrace)); + PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)outKeepaliveArray)); + } + CONTRACTL_END; + + struct + { + PTRARRAYREF keepaliveArray = NULL; + PTR_PTRArray combinedArray = NULL; + } gc; + + GCPROTECT_BEGIN(gc); + + // Extract the stack trace and keepalive arrays from the stack trace object. + if ((stackTraceObj != NULL) && ((dac_cast(OBJECTREFToObject(stackTraceObj)))->GetArrayElementType() != ELEMENT_TYPE_I1)) + { + // The stack trace object is the dynamic methods array with its first slot set to the stack trace I1Array. + gc.combinedArray = dac_cast(OBJECTREFToObject(stackTraceObj)); + stackTrace.Set(dac_cast(gc.combinedArray->GetAt(0))); + gc.keepaliveArray = dac_cast(ObjectToOBJECTREF(gc.combinedArray)); + } + else + { + stackTrace.Set(dac_cast(stackTraceObj)); + } + if (outKeepaliveArray != NULL) { *outKeepaliveArray = gc.keepaliveArray; } GCPROTECT_END(); - - return wasCreatedByForeignThread; } diff --git a/src/coreclr/vm/object.h b/src/coreclr/vm/object.h index 0fe725f6e018d..26f1922cf26f1 100644 --- a/src/coreclr/vm/object.h +++ b/src/coreclr/vm/object.h @@ -1914,6 +1914,7 @@ class StackTraceArray struct ArrayHeader { size_t m_size; + size_t m_keepAliveItemsCount; Thread * m_thread; }; @@ -1984,6 +1985,31 @@ class StackTraceArray GetHeader()->m_size = size; } + void SetKeepAliveItemsCount(size_t count) + { + WRAPPER_NO_CONTRACT; + GetHeader()->m_keepAliveItemsCount = count; + } + + size_t GetKeepAliveItemsCount() const + { + WRAPPER_NO_CONTRACT; + return GetHeader()->m_keepAliveItemsCount; + } + + void MarkAsFrozen() + { + if (m_array != NULL) + { + GetHeader()->m_thread = (Thread *)(size_t)1; + } + } + + bool IsFrozen() const + { + return GetHeader()->m_thread == (Thread *)(size_t)1; + } + private: StackTraceArray(StackTraceArray const & rhs) = delete; @@ -2020,7 +2046,7 @@ class StackTraceArray CLR_I1 const * GetRaw() const { WRAPPER_NO_CONTRACT; - assert(!!m_array); + _ASSERTE(!!m_array); return const_cast(m_array)->GetDirectPointerToNonObjectElements(); } @@ -2029,7 +2055,7 @@ class StackTraceArray { WRAPPER_NO_CONTRACT; SUPPORTS_DAC; - assert(!!m_array); + _ASSERTE(!!m_array); return dac_cast(m_array->GetDirectPointerToNonObjectElements()); } @@ -2178,7 +2204,7 @@ class ExceptionObject : public Object bool GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outKeepaliveArray = NULL) const; - static bool GetStackTraceParts(OBJECTREF stackTraceObj, StackTraceArray & stackTrace, PTRARRAYREF * outKeepaliveArray); + static void GetStackTraceParts(OBJECTREF stackTraceObj, StackTraceArray & stackTrace, PTRARRAYREF * outKeepaliveArray); OBJECTREF GetStackTraceArrayObject() const { From f774c604281e5b41ccd599e630e7ea9e4b08dc5c Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Mon, 10 Jun 2024 20:44:45 +0200 Subject: [PATCH 06/15] Rename keepalive to keepAlive --- src/coreclr/vm/clrex.h | 4 +- src/coreclr/vm/comutilnative.cpp | 38 +++++++------- src/coreclr/vm/excep.cpp | 88 ++++++++++++++++---------------- src/coreclr/vm/object.cpp | 60 +++++++++++----------- 4 files changed, 95 insertions(+), 95 deletions(-) diff --git a/src/coreclr/vm/clrex.h b/src/coreclr/vm/clrex.h index 0a2cb72de8df4..487aae3ec918d 100644 --- a/src/coreclr/vm/clrex.h +++ b/src/coreclr/vm/clrex.h @@ -58,9 +58,9 @@ class StackTraceInfo { int m_dummy; // remove this - static OBJECTREF GetKeepaliveObject(MethodDesc* pMethod); + static OBJECTREF GetKeepAliveObject(MethodDesc* pMethod); static void EnsureStackTraceArray(StackTraceArray *pStackTrace, size_t neededSize); - static void EnsureKeepaliveArray(PTRARRAYREF *ppKeepaliveArray, size_t neededSize); + static void EnsureKeepAliveArray(PTRARRAYREF *ppKeepAliveArray, size_t neededSize); public: static BOOL AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, UINT_PTR currentSP, MethodDesc* pFunc, CrawlFrame* pCf); }; diff --git a/src/coreclr/vm/comutilnative.cpp b/src/coreclr/vm/comutilnative.cpp index f1977b19855be..af69f511bb85c 100644 --- a/src/coreclr/vm/comutilnative.cpp +++ b/src/coreclr/vm/comutilnative.cpp @@ -76,7 +76,7 @@ FCIMPL0(VOID, ExceptionNative::PrepareForForeignExceptionRaise) } FCIMPLEND -void DeepCopyStackTrace(StackTraceArray &stackTrace, StackTraceArray &stackTraceCopy, PTRARRAYREF *pKeepaliveArray, PTRARRAYREF *pKeepaliveArrayCopy) +void DeepCopyStackTrace(StackTraceArray &stackTrace, StackTraceArray &stackTraceCopy, PTRARRAYREF *pKeepAliveArray, PTRARRAYREF *pKeepAliveArrayCopy) { CONTRACTL { @@ -85,8 +85,8 @@ void DeepCopyStackTrace(StackTraceArray &stackTrace, StackTraceArray &stackTrace MODE_COOPERATIVE; PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)&stackTrace)); PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)&stackTraceCopy)); - PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)pKeepaliveArray)); - PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)pKeepaliveArrayCopy)); + PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)pKeepAliveArray)); + PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)pKeepAliveArrayCopy)); } CONTRACTL_END; @@ -99,20 +99,20 @@ void DeepCopyStackTrace(StackTraceArray &stackTrace, StackTraceArray &stackTrace stackTraceCopy.Set(NULL); } - if (*pKeepaliveArray != NULL) + if (*pKeepAliveArray != NULL) { - // The stack trace object is the keepalive array with its first slot set to the stack trace I1Array. + // 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(); + unsigned cOrigKeepAlive = (*pKeepAliveArray)->GetNumComponents(); // ..and allocate a new array. This can trigger GC or throw under OOM. - *pKeepaliveArrayCopy = (PTRARRAYREF)AllocateObjectArray(cOrigKeepalive, g_pObjectClass); + *pKeepAliveArrayCopy = (PTRARRAYREF)AllocateObjectArray(cOrigKeepAlive, g_pObjectClass); // Deepcopy references to the new array we just allocated - memmoveGCRefs((*pKeepaliveArrayCopy)->GetDataPtr(), (*pKeepaliveArray)->GetDataPtr(), - cOrigKeepalive * sizeof(Object *)); + memmoveGCRefs((*pKeepAliveArrayCopy)->GetDataPtr(), (*pKeepAliveArray)->GetDataPtr(), + cOrigKeepAlive * sizeof(Object *)); - (*pKeepaliveArrayCopy)->SetAt(0, (PTRARRAYREF)stackTraceCopy.Get()); + (*pKeepAliveArrayCopy)->SetAt(0, (PTRARRAYREF)stackTraceCopy.Get()); } } @@ -132,7 +132,7 @@ FCIMPL2(VOID, ExceptionNative::GetStackTracesDeepCopy, Object* pExceptionObjectU { StackTraceArray stackTrace; EXCEPTIONREF refException = NULL; - PTRARRAYREF keepaliveArray = NULL; // Object array of Managed Resolvers + PTRARRAYREF keepAliveArray = NULL; // Object array of Managed Resolvers } gc; // GC protect the array reference @@ -141,13 +141,13 @@ FCIMPL2(VOID, ExceptionNative::GetStackTracesDeepCopy, Object* pExceptionObjectU // Get the exception object reference gc.refException = (EXCEPTIONREF)(ObjectToOBJECTREF(pExceptionObjectUnsafe)); - gc.refException->GetStackTrace(gc.stackTrace, &gc.keepaliveArray); + gc.refException->GetStackTrace(gc.stackTrace, &gc.keepAliveArray); gc.stackTrace.MarkAsFrozen(); - if (gc.keepaliveArray != NULL) + if (gc.keepAliveArray != NULL) { - *pStackTraceUnsafe = OBJECTREFToObject(gc.keepaliveArray); + *pStackTraceUnsafe = OBJECTREFToObject(gc.keepAliveArray); } else if (gc.stackTrace.Get() != NULL) { @@ -176,7 +176,7 @@ FCIMPL2(VOID, ExceptionNative::SaveStackTracesFromDeepCopy, Object* pExceptionOb { StackTraceArray stackTrace; OBJECTREF refStackTrace = NULL; - PTRARRAYREF refKeepaliveArray = NULL; + PTRARRAYREF refKeepAliveArray = NULL; EXCEPTIONREF refException = NULL; } gc; @@ -187,14 +187,14 @@ FCIMPL2(VOID, ExceptionNative::SaveStackTracesFromDeepCopy, Object* pExceptionOb gc.refException = (EXCEPTIONREF)(ObjectToOBJECTREF(pExceptionObjectUnsafe)); gc.refStackTrace = (OBJECTREF)(ObjectToOBJECTREF(pStackTraceUnsafe)); - ExceptionObject::GetStackTraceParts(gc.refStackTrace, gc.stackTrace, &gc.refKeepaliveArray); + ExceptionObject::GetStackTraceParts(gc.refStackTrace, gc.stackTrace, &gc.refKeepAliveArray); _ASSERTE((gc.stackTrace.Get() == NULL) || gc.stackTrace.IsFrozen()); - if (gc.refKeepaliveArray != NULL) + if (gc.refKeepAliveArray != NULL) { - // The stack trace object is the keepalive array with its first slot set to the stack trace I1Array. + // 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.refKeepaliveArray); + gc.refException->SetStackTrace(gc.refKeepAliveArray); } else { diff --git a/src/coreclr/vm/excep.cpp b/src/coreclr/vm/excep.cpp index 811a0a2661e1a..146cdf131d552 100644 --- a/src/coreclr/vm/excep.cpp +++ b/src/coreclr/vm/excep.cpp @@ -2926,24 +2926,24 @@ void StackTraceInfo::EnsureStackTraceArray(StackTraceArray *pStackTrace, size_t GCPROTECT_END(); } -// Ensure that there is space for the neededSize elements in the keepalive array. -void StackTraceInfo::EnsureKeepaliveArray(PTRARRAYREF *ppKeepaliveArray, size_t neededSize) +// Ensure that there is space for the neededSize elements in the keepAlive array. +void StackTraceInfo::EnsureKeepAliveArray(PTRARRAYREF *ppKeepAliveArray, size_t neededSize) { CONTRACTL { GC_TRIGGERS; THROWS; - PRECONDITION(CheckPointer(ppKeepaliveArray)); + PRECONDITION(CheckPointer(ppKeepAliveArray)); } CONTRACTL_END; - PTRARRAYREF pNewKeepaliveArray = NULL; - GCPROTECT_BEGIN(pNewKeepaliveArray); + PTRARRAYREF pNewKeepAliveArray = NULL; + GCPROTECT_BEGIN(pNewKeepAliveArray); - size_t keepaliveArrayCapacity = (*ppKeepaliveArray != NULL) ? (*ppKeepaliveArray)->GetNumComponents() : 0; - if (neededSize > keepaliveArrayCapacity) + size_t keepAliveArrayCapacity = (*ppKeepAliveArray != NULL) ? (*ppKeepAliveArray)->GetNumComponents() : 0; + if (neededSize > keepAliveArrayCapacity) { - S_SIZE_T newCapacity = S_SIZE_T(keepaliveArrayCapacity) * S_SIZE_T(2); + S_SIZE_T newCapacity = S_SIZE_T(keepAliveArrayCapacity) * S_SIZE_T(2); if (newCapacity.IsOverflow() || (neededSize > newCapacity.Value())) { newCapacity = S_SIZE_T(neededSize); @@ -2953,33 +2953,33 @@ void StackTraceInfo::EnsureKeepaliveArray(PTRARRAYREF *ppKeepaliveArray, size_t } } - keepaliveArrayCapacity = newCapacity.Value(); + keepAliveArrayCapacity = newCapacity.Value(); - if (!FitsIn(keepaliveArrayCapacity)) + if (!FitsIn(keepAliveArrayCapacity)) { EX_THROW(EEMessageException, (kOverflowException, IDS_EE_ARRAY_DIMENSIONS_EXCEEDED)); } // Allocate a new array with the needed size - pNewKeepaliveArray = (PTRARRAYREF)AllocateObjectArray(static_cast(keepaliveArrayCapacity), g_pObjectClass); - if ((*ppKeepaliveArray) != NULL) + pNewKeepAliveArray = (PTRARRAYREF)AllocateObjectArray(static_cast(keepAliveArrayCapacity), g_pObjectClass); + if ((*ppKeepAliveArray) != NULL) { - memmoveGCRefs(pNewKeepaliveArray->GetDataPtr(), - (*ppKeepaliveArray)->GetDataPtr(), + memmoveGCRefs(pNewKeepAliveArray->GetDataPtr(), + (*ppKeepAliveArray)->GetDataPtr(), neededSize * sizeof(Object *)); } - // Update the keepalive array - *ppKeepaliveArray = pNewKeepaliveArray; + // Update the keepAlive array + *ppKeepAliveArray = pNewKeepAliveArray; } GCPROTECT_END(); } -// Get a keepalive object for the given method. The keepalive object is either a +// Get a keepAlive object for the given method. The keepAlive object is either a // Resolver object for DynamicMethodDesc or a LoaderAllocator object for methods in // collectible assemblies. // Returns NULL if the method code cannot be destroyed. -OBJECTREF StackTraceInfo::GetKeepaliveObject(MethodDesc* pMethod) +OBJECTREF StackTraceInfo::GetKeepAliveObject(MethodDesc* pMethod) { LIMITED_METHOD_CONTRACT; @@ -3006,9 +3006,9 @@ OBJECTREF StackTraceInfo::GetKeepaliveObject(MethodDesc* pMethod) } #ifdef _DEBUG -// Get number of methods in the stack trace that can be collected. We need to store keepalive +// Get number of methods in the stack trace that can be collected. We need to store keepAlive // objects (Resolver / LoaderAllocator) for these methods. -int GetKeepaliveItemsCount(StackTraceArray *pStackTrace) +int GetKeepAliveItemsCount(StackTraceArray *pStackTrace) { LIMITED_METHOD_CONTRACT; @@ -3101,15 +3101,15 @@ BOOL StackTraceInfo::AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, struct { StackTraceArray stackTrace; - PTRARRAYREF pKeepaliveArray = NULL; // Object array of Managed Resolvers / Loader Allocators of methods that can be collected - OBJECTREF keepaliveObject = NULL; + PTRARRAYREF pKeepAliveArray = NULL; // Object array of Managed Resolvers / Loader Allocators of methods that can be collected + OBJECTREF keepAliveObject = NULL; } gc; GCPROTECT_BEGIN_THREAD(pThread, gc); - // Fetch the stacktrace and the keepalive array from the exception object. It returns clones of those arrays in case the + // 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. - bool wasCreatedByForeignThread = ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->GetStackTrace(gc.stackTrace, &gc.pKeepaliveArray); + bool wasCreatedByForeignThread = ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->GetStackTrace(gc.stackTrace, &gc.pKeepAliveArray); // 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)); @@ -3131,47 +3131,47 @@ BOOL StackTraceInfo::AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, } } - size_t keepaliveItemsCount = gc.stackTrace.GetKeepAliveItemsCount(); - _ASSERTE(keepaliveItemsCount == GetKeepaliveItemsCount(&gc.stackTrace)); + size_t keepAliveItemsCount = gc.stackTrace.GetKeepAliveItemsCount(); + _ASSERTE(keepAliveItemsCount == GetKeepAliveItemsCount(&gc.stackTrace)); - gc.keepaliveObject = GetKeepaliveObject(pFunc); - if (gc.keepaliveObject != NULL) + gc.keepAliveObject = GetKeepAliveObject(pFunc); + if (gc.keepAliveObject != NULL) { - // The new frame to be added is a method that can be collected, so we need to update the keepalive items count. - keepaliveItemsCount++; + // The new frame to be added is a method that can be collected, so we need to update the keepAlive items count. + keepAliveItemsCount++; stackTraceElem.flags |= STEF_KEEPALIVE; } - if (keepaliveItemsCount != 0) + if (keepAliveItemsCount != 0) { // One extra slot is added for the stack trace array - EnsureKeepaliveArray(&gc.pKeepaliveArray, keepaliveItemsCount + 1); - if (gc.keepaliveObject != NULL) + EnsureKeepAliveArray(&gc.pKeepAliveArray, keepAliveItemsCount + 1); + if (gc.keepAliveObject != NULL) { - // Add the method to the keepalive array - gc.pKeepaliveArray->SetAt(keepaliveItemsCount, gc.keepaliveObject); + // Add the method to the keepAlive array + gc.pKeepAliveArray->SetAt(keepAliveItemsCount, gc.keepAliveObject); } } else { - // There are no methods that can be collected, so we don't need the keepalive array - gc.pKeepaliveArray = NULL; + // There are no methods that can be collected, so we don't need the keepAlive array + gc.pKeepAliveArray = NULL; } - gc.stackTrace.SetKeepAliveItemsCount(keepaliveItemsCount); + gc.stackTrace.SetKeepAliveItemsCount(keepAliveItemsCount); gc.stackTrace.Append(&stackTraceElem); - _ASSERTE(GetKeepaliveItemsCount(&gc.stackTrace) == keepaliveItemsCount); + _ASSERTE(GetKeepAliveItemsCount(&gc.stackTrace) == keepAliveItemsCount); - if (gc.pKeepaliveArray != NULL) + if (gc.pKeepAliveArray != NULL) { - _ASSERTE(keepaliveItemsCount > 0); - gc.pKeepaliveArray->SetAt(0, gc.stackTrace.Get()); - ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->SetStackTrace(dac_cast(gc.pKeepaliveArray)); + _ASSERTE(keepAliveItemsCount > 0); + gc.pKeepAliveArray->SetAt(0, gc.stackTrace.Get()); + ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->SetStackTrace(dac_cast(gc.pKeepAliveArray)); } else { - _ASSERTE(keepaliveItemsCount == 0); + _ASSERTE(keepAliveItemsCount == 0); ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->SetStackTrace(dac_cast(gc.stackTrace.Get())); } diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index f4821cc52343b..31fb4911bd211 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1527,7 +1527,7 @@ void StackTraceArray::CopyFrom(StackTraceArray const & src) CopyDataFrom(src); } -int GetKeepaliveItemsCount(StackTraceArray *pStackTrace); +int GetKeepAliveItemsCount(StackTraceArray *pStackTrace); void StackTraceArray::CopyDataFrom(StackTraceArray const & src) @@ -1547,7 +1547,7 @@ void StackTraceArray::CopyDataFrom(StackTraceArray const & src) memcpyNoGCRefs(GetRaw(), src.GetRaw(), size * sizeof(StackTraceElement) + sizeof(ArrayHeader)); SetSize(size); // set size to the exact value which was used when we copied the data // another thread might have changed it at the time of copying - SetKeepAliveItemsCount(::GetKeepaliveItemsCount(this)); + SetKeepAliveItemsCount(::GetKeepAliveItemsCount(this)); SetObjectThread(); // affinitize the newly created array with the current thread } @@ -1907,7 +1907,7 @@ void ExceptionObject::SetStackTrace(OBJECTREF stackTrace) } #endif // !defined(DACCESS_COMPILE) -bool ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outKeepaliveArray /*= NULL*/) const +bool ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outKeepAliveArray /*= NULL*/) const { CONTRACTL { @@ -1915,12 +1915,12 @@ bool ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * THROWS; MODE_COOPERATIVE; PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)&stackTrace)); - PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)outKeepaliveArray)); + PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)outKeepAliveArray)); } CONTRACTL_END; bool wasCreatedByForeignThread = false; - ExceptionObject::GetStackTraceParts(_stackTrace, stackTrace, outKeepaliveArray); + ExceptionObject::GetStackTraceParts(_stackTrace, stackTrace, outKeepAliveArray); #ifndef DACCESS_COMPILE Thread *pThread = GetThread(); @@ -1930,19 +1930,19 @@ bool ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * struct { StackTraceArray newStackTrace; - PTRARRAYREF newKeepaliveArray = NULL; + PTRARRAYREF newKeepAliveArray = NULL; } gc; GCPROTECT_BEGIN(gc); - // When the stack trace was created by other thread than the current one, we create a copy of both the stack trace and the keepalive arrays to make sure + // When the stack trace was created by other thread than the current one, we create a copy of both the stack trace and the keepAlive arrays to make sure // they are not changing while the caller is accessing them. wasCreatedByForeignThread = true; gc.newStackTrace.Allocate(stackTrace.Capacity()); gc.newStackTrace.CopyDataFrom(stackTrace); stackTrace.Set(gc.newStackTrace.Get()); - int keepaliveArrayCapacity = ((*outKeepaliveArray) == NULL) ? 0 : (*outKeepaliveArray)->GetNumComponents(); + int keepAliveArrayCapacity = ((*outKeepAliveArray) == NULL) ? 0 : (*outKeepAliveArray)->GetNumComponents(); int j = 0; size_t count = stackTrace.Size(); @@ -1950,36 +1950,36 @@ bool ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * { if (stackTrace[i].flags & STEF_KEEPALIVE) { - OBJECTREF keepaliveObject = NULL; - if ((j + 1) < keepaliveArrayCapacity) + OBJECTREF keepAliveObject = NULL; + if ((j + 1) < keepAliveArrayCapacity) { - keepaliveObject = (*outKeepaliveArray)->GetAt(j + 1); + keepAliveObject = (*outKeepAliveArray)->GetAt(j + 1); } - if (keepaliveObject == NULL) + if (keepAliveObject == NULL) { - // Trim the stack trace at a point where a dynamic or collectible method is found without a corresponding keepalive object. + // Trim the stack trace at a point where a dynamic or collectible method is found without a corresponding keepAlive object. stackTrace.SetSize(i); stackTrace.SetKeepAliveItemsCount(j); - _ASSERTE(GetKeepaliveItemsCount(&stackTrace) == j); + _ASSERTE(GetKeepAliveItemsCount(&stackTrace) == j); break; } j++; } } - if (keepaliveArrayCapacity != 0) + if (keepAliveArrayCapacity != 0) { - gc.newKeepaliveArray = (PTRARRAYREF)AllocateObjectArray(static_cast(keepaliveArrayCapacity), g_pObjectClass); - _ASSERTE((*outKeepaliveArray) != NULL); - memmoveGCRefs(gc.newKeepaliveArray->GetDataPtr() + 1, - (*outKeepaliveArray)->GetDataPtr() + 1, - (keepaliveArrayCapacity - 1) * sizeof(Object *)); - gc.newKeepaliveArray->SetAt(0, stackTrace.Get()); - *outKeepaliveArray = gc.newKeepaliveArray; + gc.newKeepAliveArray = (PTRARRAYREF)AllocateObjectArray(static_cast(keepAliveArrayCapacity), g_pObjectClass); + _ASSERTE((*outKeepAliveArray) != NULL); + memmoveGCRefs(gc.newKeepAliveArray->GetDataPtr() + 1, + (*outKeepAliveArray)->GetDataPtr() + 1, + (keepAliveArrayCapacity - 1) * sizeof(Object *)); + gc.newKeepAliveArray->SetAt(0, stackTrace.Get()); + *outKeepAliveArray = gc.newKeepAliveArray; } else { - *outKeepaliveArray = NULL; + *outKeepAliveArray = NULL; } GCPROTECT_END(); } @@ -1991,7 +1991,7 @@ bool ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * // 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. /* static */ -void ExceptionObject::GetStackTraceParts(OBJECTREF stackTraceObj, StackTraceArray & stackTrace, PTRARRAYREF * outKeepaliveArray /*= NULL*/) +void ExceptionObject::GetStackTraceParts(OBJECTREF stackTraceObj, StackTraceArray & stackTrace, PTRARRAYREF * outKeepAliveArray /*= NULL*/) { CONTRACTL { @@ -1999,34 +1999,34 @@ void ExceptionObject::GetStackTraceParts(OBJECTREF stackTraceObj, StackTraceArra THROWS; MODE_COOPERATIVE; PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)&stackTrace)); - PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)outKeepaliveArray)); + PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)outKeepAliveArray)); } CONTRACTL_END; struct { - PTRARRAYREF keepaliveArray = NULL; + PTRARRAYREF keepAliveArray = NULL; PTR_PTRArray combinedArray = NULL; } gc; GCPROTECT_BEGIN(gc); - // Extract the stack trace and keepalive arrays from the stack trace object. + // Extract the stack trace and keepAlive arrays from the stack trace object. if ((stackTraceObj != NULL) && ((dac_cast(OBJECTREFToObject(stackTraceObj)))->GetArrayElementType() != ELEMENT_TYPE_I1)) { // The stack trace object is the dynamic methods array with its first slot set to the stack trace I1Array. gc.combinedArray = dac_cast(OBJECTREFToObject(stackTraceObj)); stackTrace.Set(dac_cast(gc.combinedArray->GetAt(0))); - gc.keepaliveArray = dac_cast(ObjectToOBJECTREF(gc.combinedArray)); + gc.keepAliveArray = dac_cast(ObjectToOBJECTREF(gc.combinedArray)); } else { stackTrace.Set(dac_cast(stackTraceObj)); } - if (outKeepaliveArray != NULL) + if (outKeepAliveArray != NULL) { - *outKeepaliveArray = gc.keepaliveArray; + *outKeepAliveArray = gc.keepAliveArray; } GCPROTECT_END(); } From d056127c9afa3aecebac8088cf61462b937acc00 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Mon, 10 Jun 2024 21:02:50 +0200 Subject: [PATCH 07/15] Handle possible array size overflow In the StackTraceArray::Allocate --- src/coreclr/vm/object.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index 31fb4911bd211..26da749b82705 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1484,14 +1484,14 @@ void StackTraceArray::Allocate(size_t size) } CONTRACTL_END; - size_t raw_size = size * sizeof(StackTraceElement) + sizeof(ArrayHeader); + S_SIZE_T raw_size = S_SIZE_T(size) * S_SIZE_T(sizeof(StackTraceElement)) + S_SIZE_T(sizeof(ArrayHeader)); - if (!FitsIn(raw_size)) + if (raw_size.IsOverflow() || !FitsIn(raw_size.Value()) { EX_THROW(EEMessageException, (kOverflowException, IDS_EE_ARRAY_DIMENSIONS_EXCEEDED)); } - SetArray(I1ARRAYREF(AllocatePrimitiveArray(ELEMENT_TYPE_I1, static_cast(raw_size)))); + SetArray(I1ARRAYREF(AllocatePrimitiveArray(ELEMENT_TYPE_I1, static_cast(raw_size.Value())))); SetSize(0); SetKeepAliveItemsCount(0); SetObjectThread(); From 0fa22adabe7f66cebb2f788b9e082b72358988c8 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Mon, 10 Jun 2024 22:04:29 +0200 Subject: [PATCH 08/15] Fix typo --- src/coreclr/vm/object.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index 26da749b82705..ebad7b1c0bc71 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1486,7 +1486,7 @@ void StackTraceArray::Allocate(size_t size) S_SIZE_T raw_size = S_SIZE_T(size) * S_SIZE_T(sizeof(StackTraceElement)) + S_SIZE_T(sizeof(ArrayHeader)); - if (raw_size.IsOverflow() || !FitsIn(raw_size.Value()) + if (raw_size.IsOverflow() || !FitsIn(raw_size.Value())) { EX_THROW(EEMessageException, (kOverflowException, IDS_EE_ARRAY_DIMENSIONS_EXCEEDED)); } From dfe4e47d0c5d4894b899be08ab1e3bf3d34220f5 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Mon, 10 Jun 2024 23:20:00 +0200 Subject: [PATCH 09/15] Change the size / keepAlive fields in stack trace to uint32_t Plus a build break fix --- src/coreclr/vm/excep.cpp | 26 +++-------------------- src/coreclr/vm/object.cpp | 43 ++++++++++++++++++++++++++------------- src/coreclr/vm/object.h | 18 +++++++++------- 3 files changed, 43 insertions(+), 44 deletions(-) diff --git a/src/coreclr/vm/excep.cpp b/src/coreclr/vm/excep.cpp index 146cdf131d552..bcb09bac48a71 100644 --- a/src/coreclr/vm/excep.cpp +++ b/src/coreclr/vm/excep.cpp @@ -3005,26 +3005,6 @@ OBJECTREF StackTraceInfo::GetKeepAliveObject(MethodDesc* pMethod) return NULL; } -#ifdef _DEBUG -// Get number of methods in the stack trace that can be collected. We need to store keepAlive -// objects (Resolver / LoaderAllocator) for these methods. -int GetKeepAliveItemsCount(StackTraceArray *pStackTrace) -{ - LIMITED_METHOD_CONTRACT; - - int count = 0; - for (size_t i = 0; i < pStackTrace->Size(); i++) - { - if ((*pStackTrace)[i].flags & STEF_KEEPALIVE) - { - count++; - } - } - - return count; -} -#endif // _DEBUG - // // Append stack frame to an exception stack trace. // Returns true if it appended the element, false otherwise. @@ -3131,8 +3111,8 @@ BOOL StackTraceInfo::AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, } } - size_t keepAliveItemsCount = gc.stackTrace.GetKeepAliveItemsCount(); - _ASSERTE(keepAliveItemsCount == GetKeepAliveItemsCount(&gc.stackTrace)); + uint32_t keepAliveItemsCount = gc.stackTrace.GetKeepAliveItemsCount(); + _ASSERTE(keepAliveItemsCount == gc.stackTrace.ComputeKeepAliveItemsCount()); gc.keepAliveObject = GetKeepAliveObject(pFunc); if (gc.keepAliveObject != NULL) @@ -3161,7 +3141,7 @@ BOOL StackTraceInfo::AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, gc.stackTrace.SetKeepAliveItemsCount(keepAliveItemsCount); gc.stackTrace.Append(&stackTraceElem); - _ASSERTE(GetKeepAliveItemsCount(&gc.stackTrace) == keepAliveItemsCount); + _ASSERTE(gc.stackTrace.ComputeKeepAliveItemsCount() == keepAliveItemsCount); if (gc.pKeepAliveArray != NULL) { diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index ebad7b1c0bc71..1575bba9f60ef 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1440,8 +1440,8 @@ void StackTraceArray::Append(StackTraceElement const * elem) // Only the thread that has created the array can append to it assert(GetObjectThread() == GetThreadNULLOk()); - size_t newsize = Size() + 1; - assert(newsize <= Capacity()); + uint32_t newsize = Size() + 1; + _ASSERTE(newsize <= Capacity()); memcpyNoGCRefs(GetData() + Size(), elem, sizeof(StackTraceElement)); MemoryBarrier(); // prevent the newsize from being reordered with the array copy SetSize(newsize); @@ -1464,13 +1464,13 @@ void StackTraceArray::CheckState() const if (!m_array) return; - assert(GetObjectThread() == GetThreadNULLOk()); + _ASSERTE(GetObjectThread() == GetThreadNULLOk()); - size_t size = Size(); + uint32_t size = Size(); StackTraceElement const * p; p = GetData(); - for (size_t i = 0; i < size; ++i) - assert(p[i].pFunc != NULL); + for (uint32_t i = 0; i < size; ++i) + _ASSERTE(p[i].pFunc != NULL); } void StackTraceArray::Allocate(size_t size) @@ -1527,8 +1527,23 @@ void StackTraceArray::CopyFrom(StackTraceArray const & src) CopyDataFrom(src); } -int GetKeepAliveItemsCount(StackTraceArray *pStackTrace); +// Compute the number of methods in the stack trace that can be collected. We need to store keepAlive +// objects (Resolver / LoaderAllocator) for these methods. +uint32_t StackTraceArray::ComputeKeepAliveItemsCount() +{ + LIMITED_METHOD_CONTRACT; + uint32_t count = 0; + for (uint32_t i = 0; i < Size(); i++) + { + if ((*this)[i].flags & STEF_KEEPALIVE) + { + count++; + } + } + + return count; +} void StackTraceArray::CopyDataFrom(StackTraceArray const & src) { @@ -1543,11 +1558,11 @@ void StackTraceArray::CopyDataFrom(StackTraceArray const & src) } CONTRACTL_END; - Volatile size = src.Size(); + Volatile size = src.Size(); memcpyNoGCRefs(GetRaw(), src.GetRaw(), size * sizeof(StackTraceElement) + sizeof(ArrayHeader)); SetSize(size); // set size to the exact value which was used when we copied the data // another thread might have changed it at the time of copying - SetKeepAliveItemsCount(::GetKeepAliveItemsCount(this)); + SetKeepAliveItemsCount(ComputeKeepAliveItemsCount()); SetObjectThread(); // affinitize the newly created array with the current thread } @@ -1942,11 +1957,11 @@ bool ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * gc.newStackTrace.CopyDataFrom(stackTrace); stackTrace.Set(gc.newStackTrace.Get()); - int keepAliveArrayCapacity = ((*outKeepAliveArray) == NULL) ? 0 : (*outKeepAliveArray)->GetNumComponents(); + uint32_t keepAliveArrayCapacity = ((*outKeepAliveArray) == NULL) ? 0 : (*outKeepAliveArray)->GetNumComponents(); - int j = 0; - size_t count = stackTrace.Size(); - for (size_t i = 0; i < count; i++) + uint32_t j = 0; + uint32_t count = stackTrace.Size(); + for (uint32_t i = 0; i < count; i++) { if (stackTrace[i].flags & STEF_KEEPALIVE) { @@ -1960,7 +1975,7 @@ bool ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * // Trim the stack trace at a point where a dynamic or collectible method is found without a corresponding keepAlive object. stackTrace.SetSize(i); stackTrace.SetKeepAliveItemsCount(j); - _ASSERTE(GetKeepAliveItemsCount(&stackTrace) == j); + _ASSERTE(stackTrace.ComputeKeepAliveItemsCount() == j); break; } j++; diff --git a/src/coreclr/vm/object.h b/src/coreclr/vm/object.h index 26f1922cf26f1..a2a6dd28d8458 100644 --- a/src/coreclr/vm/object.h +++ b/src/coreclr/vm/object.h @@ -1913,8 +1913,8 @@ class StackTraceArray { struct ArrayHeader { - size_t m_size; - size_t m_keepAliveItemsCount; + uint32_t m_size; + uint32_t m_keepAliveItemsCount; Thread * m_thread; }; @@ -1946,7 +1946,7 @@ class StackTraceArray m_array = array; } - size_t Size() const + uint32_t Size() const { WRAPPER_NO_CONTRACT; if (!m_array) @@ -1979,24 +1979,28 @@ class StackTraceArray return GetHeader()->m_thread; } - void SetSize(size_t size) + void SetSize(uint32_t size) { WRAPPER_NO_CONTRACT; GetHeader()->m_size = size; } - void SetKeepAliveItemsCount(size_t count) + void SetKeepAliveItemsCount(uint32_t count) { WRAPPER_NO_CONTRACT; GetHeader()->m_keepAliveItemsCount = count; } - size_t GetKeepAliveItemsCount() const + uint32_t GetKeepAliveItemsCount() const { WRAPPER_NO_CONTRACT; return GetHeader()->m_keepAliveItemsCount; } + // Compute the number of methods in the stack trace that can be collected. We need to store keepAlive + // objects (Resolver / LoaderAllocator) for these methods. + uint32_t ComputeKeepAliveItemsCount(); + void MarkAsFrozen() { if (m_array != NULL) @@ -2017,7 +2021,7 @@ class StackTraceArray void CheckState() const; - size_t GetSize() const + uint32_t GetSize() const { WRAPPER_NO_CONTRACT; return GetHeader()->m_size; From 8355c30c5c28733ecafffd06f74a393f479c30f6 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Tue, 11 Jun 2024 00:48:58 +0200 Subject: [PATCH 10/15] Remove SaveStackTracesFromDeepCopy Also rename GetStackTracesDeepCopy to GetFrozenStackTrace and move the return argument to return value. --- .../src/System/Exception.CoreCLR.cs | 11 +--- src/coreclr/vm/comutilnative.cpp | 64 ++++--------------- src/coreclr/vm/comutilnative.h | 3 +- src/coreclr/vm/ecalllist.h | 3 +- 4 files changed, 17 insertions(+), 64 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Exception.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Exception.CoreCLR.cs index 112fd7036cafb..9257e60e6b4c1 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Exception.CoreCLR.cs +++ b/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 object? currentStackTrace); - - [MethodImpl(MethodImplOptions.InternalCall)] - internal static extern void SaveStackTracesFromDeepCopy(Exception exception, object? currentStackTrace); + private static extern object? GetFrozenStackTrace(Exception exception); [MethodImpl(MethodImplOptions.InternalCall)] internal static extern uint GetExceptionCount(); @@ -153,9 +150,7 @@ internal void RestoreDispatchState(in DispatchState dispatchState) _watsonBuckets = dispatchState.WatsonBuckets; _ipForWatsonBuckets = dispatchState.IpForWatsonBuckets; _remoteStackTraceString = dispatchState.RemoteStackTrace; - - SaveStackTracesFromDeepCopy(this, dispatchState.StackTrace); - + _stackTrace = dispatchState.StackTrace; _stackTraceString = null; // Marks the TES state to indicate we have restored foreign exception @@ -243,7 +238,7 @@ public DispatchState( internal DispatchState CaptureDispatchState() { - GetStackTracesDeepCopy(this, out object? stackTrace); + object? stackTrace = GetFrozenStackTrace(this); return new DispatchState(stackTrace, _remoteStackTraceString, _ipForWatsonBuckets, _watsonBuckets); diff --git a/src/coreclr/vm/comutilnative.cpp b/src/coreclr/vm/comutilnative.cpp index af69f511bb85c..89f6d0a52aee0 100644 --- a/src/coreclr/vm/comutilnative.cpp +++ b/src/coreclr/vm/comutilnative.cpp @@ -116,8 +116,9 @@ void DeepCopyStackTrace(StackTraceArray &stackTrace, StackTraceArray &stackTrace } } -// Given an exception object, this method will extract the stacktrace and dynamic method array and set them up for return to the caller. -FCIMPL2(VOID, ExceptionNative::GetStackTracesDeepCopy, Object* pExceptionObjectUnsafe, Object **pStackTraceUnsafe); +// 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 { @@ -126,17 +127,17 @@ FCIMPL2(VOID, ExceptionNative::GetStackTracesDeepCopy, Object* pExceptionObjectU CONTRACTL_END; ASSERT(pExceptionObjectUnsafe != NULL); - ASSERT(pStackTraceUnsafe != NULL); struct { StackTraceArray stackTrace; EXCEPTIONREF refException = NULL; - PTRARRAYREF keepAliveArray = NULL; // Object array of Managed Resolvers + PTRARRAYREF keepAliveArray = NULL; // Object array of Managed Resolvers / AssemblyLoadContexts + OBJECTREF result = NULL; } gc; // 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)); @@ -147,63 +148,22 @@ FCIMPL2(VOID, ExceptionNative::GetStackTracesDeepCopy, Object* pExceptionObjectU if (gc.keepAliveArray != NULL) { - *pStackTraceUnsafe = OBJECTREFToObject(gc.keepAliveArray); + gc.result = gc.keepAliveArray; } else if (gc.stackTrace.Get() != NULL) { - *pStackTraceUnsafe = OBJECTREFToObject(gc.stackTrace.Get()); + gc.result = gc.stackTrace.Get(); } else { - *pStackTraceUnsafe = NULL; + gc.result = 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. -FCIMPL2(VOID, ExceptionNative::SaveStackTracesFromDeepCopy, Object* pExceptionObjectUnsafe, Object *pStackTraceUnsafe); -{ - CONTRACTL - { - FCALL_CHECK; - } - CONTRACTL_END; - ASSERT(pExceptionObjectUnsafe != NULL); - - struct - { - StackTraceArray stackTrace; - OBJECTREF refStackTrace = NULL; - PTRARRAYREF refKeepAliveArray = NULL; - EXCEPTIONREF refException = NULL; - } gc; - - // 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)); - - ExceptionObject::GetStackTraceParts(gc.refStackTrace, gc.stackTrace, &gc.refKeepAliveArray); - _ASSERTE((gc.stackTrace.Get() == NULL) || gc.stackTrace.IsFrozen()); - - if (gc.refKeepAliveArray != NULL) - { - // 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.refKeepAliveArray); - } - else - { - // The stack trace object is the stack trace I1Array. - // Save the stacktrace details in the exception - gc.refException->SetStackTrace(gc.stackTrace.Get()); - } + // There can be no GC after setting the frozenStackTrace until the Object is returned. HELPER_METHOD_FRAME_END(); + + return OBJECTREFToObject(gc.result); } FCIMPLEND diff --git a/src/coreclr/vm/comutilnative.h b/src/coreclr/vm/comutilnative.h index ea30a2c6a7101..c987ecd421a69 100644 --- a/src/coreclr/vm/comutilnative.h +++ b/src/coreclr/vm/comutilnative.h @@ -42,8 +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 FCDECL2(VOID, GetStackTracesDeepCopy, Object* pExceptionObjectUnsafe, Object **pStackTraceUnsafe); - static FCDECL2(VOID, SaveStackTracesFromDeepCopy, Object* pExceptionObjectUnsafe, Object *pStackTraceUnsafe); + static FCDECL1(Object *, GetFrozenStackTrace, Object* pExceptionObjectUnsafe); #ifdef FEATURE_COMINTEROP // NOTE: caller cleans up any partially initialized BSTRs in pED diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index 98a38b8603419..a60e9e6d75264 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -98,8 +98,7 @@ FCFuncStart(gExceptionFuncs) FCFuncElement("IsImmutableAgileException", ExceptionNative::IsImmutableAgileException) FCFuncElement("GetMethodFromStackTrace", SystemNative::GetMethodFromStackTrace) FCFuncElement("PrepareForForeignExceptionRaise", ExceptionNative::PrepareForForeignExceptionRaise) - FCFuncElement("GetStackTracesDeepCopy", ExceptionNative::GetStackTracesDeepCopy) - FCFuncElement("SaveStackTracesFromDeepCopy", ExceptionNative::SaveStackTracesFromDeepCopy) + FCFuncElement("GetFrozenStackTrace", ExceptionNative::GetFrozenStackTrace) FCFuncElement("GetExceptionCount", ExceptionNative::GetExceptionCount) FCFuncEnd() From 20caf05e34e3147a6af6d235ff32bd6c36dc198d Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Tue, 11 Jun 2024 01:22:56 +0200 Subject: [PATCH 11/15] Remove dummy field and an unused function --- .../Runtime/ExceptionServices/AsmOffsets.cs | 24 +++++------ src/coreclr/vm/clrex.h | 2 - src/coreclr/vm/comutilnative.cpp | 40 ------------------- src/coreclr/vm/exceptionhandling.cpp | 10 ++--- src/coreclr/vm/exceptionhandling.h | 2 - src/coreclr/vm/exinfo.h | 2 - src/coreclr/vm/exstate.cpp | 16 -------- src/coreclr/vm/exstate.h | 4 -- src/coreclr/vm/i386/excepx86.cpp | 2 +- 9 files changed, 18 insertions(+), 84 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ExceptionServices/AsmOffsets.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/ExceptionServices/AsmOffsets.cs index 4de704e77cd46..523435c4b45be 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/ExceptionServices/AsmOffsets.cs +++ b/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 = 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_pExContext = 0xa8; + public const int OFFSETOF__ExInfo__m_exception = 0xb0; + public const int OFFSETOF__ExInfo__m_kind = 0xb8; + public const int OFFSETOF__ExInfo__m_passNumber = 0xb9; + public const int OFFSETOF__ExInfo__m_idxCurClause = 0xbc; + public const int OFFSETOF__ExInfo__m_frameIter = 0xc0; 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 = 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_pExContext = 0x5c; + public const int OFFSETOF__ExInfo__m_exception = 0x60; + public const int OFFSETOF__ExInfo__m_kind = 0x64; + public const int OFFSETOF__ExInfo__m_passNumber = 0x65; + public const int OFFSETOF__ExInfo__m_idxCurClause = 0x68; + public const int OFFSETOF__ExInfo__m_frameIter = 0x6c; public const int OFFSETOF__ExInfo__m_notifyDebuggerSP = OFFSETOF__ExInfo__m_frameIter + SIZEOF__StackFrameIterator; #endif // TARGET_64BIT diff --git a/src/coreclr/vm/clrex.h b/src/coreclr/vm/clrex.h index 487aae3ec918d..bef955807eeaf 100644 --- a/src/coreclr/vm/clrex.h +++ b/src/coreclr/vm/clrex.h @@ -56,8 +56,6 @@ struct StackTraceElement class StackTraceInfo { - 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); diff --git a/src/coreclr/vm/comutilnative.cpp b/src/coreclr/vm/comutilnative.cpp index 89f6d0a52aee0..6f742347fe3c2 100644 --- a/src/coreclr/vm/comutilnative.cpp +++ b/src/coreclr/vm/comutilnative.cpp @@ -76,46 +76,6 @@ FCIMPL0(VOID, ExceptionNative::PrepareForForeignExceptionRaise) } FCIMPLEND -void DeepCopyStackTrace(StackTraceArray &stackTrace, StackTraceArray &stackTraceCopy, PTRARRAYREF *pKeepAliveArray, PTRARRAYREF *pKeepAliveArrayCopy) -{ - CONTRACTL - { - THROWS; - GC_TRIGGERS; - MODE_COOPERATIVE; - PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)&stackTrace)); - PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)&stackTraceCopy)); - PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)pKeepAliveArray)); - PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)pKeepAliveArrayCopy)); - } - 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 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); diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index d02653728a8b3..4678ac4650c27 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -2475,7 +2475,7 @@ CLRUnwindStatus ExceptionTracker::ProcessExplicitFrame( // if (!bSkipLastElement) { - m_StackTraceInfo.AppendElement(m_hThrowable, 0, sf.SP, pMD, pcfThisFrame); + StackTraceInfo::AppendElement(m_hThrowable, 0, sf.SP, pMD, pcfThisFrame); } // @@ -2759,7 +2759,7 @@ CLRUnwindStatus ExceptionTracker::ProcessManagedCallFrame( if (!bSkipLastElement) { - m_StackTraceInfo.AppendElement(m_hThrowable, uControlPC, sf.SP, pMD, pcfThisFrame); + StackTraceInfo::AppendElement(m_hThrowable, uControlPC, sf.SP, pMD, pcfThisFrame); } } @@ -7630,7 +7630,7 @@ extern "C" void QCALLTYPE AppendExceptionStackFrame(QCall::ObjectHandleOnStack e _ASSERTE(pMD == codeInfo.GetMethodDesc()); #endif // _DEBUG - pExInfo->m_StackTraceInfo.AppendElement(pExInfo->m_hThrowable, ip, sp, pMD, &pExInfo->m_frameIter.m_crawl); + StackTraceInfo::AppendElement(pExInfo->m_hThrowable, ip, sp, pMD, &pExInfo->m_frameIter.m_crawl); } } @@ -8304,7 +8304,7 @@ extern "C" bool QCALLTYPE SfiInit(StackFrameIterator* pThis, CONTEXT* pStackwalk if (pMD != NULL) { GCX_COOP(); - pExInfo->m_StackTraceInfo.AppendElement(pExInfo->m_hThrowable, 0, GetRegdisplaySP(pExInfo->m_frameIter.m_crawl.GetRegisterSet()), pMD, &pExInfo->m_frameIter.m_crawl); + StackTraceInfo::AppendElement(pExInfo->m_hThrowable, 0, GetRegdisplaySP(pExInfo->m_frameIter.m_crawl.GetRegisterSet()), pMD, &pExInfo->m_frameIter.m_crawl); #if defined(DEBUGGING_SUPPORTED) if (NotifyDebuggerOfStub(pThread, pFrame)) @@ -8570,7 +8570,7 @@ extern "C" bool QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollideCla if (pMD != NULL) { GCX_COOP(); - pTopExInfo->m_StackTraceInfo.AppendElement(pTopExInfo->m_hThrowable, 0, GetRegdisplaySP(pTopExInfo->m_frameIter.m_crawl.GetRegisterSet()), pMD, &pTopExInfo->m_frameIter.m_crawl); + StackTraceInfo::AppendElement(pTopExInfo->m_hThrowable, 0, GetRegdisplaySP(pTopExInfo->m_frameIter.m_crawl.GetRegisterSet()), pMD, &pTopExInfo->m_frameIter.m_crawl); #if defined(DEBUGGING_SUPPORTED) if (NotifyDebuggerOfStub(pThread, pFrame)) diff --git a/src/coreclr/vm/exceptionhandling.h b/src/coreclr/vm/exceptionhandling.h index 41a4055b68cb7..108856bec40c6 100644 --- a/src/coreclr/vm/exceptionhandling.h +++ b/src/coreclr/vm/exceptionhandling.h @@ -101,8 +101,6 @@ struct ExceptionTrackerBase OBJECTHANDLE m_hThrowable; // EXCEPTION_RECORD and CONTEXT_RECORD describing the exception and its location DAC_EXCEPTION_POINTERS m_ptrs; - // Stack trace of the current exception - StackTraceInfo m_StackTraceInfo; // Information for the funclet we are calling EHClauseInfo m_EHClauseInfo; // Flags representing exception handling state (exception is rethrown, unwind has started, various debugger notifications sent etc) diff --git a/src/coreclr/vm/exinfo.h b/src/coreclr/vm/exinfo.h index 86a31166ac18d..781ef4effd830 100644 --- a/src/coreclr/vm/exinfo.h +++ b/src/coreclr/vm/exinfo.h @@ -57,8 +57,6 @@ class ExInfo LPVOID m_dEsp; // Esp when fault occurred, OR esp to restore on endcatch - StackTraceInfo m_StackTraceInfo; - PTR_ExInfo m_pPrevNestedInfo; // pointer to nested info if are handling nested exception size_t* m_pShadowSP; // Zero this after endcatch diff --git a/src/coreclr/vm/exstate.cpp b/src/coreclr/vm/exstate.cpp index a285d85458e6c..10d99a7ef61cf 100644 --- a/src/coreclr/vm/exstate.cpp +++ b/src/coreclr/vm/exstate.cpp @@ -52,22 +52,6 @@ ThreadExceptionState::~ThreadExceptionState() #endif // !TARGET_UNIX } -#if defined(_DEBUG) -void ThreadExceptionState::AssertStackTraceInfo(StackTraceInfo *pSTI) -{ - LIMITED_METHOD_CONTRACT; -#if defined(FEATURE_EH_FUNCLETS) - - _ASSERTE(pSTI == &(m_pCurrentTracker->m_StackTraceInfo) || pSTI == &(m_OOMTracker.m_StackTraceInfo)); - -#else // !FEATURE_EH_FUNCLETS - - _ASSERTE(pSTI == &(m_currentExInfo.m_StackTraceInfo)); - -#endif // !FEATURE_EH_FUNCLETS -} // void ThreadExceptionState::AssertStackTraceInfo() -#endif // _debug - #ifndef DACCESS_COMPILE Thread* ThreadExceptionState::GetMyThread() diff --git a/src/coreclr/vm/exstate.h b/src/coreclr/vm/exstate.h index 2d60a52f2bb50..09f9f6aca6fe3 100644 --- a/src/coreclr/vm/exstate.h +++ b/src/coreclr/vm/exstate.h @@ -138,10 +138,6 @@ class ThreadExceptionState ResetThreadExceptionFlag(TEF_ForeignExceptionRaise); } -#if defined(_DEBUG) - void AssertStackTraceInfo(StackTraceInfo *pSTI); -#endif // _debug - private: Thread* GetMyThread(); diff --git a/src/coreclr/vm/i386/excepx86.cpp b/src/coreclr/vm/i386/excepx86.cpp index 7cc6ebc4c8f0b..7e88af5fb9ed1 100644 --- a/src/coreclr/vm/i386/excepx86.cpp +++ b/src/coreclr/vm/i386/excepx86.cpp @@ -2198,7 +2198,7 @@ StackWalkAction COMPlusThrowCallback( // SWA value if (!pData->bSkipLastElement) { // Append the current frame to the stack trace and save the save trace to the managed Exception object. - pExInfo->m_StackTraceInfo.AppendElement(pThread->GetThrowableAsHandle(), currentIP, currentSP, pFunc, pCf); + StackTraceInfo::AppendElement(pThread->GetThrowableAsHandle(), currentIP, currentSP, pFunc, pCf); } } else From e989ed25dd5b7eaa6fc44e55155746bb571dd09b Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Tue, 11 Jun 2024 02:07:59 +0200 Subject: [PATCH 12/15] Cleanup based on feedback --- src/coreclr/vm/clrex.h | 2 +- src/coreclr/vm/comutilnative.cpp | 8 +------- src/coreclr/vm/excep.cpp | 12 ++++-------- src/coreclr/vm/object.cpp | 25 +++++++------------------ src/coreclr/vm/object.h | 2 +- 5 files changed, 14 insertions(+), 35 deletions(-) diff --git a/src/coreclr/vm/clrex.h b/src/coreclr/vm/clrex.h index bef955807eeaf..d9d543edc3914 100644 --- a/src/coreclr/vm/clrex.h +++ b/src/coreclr/vm/clrex.h @@ -60,7 +60,7 @@ class StackTraceInfo static void EnsureStackTraceArray(StackTraceArray *pStackTrace, size_t neededSize); static void EnsureKeepAliveArray(PTRARRAYREF *ppKeepAliveArray, size_t neededSize); public: - static BOOL AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, UINT_PTR currentSP, MethodDesc* pFunc, CrawlFrame* pCf); + static void AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, UINT_PTR currentSP, MethodDesc* pFunc, CrawlFrame* pCf); }; diff --git a/src/coreclr/vm/comutilnative.cpp b/src/coreclr/vm/comutilnative.cpp index 6f742347fe3c2..db26e5a5cf3d1 100644 --- a/src/coreclr/vm/comutilnative.cpp +++ b/src/coreclr/vm/comutilnative.cpp @@ -110,17 +110,11 @@ FCIMPL1(Object *, ExceptionNative::GetFrozenStackTrace, Object* pExceptionObject { gc.result = gc.keepAliveArray; } - else if (gc.stackTrace.Get() != NULL) - { - gc.result = gc.stackTrace.Get(); - } else { - gc.result = NULL; + gc.result = gc.stackTrace.Get(); } - // There can be no GC after setting the frozenStackTrace until the Object is returned. - HELPER_METHOD_FRAME_END(); return OBJECTREFToObject(gc.result); diff --git a/src/coreclr/vm/excep.cpp b/src/coreclr/vm/excep.cpp index bcb09bac48a71..0061d9bcabbf5 100644 --- a/src/coreclr/vm/excep.cpp +++ b/src/coreclr/vm/excep.cpp @@ -3007,9 +3007,8 @@ OBJECTREF StackTraceInfo::GetKeepAliveObject(MethodDesc* pMethod) // // Append stack frame to an exception stack trace. -// Returns true if it appended the element, false otherwise. // -BOOL StackTraceInfo::AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, UINT_PTR currentSP, MethodDesc* pFunc, CrawlFrame* pCf) +void StackTraceInfo::AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, UINT_PTR currentSP, MethodDesc* pFunc, CrawlFrame* pCf) { CONTRACTL { @@ -3019,7 +3018,6 @@ BOOL StackTraceInfo::AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, } CONTRACTL_END - BOOL bRetVal = FALSE; Thread *pThread = GetThread(); MethodTable* pMT = ObjectFromHandle(hThrowable)->GetMethodTable(); _ASSERTE(IsException(pMT)); @@ -3034,7 +3032,7 @@ BOOL StackTraceInfo::AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, LOG((LF_EH, LL_INFO10000, "StackTraceInfo::AppendElement IP = %p, SP = %p, %s::%s\n", currentIP, currentSP, pFunc ? pFunc->m_pszDebugClassName : "", pFunc ? pFunc->m_pszDebugMethodName : "" )); if (pFunc != NULL && pFunc->IsILStub()) - return FALSE; + return; // Do not save stacktrace to preallocated exception. These are shared. if (CLRException::IsPreallocatedExceptionHandle(hThrowable)) @@ -3044,7 +3042,7 @@ BOOL StackTraceInfo::AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, // exception like a RudeThreadAbort, which will replace the exception // containing the restored stack trace. - return FALSE; + return; } StackTraceElement stackTraceElem; @@ -3089,7 +3087,7 @@ BOOL 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. - bool wasCreatedByForeignThread = ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->GetStackTrace(gc.stackTrace, &gc.pKeepAliveArray); + ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->GetStackTrace(gc.stackTrace, &gc.pKeepAliveArray); // 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)); @@ -3164,8 +3162,6 @@ BOOL StackTraceInfo::AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP, { } EX_END_CATCH(SwallowAllExceptions) - - return TRUE; } void UnwindFrameChain(Thread* pThread, LPVOID pvLimitSP) diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index 1575bba9f60ef..55ae07f21d1ca 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1558,7 +1558,7 @@ void StackTraceArray::CopyDataFrom(StackTraceArray const & src) } CONTRACTL_END; - Volatile size = src.Size(); + uint32_t size = src.Size(); memcpyNoGCRefs(GetRaw(), src.GetRaw(), size * sizeof(StackTraceElement) + sizeof(ArrayHeader)); SetSize(size); // set size to the exact value which was used when we copied the data // another thread might have changed it at the time of copying @@ -1922,7 +1922,7 @@ void ExceptionObject::SetStackTrace(OBJECTREF stackTrace) } #endif // !defined(DACCESS_COMPILE) -bool ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outKeepAliveArray /*= NULL*/) const +void ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outKeepAliveArray /*= NULL*/) const { CONTRACTL { @@ -1934,7 +1934,6 @@ bool ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * } CONTRACTL_END; - bool wasCreatedByForeignThread = false; ExceptionObject::GetStackTraceParts(_stackTrace, stackTrace, outKeepAliveArray); #ifndef DACCESS_COMPILE @@ -1952,7 +1951,6 @@ bool ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * // When the stack trace was created by other thread than the current one, we create a copy of both the stack trace and the keepAlive arrays to make sure // they are not changing while the caller is accessing them. - wasCreatedByForeignThread = true; gc.newStackTrace.Allocate(stackTrace.Capacity()); gc.newStackTrace.CopyDataFrom(stackTrace); stackTrace.Set(gc.newStackTrace.Get()); @@ -1999,8 +1997,6 @@ bool ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * GCPROTECT_END(); } #endif // DACCESS_COMPILE - - return wasCreatedByForeignThread; } // Get the stack trace and the dynamic method array from the stack trace object. @@ -2018,21 +2014,15 @@ void ExceptionObject::GetStackTraceParts(OBJECTREF stackTraceObj, StackTraceArra } CONTRACTL_END; - struct - { - PTRARRAYREF keepAliveArray = NULL; - PTR_PTRArray combinedArray = NULL; - } gc; - - GCPROTECT_BEGIN(gc); + 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)) { // The stack trace object is the dynamic methods array with its first slot set to the stack trace I1Array. - gc.combinedArray = dac_cast(OBJECTREFToObject(stackTraceObj)); - stackTrace.Set(dac_cast(gc.combinedArray->GetAt(0))); - gc.keepAliveArray = dac_cast(ObjectToOBJECTREF(gc.combinedArray)); + PTR_PTRArray combinedArray = dac_cast(OBJECTREFToObject(stackTraceObj)); + stackTrace.Set(dac_cast(combinedArray->GetAt(0))); + keepAliveArray = dac_cast(ObjectToOBJECTREF(combinedArray)); } else { @@ -2041,7 +2031,6 @@ void ExceptionObject::GetStackTraceParts(OBJECTREF stackTraceObj, StackTraceArra if (outKeepAliveArray != NULL) { - *outKeepAliveArray = gc.keepAliveArray; + *outKeepAliveArray = keepAliveArray; } - GCPROTECT_END(); } diff --git a/src/coreclr/vm/object.h b/src/coreclr/vm/object.h index a2a6dd28d8458..c8f8ce664aa55 100644 --- a/src/coreclr/vm/object.h +++ b/src/coreclr/vm/object.h @@ -2206,7 +2206,7 @@ class ExceptionObject : public Object void SetStackTrace(OBJECTREF stackTrace); - bool GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outKeepaliveArray = NULL) const; + void GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outKeepaliveArray = NULL) const; static void GetStackTraceParts(OBJECTREF stackTraceObj, StackTraceArray & stackTrace, PTRARRAYREF * outKeepaliveArray); From 12db638c17f983601e8c72ea3ea89b3be80fdc0e Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Tue, 11 Jun 2024 17:23:39 +0200 Subject: [PATCH 13/15] Move the race handling into GetStackTrace only Plus an unused method removal and a little naming / contract cleanup --- src/coreclr/vm/object.cpp | 66 +++++++++++++++++---------------------- src/coreclr/vm/object.h | 5 +-- 2 files changed, 30 insertions(+), 41 deletions(-) diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index 55ae07f21d1ca..1d00d2d7022cc 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1508,25 +1508,6 @@ size_t StackTraceArray::Capacity() const return (m_array->GetNumComponents() - sizeof(ArrayHeader)) / sizeof(StackTraceElement); } -// Deep copies the stack trace array -void StackTraceArray::CopyFrom(StackTraceArray const & src) -{ - CONTRACTL - { - THROWS; - GC_TRIGGERS; - MODE_COOPERATIVE; - INJECT_FAULT(ThrowOutOfMemory();); - PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)this)); - PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)&src)); - } - CONTRACTL_END; - - m_array = (I1ARRAYREF) AllocatePrimitiveArray(ELEMENT_TYPE_I1, static_cast(src.Get()->GetNumComponents())); - - CopyDataFrom(src); -} - // Compute the number of methods in the stack trace that can be collected. We need to store keepAlive // objects (Resolver / LoaderAllocator) for these methods. uint32_t StackTraceArray::ComputeKeepAliveItemsCount() @@ -1545,14 +1526,13 @@ uint32_t StackTraceArray::ComputeKeepAliveItemsCount() return count; } -void StackTraceArray::CopyDataFrom(StackTraceArray const & src) +uint32_t StackTraceArray::CopyDataFrom(StackTraceArray const & src) { CONTRACTL { - THROWS; - GC_TRIGGERS; + NOTHROW; + GC_NOTRIGGER; MODE_COOPERATIVE; - INJECT_FAULT(ThrowOutOfMemory();); PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)this)); PRECONDITION(IsProtectedByGCFrame((OBJECTREF*)&src)); } @@ -1560,10 +1540,10 @@ void StackTraceArray::CopyDataFrom(StackTraceArray const & src) uint32_t size = src.Size(); memcpyNoGCRefs(GetRaw(), src.GetRaw(), size * sizeof(StackTraceElement) + sizeof(ArrayHeader)); - SetSize(size); // set size to the exact value which was used when we copied the data - // another thread might have changed it at the time of copying - SetKeepAliveItemsCount(ComputeKeepAliveItemsCount()); - SetObjectThread(); // affinitize the newly created array with the current thread + // Affinitize the copy with the current thread + SetObjectThread(); + + return size; } #ifdef _DEBUG @@ -1922,6 +1902,13 @@ void ExceptionObject::SetStackTrace(OBJECTREF stackTrace) } #endif // !defined(DACCESS_COMPILE) +// Get the stack trace and keep alive array for the exception. +// Both arrays returned by the method are safe to work with without other threads modifying them. +// - if the stack trace was created by the current thread, the arrays are returned as is. +// - if it was created by another thread, deep copies of the arrays are returned. It is ensured +// 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 { CONTRACTL @@ -1952,41 +1939,46 @@ void ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * // When the stack trace was created by other thread than the current one, we create a copy of both the stack trace and the keepAlive arrays to make sure // they are not changing while the caller is accessing them. gc.newStackTrace.Allocate(stackTrace.Capacity()); - gc.newStackTrace.CopyDataFrom(stackTrace); + uint32_t numCopiedFrames = gc.newStackTrace.CopyDataFrom(stackTrace); + // Set size to the exact value which was used when we copied the data, + // another thread might have changed it at the time of copying + gc.newStackTrace.SetSize(numCopiedFrames); + stackTrace.Set(gc.newStackTrace.Get()); uint32_t keepAliveArrayCapacity = ((*outKeepAliveArray) == NULL) ? 0 : (*outKeepAliveArray)->GetNumComponents(); - uint32_t j = 0; + uint32_t keepAliveItemsCount = 0; uint32_t count = stackTrace.Size(); - for (uint32_t i = 0; i < count; i++) + for (uint32_t i = 0; i < numCopiedFrames; i++) { if (stackTrace[i].flags & STEF_KEEPALIVE) { OBJECTREF keepAliveObject = NULL; - if ((j + 1) < keepAliveArrayCapacity) + if ((keepAliveItemsCount + 1) < keepAliveArrayCapacity) { - keepAliveObject = (*outKeepAliveArray)->GetAt(j + 1); + keepAliveObject = (*outKeepAliveArray)->GetAt(keepAliveItemsCount + 1); } if (keepAliveObject == NULL) { // Trim the stack trace at a point where a dynamic or collectible method is found without a corresponding keepAlive object. stackTrace.SetSize(i); - stackTrace.SetKeepAliveItemsCount(j); - _ASSERTE(stackTrace.ComputeKeepAliveItemsCount() == j); break; } - j++; + keepAliveItemsCount++; } } + stackTrace.SetKeepAliveItemsCount(keepAliveItemsCount); + _ASSERTE(stackTrace.ComputeKeepAliveItemsCount() == keepAliveItemsCount); + if (keepAliveArrayCapacity != 0) { - gc.newKeepAliveArray = (PTRARRAYREF)AllocateObjectArray(static_cast(keepAliveArrayCapacity), g_pObjectClass); + gc.newKeepAliveArray = (PTRARRAYREF)AllocateObjectArray(keepAliveArrayCapacity, g_pObjectClass); _ASSERTE((*outKeepAliveArray) != NULL); memmoveGCRefs(gc.newKeepAliveArray->GetDataPtr() + 1, (*outKeepAliveArray)->GetDataPtr() + 1, - (keepAliveArrayCapacity - 1) * sizeof(Object *)); + (keepAliveItemsCount) * sizeof(Object *)); gc.newKeepAliveArray->SetAt(0, stackTrace.Get()); *outKeepAliveArray = gc.newKeepAliveArray; } diff --git a/src/coreclr/vm/object.h b/src/coreclr/vm/object.h index c8f8ce664aa55..b79dcf4bd31d5 100644 --- a/src/coreclr/vm/object.h +++ b/src/coreclr/vm/object.h @@ -1968,10 +1968,7 @@ class StackTraceArray return m_array; } - // Deep copies the array - void CopyFrom(StackTraceArray const & src); - - void CopyDataFrom(StackTraceArray const & src); + uint32_t CopyDataFrom(StackTraceArray const & src); Thread * GetObjectThread() const { From 4259620d0761cc72ba5b24b0c843be3bcf09561b Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Tue, 11 Jun 2024 21:13:30 +0200 Subject: [PATCH 14/15] Add VolatileLoad/Store around the size / keep alive count Also remove the memory barrier from the StackTraceArray::Append since it is not needed after that change. --- src/coreclr/vm/object.cpp | 2 -- src/coreclr/vm/object.h | 8 ++++---- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index 1d00d2d7022cc..7610b8362fb5a 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1443,7 +1443,6 @@ void StackTraceArray::Append(StackTraceElement const * elem) uint32_t newsize = Size() + 1; _ASSERTE(newsize <= Capacity()); memcpyNoGCRefs(GetData() + Size(), elem, sizeof(StackTraceElement)); - MemoryBarrier(); // prevent the newsize from being reordered with the array copy SetSize(newsize); #if defined(_DEBUG) @@ -1949,7 +1948,6 @@ void ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * uint32_t keepAliveArrayCapacity = ((*outKeepAliveArray) == NULL) ? 0 : (*outKeepAliveArray)->GetNumComponents(); uint32_t keepAliveItemsCount = 0; - uint32_t count = stackTrace.Size(); for (uint32_t i = 0; i < numCopiedFrames; i++) { if (stackTrace[i].flags & STEF_KEEPALIVE) diff --git a/src/coreclr/vm/object.h b/src/coreclr/vm/object.h index b79dcf4bd31d5..018135b5205d1 100644 --- a/src/coreclr/vm/object.h +++ b/src/coreclr/vm/object.h @@ -1979,19 +1979,19 @@ class StackTraceArray void SetSize(uint32_t size) { WRAPPER_NO_CONTRACT; - GetHeader()->m_size = size; + VolatileStore(&GetHeader()->m_size, size); } void SetKeepAliveItemsCount(uint32_t count) { WRAPPER_NO_CONTRACT; - GetHeader()->m_keepAliveItemsCount = count; + VolatileStore(&GetHeader()->m_keepAliveItemsCount, count); } uint32_t GetKeepAliveItemsCount() const { WRAPPER_NO_CONTRACT; - return GetHeader()->m_keepAliveItemsCount; + return VolatileLoad(&GetHeader()->m_keepAliveItemsCount); } // Compute the number of methods in the stack trace that can be collected. We need to store keepAlive @@ -2021,7 +2021,7 @@ class StackTraceArray uint32_t GetSize() const { WRAPPER_NO_CONTRACT; - return GetHeader()->m_size; + return VolatileLoad(&GetHeader()->m_size); } void SetObjectThread() From fcf61821a8e4e469e7c5d58f21b9c1cd6b974b31 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 13 Jun 2024 01:19:28 +0200 Subject: [PATCH 15/15] Add comment on why trimming the stack trace by keep alive is needed I have also realized that when we need to trim, the keepAlive array is always fully populated, so we don't need to check for cases where there would be NULL in an entry of the array. --- src/coreclr/vm/object.cpp | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index 7610b8362fb5a..245a974842edf 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1947,22 +1947,41 @@ void ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * uint32_t keepAliveArrayCapacity = ((*outKeepAliveArray) == NULL) ? 0 : (*outKeepAliveArray)->GetNumComponents(); + // It is possible that another thread was modifying the stack trace array and keep alive array while we were making the copies. + // The following sequence of events could have happened: + // Case 1: + // * The current thread gets the stack trace array and the keep alive array references using the ExceptionObject::GetStackTraceParts above + // * The current thread reads the size of the stack trace array + // * Another thread adds a new stack frame to the stack trace array and a corresponding keep alive object to the keep alive array + // * The current thread creates a copy of the stack trace using the size it read before + // * The keep alive count stored in the stack trace array doesn't match the elements in the copy of the stack trace array + // In this case, we need to recompute the keep alive count based on the copied stack trace array. + // + // Case 2: + // * The current thread gets the stack trace array and the keep alive array references using the ExceptionObject::GetStackTraceParts above + // * Another thread adds a stack frame with a keep alive item and that exceeds the keep alive array capacity. So it allocates a new keep alive array + // and adds the new keep alive item to it. + // * Thus the keep alive array this thread has read doesn't have the keep alive item, but the stack trace array contains the element the other thread has added. + // In this case, we need to trim the stack trace array at the first element that doesn't have a corresponding keep alive object in the keep alive array. + // We cannot fetch the keep alive object for that stack trace entry, because in the meanwhile, the keep alive array that the other thread created + // may have been collected and the method related to the stack trace entry may have been collected as well. + // uint32_t keepAliveItemsCount = 0; for (uint32_t i = 0; i < numCopiedFrames; i++) { if (stackTrace[i].flags & STEF_KEEPALIVE) { - OBJECTREF keepAliveObject = NULL; - if ((keepAliveItemsCount + 1) < keepAliveArrayCapacity) - { - keepAliveObject = (*outKeepAliveArray)->GetAt(keepAliveItemsCount + 1); - } - if (keepAliveObject == NULL) + if ((keepAliveItemsCount + 1) >= keepAliveArrayCapacity) { // Trim the stack trace at a point where a dynamic or collectible method is found without a corresponding keepAlive object. stackTrace.SetSize(i); break; } + else + { + _ASSERTE((*outKeepAliveArray)->GetAt(keepAliveItemsCount + 1) != NULL); + } + keepAliveItemsCount++; } }