From 6029fb1572b3c4ef5a71d685426a42b5984b3fa7 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Sat, 28 Sep 2019 18:32:00 +0100 Subject: [PATCH] Remove global locks from Exception stack trace handling (dotnet/coreclr#26823) Commit migrated from https://github.com/dotnet/coreclr/commit/bef420a74e1a7ff81883b8bb3a36a9a452e0af61 --- .../src/System/Exception.CoreCLR.cs | 18 ++-- src/coreclr/src/vm/comutilnative.cpp | 4 +- src/coreclr/src/vm/debugdebugger.cpp | 6 +- src/coreclr/src/vm/dwbucketmanager.hpp | 69 ------------ src/coreclr/src/vm/dwreport.cpp | 4 +- src/coreclr/src/vm/excep.cpp | 4 +- src/coreclr/src/vm/mscorlib.h | 7 +- src/coreclr/src/vm/object.cpp | 117 +++++++++++++++------ src/coreclr/src/vm/object.h | 9 +- 9 files changed, 112 insertions(+), 126 deletions(-) diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Exception.CoreCLR.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Exception.CoreCLR.cs index 67421d0..513ce88 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/Exception.CoreCLR.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/Exception.CoreCLR.cs @@ -243,14 +243,6 @@ namespace System _stackTraceString = null; } - // This is the object against which a lock will be taken - // when attempt to restore the EDI. Since its static, its possible - // that unrelated exception object restorations could get blocked - // for a small duration but that sounds reasonable considering - // such scenarios are going to be extremely rare, where timing - // matches precisely. - private static readonly object s_DispatchStateLock = new object(); - [MethodImpl(MethodImplOptions.InternalCall)] private static extern void PrepareForForeignExceptionRaise(); @@ -293,6 +285,9 @@ namespace System } } + // Returns the lock, falling back to the static lock if the Exception is uninitialized (very rare). + private object StackTraceLock => _stackTraceLock ?? s_stackTraceLock; + // This is invoked by ExceptionDispatchInfo.Throw to restore the exception stack trace, corresponding to the original throw of the // exception, just before the exception is "rethrown". internal void RestoreDispatchState(in DispatchState dispatchState) @@ -324,7 +319,7 @@ namespace System // // Since EDI can be created at various points during exception dispatch (e.g. at various frames on the stack) for the same exception instance, // they can have different data to be restored. Thus, to ensure atomicity of restoration from each EDI, perform the restore under a lock. - lock (s_DispatchStateLock) + lock (StackTraceLock) { _watsonBuckets = dispatchState.WatsonBuckets; _ipForWatsonBuckets = dispatchState.IpForWatsonBuckets; @@ -340,6 +335,9 @@ namespace System } } + // This is the static object against which a lock will be taken when the Exception is uninitialized (constructor not called). + private static readonly object s_stackTraceLock = new object(); + private MethodBase? _exceptionMethod; // Needed for serialization. internal string? _message; private IDictionary? _data; @@ -356,6 +354,8 @@ namespace System // unless a System.Resolver object roots it. private readonly object? _dynamicMethods; private string? _source; // Mainly used by VB. + // This is the object against which a lock will be taken when attempting to restore the EDI. + private readonly object _stackTraceLock = new object(); private UIntPtr _ipForWatsonBuckets; // Used to persist the IP for Watson Bucketing private readonly IntPtr _xptrs; // Internal EE stuff private readonly int _xcode = _COMPlusExceptionCode; // Internal EE stuff diff --git a/src/coreclr/src/vm/comutilnative.cpp b/src/coreclr/src/vm/comutilnative.cpp index e5e3cac..21d2219 100644 --- a/src/coreclr/src/vm/comutilnative.cpp +++ b/src/coreclr/src/vm/comutilnative.cpp @@ -250,11 +250,11 @@ FCIMPL3(VOID, ExceptionNative::SaveStackTracesFromDeepCopy, Object* pExceptionOb if (gc.stackTrace.Size() > 0) { // Save the stacktrace details in the exception under a lock - gc.refException->SetStackTrace(gc.stackTrace, gc.dynamicMethodsArray); + gc.refException->SetStackTrace(gc.stackTrace.Get(), gc.dynamicMethodsArray); } else { - gc.refException->SetNullStackTrace(); + gc.refException->SetStackTrace(NULL, NULL); } HELPER_METHOD_FRAME_END(); diff --git a/src/coreclr/src/vm/debugdebugger.cpp b/src/coreclr/src/vm/debugdebugger.cpp index 58c186a..abec53f 100644 --- a/src/coreclr/src/vm/debugdebugger.cpp +++ b/src/coreclr/src/vm/debugdebugger.cpp @@ -1086,9 +1086,11 @@ void DebugStackTrace::GetStackFramesFromException(OBJECTREF * e, // Now get the _stackTrace reference StackTraceArray traceData; - EXCEPTIONREF(*e)->GetStackTrace(traceData, pDynamicMethodArray); - + ZeroMemory(&traceData, sizeof(traceData)); 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/src/vm/dwbucketmanager.hpp b/src/coreclr/src/vm/dwbucketmanager.hpp index 533acc3..e33d227 100644 --- a/src/coreclr/src/vm/dwbucketmanager.hpp +++ b/src/coreclr/src/vm/dwbucketmanager.hpp @@ -307,7 +307,6 @@ private: DWORD GetILOffset(); bool GetFileVersionInfoForModule(Module* pModule, USHORT& major, USHORT& minor, USHORT& build, USHORT& revision); bool IsCodeContractsFrame(MethodDesc* pMD); - void FindFaultingMethodInfo(); OBJECTREF GetRealExceptionObject(); WCHAR* GetParamBufferForIndex(BucketParameterIndex paramIndex); void LogParam(__in_z LPCWSTR paramValue, BucketParameterIndex paramIndex); @@ -360,9 +359,6 @@ BaseBucketParamsManager::BaseBucketParamsManager(GenericModeBlock* pGenericModeB if (codeInfo.IsValid()) { m_pFaultingMD = codeInfo.GetMethodDesc(); - - if (m_pFaultingMD) - FindFaultingMethodInfo(); } } @@ -1034,71 +1030,6 @@ bool BaseBucketParamsManager::IsCodeContractsFrame(MethodDesc* pMD) return false; } -// code contract failures will have several frames on the stack which are part of the code contracts infrastructure. -// as such we don't want to blame any of these frames since they're just propagating the fault from the user's code. -// the purpose of this function is to identify if the current faulting frame is part of the code contract infrastructure -// and if it is to traverse the stack trace in the exception object until the first frame which isn't code contracts stuff. -void BaseBucketParamsManager::FindFaultingMethodInfo() -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - MODE_ANY; - PRECONDITION(m_pFaultingMD != NULL); - } - CONTRACTL_END; - - // check if this frame is part of the code contracts infrastructure - if (IsCodeContractsFrame(m_pFaultingMD)) - { - // it is so we need to do more searching to find the correct faulting MethodDesc. - // iterate over each frame in the stack trace object until we find the first - // frame that isn't part of the code contracts goop. - GCX_COOP(); - - OBJECTREF throwable = GetRealExceptionObject(); - - if (throwable != NULL) - { - StackTraceArray traceData; - EXCEPTIONREF(throwable)->GetStackTrace(traceData); - - GCPROTECT_BEGIN(traceData); - - size_t numElements = traceData.Size(); - - ContractFailureKind kind = GetContractFailureKind(throwable); - - // skip frame 0 since we already know it's part of code contracts - for (size_t index = 1; index < numElements; ++index) - { - StackTraceElement const& cur = traceData[index]; - - MethodDesc* pMD = cur.pFunc; - _ASSERTE(pMD); - - if (!IsCodeContractsFrame(pMD)) - { - // we want the next frame for preconditions however if we don't have it for some - // reason then just use this frame (better than defaulting to the code contracts goop) - if ((kind == CONTRACT_FAILURE_PRECONDITION) && (index + 1 < numElements)) - { - _ASSERTE(!IsCodeContractsFrame(traceData[index + 1].pFunc)); - continue; - } - - m_pFaultingMD = pMD; - m_faultingPc = cur.ip; - break; - } - } - - GCPROTECT_END(); - } - } -} - // gets the "real" exception object. it might be m_pException or the exception object on the thread OBJECTREF BaseBucketParamsManager::GetRealExceptionObject() { diff --git a/src/coreclr/src/vm/dwreport.cpp b/src/coreclr/src/vm/dwreport.cpp index 14e90f7..f4fe1a4 100644 --- a/src/coreclr/src/vm/dwreport.cpp +++ b/src/coreclr/src/vm/dwreport.cpp @@ -521,9 +521,9 @@ UINT_PTR GetIPOfThrowSite( { // Get the BYTE[] containing the stack trace. StackTraceArray traceData; - ((EXCEPTIONREF)throwable)->GetStackTrace(traceData); - + ZeroMemory(&traceData, sizeof(traceData)); GCPROTECT_BEGIN(traceData); + ((EXCEPTIONREF)throwable)->GetStackTrace(traceData); // Grab the first non-zero, if there is one. for (size_t ix = 0; ix < traceData.Size(); ++ix) { diff --git a/src/coreclr/src/vm/excep.cpp b/src/coreclr/src/vm/excep.cpp index d180091..f0ce1c1 100644 --- a/src/coreclr/src/vm/excep.cpp +++ b/src/coreclr/src/vm/excep.cpp @@ -2211,7 +2211,7 @@ void StackTraceInfo::SaveStackTrace(BOOL bAllowAllocMem, OBJECTHANDLE hThrowable { CONTRACTL { - NOTHROW; + THROWS; GC_TRIGGERS; MODE_COOPERATIVE; } @@ -2542,7 +2542,7 @@ void StackTraceInfo::SaveStackTrace(BOOL bAllowAllocMem, OBJECTHANDLE hThrowable } } - ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->SetStackTrace(gc.stackTrace, gc.dynamicMethodsArray); + ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->SetStackTrace(gc.stackTrace.Get(), gc.dynamicMethodsArray); // Update _stackTraceString field. ((EXCEPTIONREF)ObjectFromHandle(hThrowable))->SetStackTraceString(NULL); diff --git a/src/coreclr/src/vm/mscorlib.h b/src/coreclr/src/vm/mscorlib.h index 1e743ba..88935e3 100644 --- a/src/coreclr/src/vm/mscorlib.h +++ b/src/coreclr/src/vm/mscorlib.h @@ -341,16 +341,17 @@ DEFINE_FIELD_U(_message, ExceptionObject, _message) DEFINE_FIELD_U(_data, ExceptionObject, _data) DEFINE_FIELD_U(_innerException, ExceptionObject, _innerException) DEFINE_FIELD_U(_helpURL, ExceptionObject, _helpURL) -DEFINE_FIELD_U(_source, ExceptionObject, _source) 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(_stackTraceLock, ExceptionObject, _stackTraceLock) +DEFINE_FIELD_U(_ipForWatsonBuckets,ExceptionObject, _ipForWatsonBuckets) DEFINE_FIELD_U(_xptrs, ExceptionObject, _xptrs) -DEFINE_FIELD_U(_HResult, ExceptionObject, _HResult) DEFINE_FIELD_U(_xcode, ExceptionObject, _xcode) -DEFINE_FIELD_U(_ipForWatsonBuckets,ExceptionObject, _ipForWatsonBuckets) +DEFINE_FIELD_U(_HResult, ExceptionObject, _HResult) DEFINE_CLASS(EXCEPTION, System, Exception) DEFINE_METHOD(EXCEPTION, GET_CLASS_NAME, GetClassName, IM_RetStr) DEFINE_PROPERTY(EXCEPTION, MESSAGE, Message, Str) diff --git a/src/coreclr/src/vm/object.cpp b/src/coreclr/src/vm/object.cpp index 387fe22..96732c1 100644 --- a/src/coreclr/src/vm/object.cpp +++ b/src/coreclr/src/vm/object.cpp @@ -1969,54 +1969,114 @@ StackTraceElement & StackTraceArray::operator[](size_t index) } #if !defined(DACCESS_COMPILE) -// Define the lock used to access stacktrace from an exception object +// Define the lock used to access stacktrace from a global exception type SpinLock g_StackTraceArrayLock; -void ExceptionObject::SetStackTrace(StackTraceArray const & stackTrace, PTRARRAYREF dynamicMethodArray) +void ExceptionObject::SetStackTrace(I1ARRAYREF stackTrace, PTRARRAYREF dynamicMethodArray) { CONTRACTL { - GC_NOTRIGGER; - NOTHROW; + THROWS; + GC_TRIGGERS; MODE_COOPERATIVE; } CONTRACTL_END; - Thread *m_pThread = GetThread(); - SpinLock::AcquireLock(&g_StackTraceArrayLock); + if (_stackTraceLock == NULL || GetMethodTable() == g_pOutOfMemoryExceptionClass) + { + // We can't activate the SyncBlock for an OutOfMemoryException, so use preallocated SpinLock + SpinLock::AcquireLock(&g_StackTraceArrayLock); - SetObjectReference((OBJECTREF*)&_stackTrace, (OBJECTREF)stackTrace.Get()); - SetObjectReference((OBJECTREF*)&_dynamicMethods, (OBJECTREF)dynamicMethodArray); + SetObjectReference((OBJECTREF*)&_stackTrace, (OBJECTREF)stackTrace); + SetObjectReference((OBJECTREF*)&_dynamicMethods, (OBJECTREF)dynamicMethodArray); - SpinLock::ReleaseLock(&g_StackTraceArrayLock); + SpinLock::ReleaseLock(&g_StackTraceArrayLock); + } + else + { + struct _gc + { + EXCEPTIONREF throwable; + OBJECTREF dynamicMethodArray; + OBJECTREF stateLock; + OBJECTREF trace; + }; + _gc gc; + gc.throwable = (EXCEPTIONREF)this; + gc.dynamicMethodArray = (OBJECTREF)dynamicMethodArray; + gc.stateLock = _stackTraceLock; + gc.trace = stackTrace; + GCPROTECT_BEGIN(gc); + + // Aquire lock + gc.stateLock->EnterObjMonitor(); + + SetObjectReference((OBJECTREF*)&gc.throwable->_stackTrace, gc.trace); + SetObjectReference((OBJECTREF*)&gc.throwable->_dynamicMethods, gc.dynamicMethodArray); + + // Release lock + gc.stateLock->LeaveObjMonitor(); + GCPROTECT_END(); + } } -void ExceptionObject::SetNullStackTrace() -{ +void ExceptionObject::GetStackTrace(StackTraceArray& stackTrace, PTRARRAYREF* outDynamicMethodArray /*= NULL*/) +{ CONTRACTL { - GC_NOTRIGGER; - NOTHROW; + THROWS; + GC_TRIGGERS; MODE_COOPERATIVE; } CONTRACTL_END; - Thread *m_pThread = GetThread(); - SpinLock::AcquireLock(&g_StackTraceArrayLock); + if (_stackTraceLock == NULL || GetMethodTable() == g_pOutOfMemoryExceptionClass) + { + // We can't activate the SyncBlock for an OutOfMemoryException, so use preallocated SpinLock + SpinLock::AcquireLock(&g_StackTraceArrayLock); - I1ARRAYREF stackTraceArray = NULL; - PTRARRAYREF dynamicMethodArray = NULL; + StackTraceArray temp(_stackTrace); + stackTrace.Swap(temp); - SetObjectReference((OBJECTREF*)&_stackTrace, (OBJECTREF)stackTraceArray); - SetObjectReference((OBJECTREF*)&_dynamicMethods, (OBJECTREF)dynamicMethodArray); + if (outDynamicMethodArray != NULL) + { + *outDynamicMethodArray = _dynamicMethods; + } - SpinLock::ReleaseLock(&g_StackTraceArrayLock); -} + SpinLock::ReleaseLock(&g_StackTraceArrayLock); + } + else + { + struct _gc + { + EXCEPTIONREF throwable; + OBJECTREF stateLock; + }; + _gc gc; + gc.throwable = (EXCEPTIONREF)this; + gc.stateLock = _stackTraceLock; + GCPROTECT_BEGIN(gc); -#endif // !defined(DACCESS_COMPILE) + // Aquire lock + gc.stateLock->EnterObjMonitor(); + + StackTraceArray temp(gc.throwable->_stackTrace); + stackTrace.Swap(temp); -void ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outDynamicMethodArray /*= NULL*/) const + if (outDynamicMethodArray != NULL) + { + *outDynamicMethodArray = gc.throwable->_dynamicMethods; + } + + // Release lock + gc.stateLock->LeaveObjMonitor(); + + GCPROTECT_END(); + } +} +#else +void ExceptionObject::GetStackTrace(StackTraceArray& stackTrace, PTRARRAYREF* outDynamicMethodArray /*= NULL*/) const { CONTRACTL { @@ -2026,11 +2086,6 @@ void ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * } CONTRACTL_END; -#if !defined(DACCESS_COMPILE) - Thread *m_pThread = GetThread(); - SpinLock::AcquireLock(&g_StackTraceArrayLock); -#endif // !defined(DACCESS_COMPILE) - StackTraceArray temp(_stackTrace); stackTrace.Swap(temp); @@ -2038,12 +2093,8 @@ void ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * { *outDynamicMethodArray = _dynamicMethods; } - -#if !defined(DACCESS_COMPILE) - SpinLock::ReleaseLock(&g_StackTraceArrayLock); -#endif // !defined(DACCESS_COMPILE) - } +#endif // !defined(DACCESS_COMPILE) bool LAHashDependentHashTrackerObject::IsLoaderAllocatorLive() { diff --git a/src/coreclr/src/vm/object.h b/src/coreclr/src/vm/object.h index ae2d2e9..e6b194b 100644 --- a/src/coreclr/src/vm/object.h +++ b/src/coreclr/src/vm/object.h @@ -2506,10 +2506,7 @@ public: return _xptrs; } - void SetStackTrace(StackTraceArray const & stackTrace, PTRARRAYREF dynamicMethodArray); - void SetNullStackTrace(); - - void GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outDynamicMethodArray = NULL) const; + void SetStackTrace(I1ARRAYREF stackTrace, PTRARRAYREF dynamicMethodArray); #ifdef DACCESS_COMPILE I1ARRAYREF GetStackTraceArrayObject() const @@ -2517,6 +2514,9 @@ public: LIMITED_METHOD_DAC_CONTRACT; return _stackTrace; } + void GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outDynamicMethodArray = NULL) const; +#else + void GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * outDynamicMethodArray = NULL); #endif // DACCESS_COMPILE void SetInnerException(OBJECTREF innerException) @@ -2673,6 +2673,7 @@ private: STRINGREF _remoteStackTraceString; PTRARRAYREF _dynamicMethods; STRINGREF _source; // Mainly used by VB. + OBJECTREF _stackTraceLock; // Lock used to access stacktrace from an exception object UINT_PTR _ipForWatsonBuckets; // Contains the IP of exception for watson bucketing void* _xptrs; -- 2.7.4