From ba2662a9837cd90463e4f10569a141893cfa91a4 Mon Sep 17 00:00:00 2001 From: Mike McLaughlin Date: Tue, 7 Jan 2020 17:25:19 -0800 Subject: [PATCH 1/5] Fix unhandled exception line number issues There are a few paths to get the place (DebugStackTrace::DebugStackTraceElement::InitPass2) where the offset/ip needs an adjustment: 1) The System.Diagnostics.StackTrace constructors that take an exception object. The stack trace in the exception object is used (from the _stackTrace field). 2) Processing an unhandled exception for display (ExceptionTracker::ProcessOSExceptionNotification). 3) The System.Diagnostics.StackTrace constructors that don't take an exception object that get the stack trace of the current thread. For cases #1 and #2 the StackTraceInfo/StackTraceElement structs are built when the stack trace for an exception is generated and is put in the private _stackTrace Exception object field. The IP in each StackTraceElement is decremented for hardware exceptions and not for software exceptions because the CrawlFrame isInterrupted/hasFaulted fields are not initialized (always false). This is backwards for h/w exceptions leaf node frames but really can't be changed to be compatible with other code in the runtime and SOS. The fIsLastFrameFromForeignStackTrace BOOL in the StackTraceElement/DebugStackTraceElement structs have been replaced with INT "flags" field defined by the StackTraceElementFlags enum. There is a new flag that is set (STEF_IP_ADJUSTED) if the IP has been already adjusted/decremented. This flag is used to adjust the native offset when it is converted to an IL offset for source/line number lookup in DebugStackTraceElement::InitPass2(). When the stack trace for an exception is rendered to a string (via the GetStackFramesInternal FCALL) the internal GetStackFramesData/DebugStackTraceElement structs are initialized. This new "flags" field is passed from the StackTraceElement to the DebugStackTraceElement struct. For case #3 all this happens in the GetStackFramesInternal FCALL called from the managed constructor building the GetStackFramesData/DebugStackTraceElement structs directly. Fixes issues #27765 and #25740. --- src/coreclr/src/vm/clrex.h | 3 ++ src/coreclr/src/vm/debugdebugger.cpp | 53 +++++++++++++++++++----- src/coreclr/src/vm/debugdebugger.h | 14 ++++--- src/coreclr/src/vm/excep.cpp | 7 +++- src/coreclr/src/vm/exceptionhandling.cpp | 12 +++++- src/coreclr/src/vm/stackwalk.h | 19 +++++++++ 6 files changed, 91 insertions(+), 17 deletions(-) diff --git a/src/coreclr/src/vm/clrex.h b/src/coreclr/src/vm/clrex.h index c715c54cbb6ae2..fdb95a97db91c2 100644 --- a/src/coreclr/src/vm/clrex.h +++ b/src/coreclr/src/vm/clrex.h @@ -30,6 +30,9 @@ struct StackTraceElement // TRUE if this element represents the last frame of the foreign // exception stack trace. BOOL fIsLastFrameFromForeignStackTrace; + // TRUE if the ip needs to be adjusted back to the calling or + // throwing instruction. + BOOL fAdjustOffset; bool operator==(StackTraceElement const & rhs) const { diff --git a/src/coreclr/src/vm/debugdebugger.cpp b/src/coreclr/src/vm/debugdebugger.cpp index 9cb271b840a2df..a4833f95d5339b 100644 --- a/src/coreclr/src/vm/debugdebugger.cpp +++ b/src/coreclr/src/vm/debugdebugger.cpp @@ -939,8 +939,9 @@ void DebugStackTrace::GetStackFramesHelper(Frame *pStartFrame, // Do a 2nd pass outside of any locks. // This will compute IL offsets. - for(INT32 i = 0; i < pData->cElements; i++) + for (INT32 i = 0; i < pData->cElements; i++) { + // pStartFrame is NULL when collecting frames for current thread. pData->pElements[i].InitPass2(); } @@ -1026,14 +1027,20 @@ StackWalkAction DebugStackTrace::GetStackFramesCallback(CrawlFrame* pCf, VOID* d dwNativeOffset = 0; } + // We can assume that dwNativeOffset points to an the instruction after [call IL_Throw] when these conditions are met: + // 1. !pCf->HasFaulted() - It wasn't a "hardware" exception (Access violation, dev by 0, etc.) + // 2. !pCf->IsIPadjusted() - It hasn't been previously adjusted to point to a call (like [call IL_Throw], etc.) + BOOL fAdjustOffset = !pCf->HasFaulted() && !pCf->IsIPadjusted(); + pData->pElements[pData->cElements].InitPass1( dwNativeOffset, pFunc, - ip); + ip, + FALSE, + fAdjustOffset); // We'll init the IL offsets outside the TSL lock. - ++pData->cElements; // Since we may be asynchronously walking another thread's stack, @@ -1134,9 +1141,12 @@ void DebugStackTrace::GetStackFramesFromException(OBJECTREF * e, dwNativeOffset = 0; } - pData->pElements[i].InitPass1(dwNativeOffset, pMD, (PCODE) cur.ip - , cur.fIsLastFrameFromForeignStackTrace - ); + pData->pElements[i].InitPass1( + dwNativeOffset, + pMD, + (PCODE)cur.ip, + cur.fIsLastFrameFromForeignStackTrace, + cur.fAdjustOffset); #ifndef DACCESS_COMPILE pData->pElements[i].InitPass2(); #endif @@ -1156,8 +1166,9 @@ void DebugStackTrace::GetStackFramesFromException(OBJECTREF * e, void DebugStackTrace::DebugStackTraceElement::InitPass1( DWORD dwNativeOffset, MethodDesc *pFunc, - PCODE ip - , BOOL fIsLastFrameFromForeignStackTrace /*= FALSE*/ + PCODE ip, + BOOL fIsLastFrameFromForeignStackTrace, /*= FALSE*/ + BOOL fAdjustOffset /*= FALSE*/ ) { LIMITED_METHOD_CONTRACT; @@ -1170,6 +1181,7 @@ void DebugStackTrace::DebugStackTraceElement::InitPass1( this->dwOffset = dwNativeOffset; this->ip = ip; this->fIsLastFrameFromForeignStackTrace = fIsLastFrameFromForeignStackTrace; + this->fAdjustOffset = fAdjustOffset; } #ifndef DACCESS_COMPILE @@ -1188,14 +1200,35 @@ void DebugStackTrace::DebugStackTraceElement::InitPass2() _ASSERTE(!ThreadStore::HoldingThreadStore()); - bool bRes = false; + bool bRes = false; #ifdef DEBUGGING_SUPPORTED // Calculate the IL offset using the debugging services if ((this->ip != NULL) && g_pDebugInterface) { + // To get the source line number of the actual code that threw an exception, the dwOffset needs to be + // adjusted in certain cases when calculating the IL offset. + // + // The dwOffset of the stack frame points to either: + // + // 1) The instruction that caused a hardware exception (div by zero, null ref, etc). + // 2) The instruction after the call to an internal runtime function (FCALL like IL_Throw, IL_Rethrow, + // JIT_OverFlow, etc.) that caused a software exception. + // 3) The instruction after the call to a managed function (non-leaf node). + // + // #2 and #3 are the cases that need to adjust dwOffset because they point after the call instructionand + // may point to the next (incorrect) IL instruction/source line. fAdjustOffset is false if the dwOffset/ip + // has already been adjusted or is case #1. fAdjustOffset is true if the dwOffset/IP hasn't been already + // adjusted for cases #2 or #3. + // + // When the dwOffset needs to be adjusted it is a lot simpler to decrement instead of trying to figure out + // the beginning of the instruction. It is enough for GetILOffsetFromNative to return the IL offset of the + // instruction throwing the exception. bRes = g_pDebugInterface->GetILOffsetFromNative( - pFunc, (LPCBYTE) this->ip, this->dwOffset, &this->dwILOffset); + pFunc, + (LPCBYTE)this->ip, + this->fAdjustOffset && this->dwOffset >= STACKWALK_CONTROLPC_ADJUST_OFFSET ? this->dwOffset - STACKWALK_CONTROLPC_ADJUST_OFFSET : this->dwOffset, + &this->dwILOffset); } #endif // !DEBUGGING_SUPPORTED diff --git a/src/coreclr/src/vm/debugdebugger.h b/src/coreclr/src/vm/debugdebugger.h index e8334ff85545b3..d807896887342e 100644 --- a/src/coreclr/src/vm/debugdebugger.h +++ b/src/coreclr/src/vm/debugdebugger.h @@ -102,16 +102,20 @@ class DebugStackTrace PCODE ip; // TRUE if this element represents the last frame of the foreign // exception stack trace. - BOOL fIsLastFrameFromForeignStackTrace; + BOOL fIsLastFrameFromForeignStackTrace; + // TRUE if the dwOffset (native offset) needs to be adjusted back to the + // calling or throwing instruction. + BOOL fAdjustOffset; // Initialization done under TSL. // This is used when first collecting the stack frame data. void InitPass1( DWORD dwNativeOffset, MethodDesc *pFunc, - PCODE ip - , BOOL fIsLastFrameFromForeignStackTrace = FALSE - ); + PCODE ip, + BOOL fIsLastFrameFromForeignStackTrace = FALSE, + BOOL fAdjustOffset = FALSE + ); // Initialization done outside the TSL. // This will init the dwILOffset field (and potentially anything else @@ -131,7 +135,7 @@ class DebugStackTrace DebugStackTraceElement* pElements; THREADBASEREF TargetThread; AppDomain *pDomain; - BOOL fDoWeHaveAnyFramesFromForeignStackTrace; + BOOL fDoWeHaveAnyFramesFromForeignStackTrace; GetStackFramesData() : skip(0), diff --git a/src/coreclr/src/vm/excep.cpp b/src/coreclr/src/vm/excep.cpp index ab1bde1c8c180b..a93ea5722d832f 100644 --- a/src/coreclr/src/vm/excep.cpp +++ b/src/coreclr/src/vm/excep.cpp @@ -3557,10 +3557,15 @@ BOOL StackTraceInfo::AppendElement(BOOL bAllowAllocMem, UINT_PTR currentIP, UINT // the current exception). Hence, set the corresponding flag to FALSE. pStackTraceElem->fIsLastFrameFromForeignStackTrace = FALSE; + // We can assume that IP points to an the instruction after [call IL_Throw] when these conditions are met: + // 1. !pCf->HasFaulted() - It wasn't a "hardware" exception (Access violation, dev by 0, etc.) + // 2. !pCf->IsIPadjusted() - It hasn't been previously adjusted to point to the call (i.e. like [call IL_Throw], etc.) + pStackTraceElem->fAdjustOffset = !pCf->HasFaulted() && !pCf->IsIPadjusted(); + // 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->HasFaulted() || pCf->IsIPadjusted()) && pStackTraceElem->ip != 0) + if (!pStackTraceElem->fAdjustOffset && pStackTraceElem->ip != 0) { pStackTraceElem->ip -= 1; } diff --git a/src/coreclr/src/vm/exceptionhandling.cpp b/src/coreclr/src/vm/exceptionhandling.cpp index 37e85b28d00c5c..a614b299b0cbb6 100644 --- a/src/coreclr/src/vm/exceptionhandling.cpp +++ b/src/coreclr/src/vm/exceptionhandling.cpp @@ -1333,7 +1333,12 @@ void ExceptionTracker::InitializeCrawlFrameForExplicitFrame(CrawlFrame* pcfThisF INDEBUG(memset(pcfThisFrame, 0xCC, sizeof(*pcfThisFrame))); + // Clear various flags because the above INDEBUG sets them to true pcfThisFrame->isFrameless = false; + pcfThisFrame->isInterrupted = false; + pcfThisFrame->hasFaulted = false; + pcfThisFrame->isIPadjusted = false; + pcfThisFrame->pFrame = pFrame; pcfThisFrame->pFunc = pFrame->GetFunction(); @@ -1417,6 +1422,12 @@ void ExceptionTracker::InitializeCrawlFrame(CrawlFrame* pcfThisFrame, Thread* pT INDEBUG(memset(pcfThisFrame, 0xCC, sizeof(*pcfThisFrame))); pcfThisFrame->pRD = pRD; + // Clear various flags because the above INDEBUG sets them to true + pcfThisFrame->pFunc = NULL; + pcfThisFrame->isInterrupted = false; + pcfThisFrame->hasFaulted = false; + pcfThisFrame->isIPadjusted = false; + #ifdef FEATURE_INTERPRETER pcfThisFrame->pFrame = NULL; #endif // FEATURE_INTERPRETER @@ -1571,7 +1582,6 @@ void ExceptionTracker::InitializeCrawlFrame(CrawlFrame* pcfThisFrame, Thread* pT } pcfThisFrame->pThread = pThread; - pcfThisFrame->hasFaulted = false; Frame* pTopFrame = pThread->GetFrame(); pcfThisFrame->isIPadjusted = (FRAME_TOP != pTopFrame) && (pTopFrame->GetVTablePtr() != FaultingExceptionFrame::GetMethodFrameVPtr()); diff --git a/src/coreclr/src/vm/stackwalk.h b/src/coreclr/src/vm/stackwalk.h index 77f62647e64775..e9a28f509d8d83 100644 --- a/src/coreclr/src/vm/stackwalk.h +++ b/src/coreclr/src/vm/stackwalk.h @@ -102,6 +102,7 @@ class CrawlFrame inline Frame* GetFrame() // will return NULL for "frameless methods" { LIMITED_METHOD_DAC_CONTRACT; + _ASSERTE((int)isFrameless != 0xcc); if (isFrameless) return NULL; @@ -173,6 +174,7 @@ class CrawlFrame inline bool IsFrameless() { LIMITED_METHOD_DAC_CONTRACT; + _ASSERTE((int)isFrameless != 0xcc); return isFrameless; } @@ -183,6 +185,7 @@ class CrawlFrame inline bool IsActiveFrame() { LIMITED_METHOD_DAC_CONTRACT; + _ASSERTE((int)isFirst != 0xcc); return isFirst; } @@ -193,6 +196,7 @@ class CrawlFrame inline bool IsActiveFunc() { LIMITED_METHOD_DAC_CONTRACT; + _ASSERTE((int)isFirst != 0xcc); return (pFunc && isFirst); } @@ -204,6 +208,7 @@ class CrawlFrame bool IsInterrupted() { LIMITED_METHOD_DAC_CONTRACT; + _ASSERTE((int)isInterrupted != 0xcc); return (pFunc && isInterrupted /* && isFrameless?? */); } @@ -214,6 +219,7 @@ class CrawlFrame bool HasFaulted() { LIMITED_METHOD_DAC_CONTRACT; + _ASSERTE((int)hasFaulted != 0xcc); return (pFunc && hasFaulted /* && isFrameless?? */); } @@ -225,6 +231,8 @@ class CrawlFrame bool IsNativeMarker() { LIMITED_METHOD_DAC_CONTRACT; + _ASSERTE((int)isNativeMarker != 0xcc); + return isNativeMarker; } @@ -239,6 +247,8 @@ class CrawlFrame bool IsNoFrameTransition() { LIMITED_METHOD_DAC_CONTRACT; + _ASSERTE((int)isNoFrameTransition != 0xcc); + return isNoFrameTransition; } @@ -249,6 +259,8 @@ class CrawlFrame TADDR GetNoFrameTransitionMarker() { LIMITED_METHOD_DAC_CONTRACT; + _ASSERTE((int)isNoFrameTransition != 0xcc); + return (isNoFrameTransition ? taNoFrameTransitionMarker : NULL); } @@ -259,6 +271,7 @@ class CrawlFrame bool IsIPadjusted() { LIMITED_METHOD_DAC_CONTRACT; + _ASSERTE((int)isIPadjusted != 0xcc); return (pFunc && isIPadjusted /* && isFrameless?? */); } @@ -336,6 +349,7 @@ class CrawlFrame GCInfoToken GetGCInfoToken() { LIMITED_METHOD_DAC_CONTRACT; + _ASSERTE((int)isFrameless != 0xcc); _ASSERTE(isFrameless); return codeInfo.GetGCInfoToken(); } @@ -343,6 +357,7 @@ class CrawlFrame PTR_VOID GetGCInfo() { LIMITED_METHOD_DAC_CONTRACT; + _ASSERTE((int)isFrameless != 0xcc); _ASSERTE(isFrameless); return codeInfo.GetGCInfo(); } @@ -350,6 +365,7 @@ class CrawlFrame const METHODTOKEN& GetMethodToken() { LIMITED_METHOD_DAC_CONTRACT; + _ASSERTE((int)isFrameless != 0xcc); _ASSERTE(isFrameless); return codeInfo.GetMethodToken(); } @@ -357,6 +373,7 @@ class CrawlFrame unsigned GetRelOffset() { LIMITED_METHOD_DAC_CONTRACT; + _ASSERTE((int)isFrameless != 0xcc); _ASSERTE(isFrameless); return codeInfo.GetRelOffset(); } @@ -364,6 +381,7 @@ class CrawlFrame IJitManager* GetJitManager() { LIMITED_METHOD_DAC_CONTRACT; + _ASSERTE((int)isFrameless != 0xcc); _ASSERTE(isFrameless); return codeInfo.GetJitManager(); } @@ -371,6 +389,7 @@ class CrawlFrame ICodeManager* GetCodeManager() { LIMITED_METHOD_DAC_CONTRACT; + _ASSERTE((int)isFrameless != 0xcc); _ASSERTE(isFrameless); return codeInfo.GetCodeManager(); } From 2d7156c4124aec1dc50b044fd5248a13d3386a24 Mon Sep 17 00:00:00 2001 From: Mike McLaughlin Date: Thu, 23 Jan 2020 13:34:01 -0800 Subject: [PATCH 2/5] Code review feedback --- src/coreclr/src/debug/daccess/dacdbiimpl.cpp | 2 +- src/coreclr/src/vm/clrex.h | 20 ++++++++---- src/coreclr/src/vm/debugdebugger.cpp | 32 ++++++++------------ src/coreclr/src/vm/debugdebugger.h | 12 ++------ src/coreclr/src/vm/excep.cpp | 12 +++----- src/coreclr/src/vm/stackwalk.cpp | 2 +- 6 files changed, 36 insertions(+), 44 deletions(-) diff --git a/src/coreclr/src/debug/daccess/dacdbiimpl.cpp b/src/coreclr/src/debug/daccess/dacdbiimpl.cpp index 8f46469816faaf..2bf8efaee3c195 100644 --- a/src/coreclr/src/debug/daccess/dacdbiimpl.cpp +++ b/src/coreclr/src/debug/daccess/dacdbiimpl.cpp @@ -3633,7 +3633,7 @@ void DacDbiInterfaceImpl::GetStackFramesFromException(VMPTR_Object vmObject, Dac currentFrame.vmDomainFile.SetHostPtr(pDomainFile); currentFrame.ip = currentElement.ip; currentFrame.methodDef = currentElement.pFunc->GetMemberDef(); - currentFrame.isLastForeignExceptionFrame = currentElement.fIsLastFrameFromForeignStackTrace; + currentFrame.isLastForeignExceptionFrame = (currentElement.flags & STEF_LAST_FRAME_FROM_FOREIGN_STACK_TRACE) != 0; } } } diff --git a/src/coreclr/src/vm/clrex.h b/src/coreclr/src/vm/clrex.h index fdb95a97db91c2..14f591a865b064 100644 --- a/src/coreclr/src/vm/clrex.h +++ b/src/coreclr/src/vm/clrex.h @@ -22,17 +22,23 @@ class AssemblySpec; class PEFile; class PEAssembly; +enum StackTraceElementFlags +{ + // Set if this element represents the last frame of the foreign exception stack trace + STEF_LAST_FRAME_FROM_FOREIGN_STACK_TRACE = 0x0001, + + // Set if the "ip" field has already been adjusted (decremented) + STEF_IP_ADJUSTED = 0x0002, +}; + +// This struct is used by SOS in the diagnostic repo and needs to remain backwards compatible. +// See: https://github.com/dotnet/diagnostics/blob/master/src/SOS/Strike/strike.cpp#L2245 struct StackTraceElement { UINT_PTR ip; UINT_PTR sp; PTR_MethodDesc pFunc; - // TRUE if this element represents the last frame of the foreign - // exception stack trace. - BOOL fIsLastFrameFromForeignStackTrace; - // TRUE if the ip needs to be adjusted back to the calling or - // throwing instruction. - BOOL fAdjustOffset; + INT flags; // This is StackTraceElementFlags but it needs to always be "int" sized for compatibility with SOS. bool operator==(StackTraceElement const & rhs) const { @@ -47,6 +53,8 @@ struct StackTraceElement } }; +// This struct is used by SOS in the diagnostic repo and needs to remain backwards compatible. +// See: https://github.com/dotnet/diagnostics/blob/master/src/SOS/Strike/strike.cpp#L2669 class StackTraceInfo { private: diff --git a/src/coreclr/src/vm/debugdebugger.cpp b/src/coreclr/src/vm/debugdebugger.cpp index a4833f95d5339b..a0fdfa74885b6f 100644 --- a/src/coreclr/src/vm/debugdebugger.cpp +++ b/src/coreclr/src/vm/debugdebugger.cpp @@ -510,7 +510,7 @@ FCIMPL4(void, DebugStackTrace::GetStackFramesInternal, // Set the BOOL indicating if the frame represents the last frame from a foreign exception stack trace. U1 *pIsLastFrameFromForeignExceptionStackTraceU1 = (U1 *)((BOOLARRAYREF)pStackFrameHelper->rgiLastFrameFromForeignExceptionStackTrace) ->GetDirectPointerToNonObjectElements(); - pIsLastFrameFromForeignExceptionStackTraceU1 [iNumValidFrames] = (U1) data.pElements[i].fIsLastFrameFromForeignStackTrace; + pIsLastFrameFromForeignExceptionStackTraceU1 [iNumValidFrames] = (U1)(data.pElements[i].flags & STEF_LAST_FRAME_FROM_FOREIGN_STACK_TRACE); } MethodDesc *pMethod = data.pElements[i].pFunc; @@ -1027,17 +1027,14 @@ StackWalkAction DebugStackTrace::GetStackFramesCallback(CrawlFrame* pCf, VOID* d dwNativeOffset = 0; } - // We can assume that dwNativeOffset points to an the instruction after [call IL_Throw] when these conditions are met: - // 1. !pCf->HasFaulted() - It wasn't a "hardware" exception (Access violation, dev by 0, etc.) - // 2. !pCf->IsIPadjusted() - It hasn't been previously adjusted to point to a call (like [call IL_Throw], etc.) - BOOL fAdjustOffset = !pCf->HasFaulted() && !pCf->IsIPadjusted(); + // Pass on to InitPass2 that the IP has already been adjusted (decremented by 1) + INT flags = pCf->IsIPadjusted() ? STEF_IP_ADJUSTED : 0; pData->pElements[pData->cElements].InitPass1( dwNativeOffset, pFunc, ip, - FALSE, - fAdjustOffset); + flags); // We'll init the IL offsets outside the TSL lock. @@ -1116,7 +1113,7 @@ void DebugStackTrace::GetStackFramesFromException(OBJECTREF * e, // If we come across any frame representing foreign exception stack trace, // then set the flag indicating so. This will be used to allocate the // corresponding array in StackFrameHelper. - if (cur.fIsLastFrameFromForeignStackTrace) + if ((cur.flags & STEF_LAST_FRAME_FROM_FOREIGN_STACK_TRACE) != 0) { pData->fDoWeHaveAnyFramesFromForeignStackTrace = TRUE; } @@ -1145,8 +1142,7 @@ void DebugStackTrace::GetStackFramesFromException(OBJECTREF * e, dwNativeOffset, pMD, (PCODE)cur.ip, - cur.fIsLastFrameFromForeignStackTrace, - cur.fAdjustOffset); + cur.flags); #ifndef DACCESS_COMPILE pData->pElements[i].InitPass2(); #endif @@ -1167,8 +1163,7 @@ void DebugStackTrace::DebugStackTraceElement::InitPass1( DWORD dwNativeOffset, MethodDesc *pFunc, PCODE ip, - BOOL fIsLastFrameFromForeignStackTrace, /*= FALSE*/ - BOOL fAdjustOffset /*= FALSE*/ + INT flags ) { LIMITED_METHOD_CONTRACT; @@ -1180,8 +1175,7 @@ void DebugStackTrace::DebugStackTraceElement::InitPass1( this->pFunc = pFunc; this->dwOffset = dwNativeOffset; this->ip = ip; - this->fIsLastFrameFromForeignStackTrace = fIsLastFrameFromForeignStackTrace; - this->fAdjustOffset = fAdjustOffset; + this->flags = flags; } #ifndef DACCESS_COMPILE @@ -1216,18 +1210,18 @@ void DebugStackTrace::DebugStackTraceElement::InitPass2() // JIT_OverFlow, etc.) that caused a software exception. // 3) The instruction after the call to a managed function (non-leaf node). // - // #2 and #3 are the cases that need to adjust dwOffset because they point after the call instructionand - // may point to the next (incorrect) IL instruction/source line. fAdjustOffset is false if the dwOffset/ip - // has already been adjusted or is case #1. fAdjustOffset is true if the dwOffset/IP hasn't been already - // adjusted for cases #2 or #3. + // #2 and #3 are the cases that need to adjust dwOffset because they point after the call instruction + // and may point to the next (incorrect) IL instruction/source line. If STEF_IP_ADJUSTED is set, + // IP/dwOffset has already be decremented so don't decrement it again. // // When the dwOffset needs to be adjusted it is a lot simpler to decrement instead of trying to figure out // the beginning of the instruction. It is enough for GetILOffsetFromNative to return the IL offset of the // instruction throwing the exception. + bool fAdjustOffset = (this->flags & STEF_IP_ADJUSTED) == 0 && this->dwOffset >= STACKWALK_CONTROLPC_ADJUST_OFFSET; bRes = g_pDebugInterface->GetILOffsetFromNative( pFunc, (LPCBYTE)this->ip, - this->fAdjustOffset && this->dwOffset >= STACKWALK_CONTROLPC_ADJUST_OFFSET ? this->dwOffset - STACKWALK_CONTROLPC_ADJUST_OFFSET : this->dwOffset, + fAdjustOffset ? this->dwOffset - STACKWALK_CONTROLPC_ADJUST_OFFSET : this->dwOffset, &this->dwILOffset); } diff --git a/src/coreclr/src/vm/debugdebugger.h b/src/coreclr/src/vm/debugdebugger.h index d807896887342e..92a5f601d08b26 100644 --- a/src/coreclr/src/vm/debugdebugger.h +++ b/src/coreclr/src/vm/debugdebugger.h @@ -96,16 +96,11 @@ class DebugStackTrace private: #endif // DACCESS_COMPILE struct DebugStackTraceElement { - DWORD dwOffset; // native offset + DWORD dwOffset; // native offset DWORD dwILOffset; MethodDesc *pFunc; PCODE ip; - // TRUE if this element represents the last frame of the foreign - // exception stack trace. - BOOL fIsLastFrameFromForeignStackTrace; - // TRUE if the dwOffset (native offset) needs to be adjusted back to the - // calling or throwing instruction. - BOOL fAdjustOffset; + INT flags; // StackStackElementFlags // Initialization done under TSL. // This is used when first collecting the stack frame data. @@ -113,8 +108,7 @@ class DebugStackTrace DWORD dwNativeOffset, MethodDesc *pFunc, PCODE ip, - BOOL fIsLastFrameFromForeignStackTrace = FALSE, - BOOL fAdjustOffset = FALSE + INT flags // StackStackElementFlags ); // Initialization done outside the TSL. diff --git a/src/coreclr/src/vm/excep.cpp b/src/coreclr/src/vm/excep.cpp index a93ea5722d832f..16fe93e235b389 100644 --- a/src/coreclr/src/vm/excep.cpp +++ b/src/coreclr/src/vm/excep.cpp @@ -2367,7 +2367,7 @@ void StackTraceInfo::SaveStackTrace(BOOL bAllowAllocMem, OBJECTHANDLE hThrowable // "numCurrentFrames" can be zero if the user created an EDI using // an unthrown exception. StackTraceElement & refLastElementFromForeignStackTrace = gc.stackTrace[numCurrentFrames - 1]; - refLastElementFromForeignStackTrace.fIsLastFrameFromForeignStackTrace = TRUE; + refLastElementFromForeignStackTrace.flags |= STEF_LAST_FRAME_FROM_FOREIGN_STACK_TRACE; } } @@ -3555,19 +3555,15 @@ BOOL StackTraceInfo::AppendElement(BOOL bAllowAllocMem, UINT_PTR currentIP, UINT // 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->fIsLastFrameFromForeignStackTrace = FALSE; - - // We can assume that IP points to an the instruction after [call IL_Throw] when these conditions are met: - // 1. !pCf->HasFaulted() - It wasn't a "hardware" exception (Access violation, dev by 0, etc.) - // 2. !pCf->IsIPadjusted() - It hasn't been previously adjusted to point to the call (i.e. like [call IL_Throw], etc.) - pStackTraceElem->fAdjustOffset = !pCf->HasFaulted() && !pCf->IsIPadjusted(); + pStackTraceElem->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 (!pStackTraceElem->fAdjustOffset && pStackTraceElem->ip != 0) + if (!(pCf->HasFaulted() || pCf->IsIPadjusted()) && pStackTraceElem->ip != 0) { pStackTraceElem->ip -= 1; + pStackTraceElem->flags |= STEF_IP_ADJUSTED; } ++m_dFrameCount; diff --git a/src/coreclr/src/vm/stackwalk.cpp b/src/coreclr/src/vm/stackwalk.cpp index 1e475b02ae8745..a81667157b77e0 100644 --- a/src/coreclr/src/vm/stackwalk.cpp +++ b/src/coreclr/src/vm/stackwalk.cpp @@ -1499,7 +1499,7 @@ void StackFrameIterator::ResetCrawlFrame() m_crawl.isFirst = true; m_crawl.isInterrupted = false; m_crawl.hasFaulted = false; - m_crawl.isIPadjusted = false; // can be removed + m_crawl.isIPadjusted = false; m_crawl.isNativeMarker = false; m_crawl.isProfilerDoStackSnapshot = !!(this->m_flags & PROFILER_DO_STACK_SNAPSHOT); From d7f0c507204e38009b511e4fb157d16ae1a5c197 Mon Sep 17 00:00:00 2001 From: Mike McLaughlin Date: Thu, 23 Jan 2020 21:05:57 -0800 Subject: [PATCH 3/5] Fix IL offset map search --- src/coreclr/src/debug/daccess/task.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/coreclr/src/debug/daccess/task.cpp b/src/coreclr/src/debug/daccess/task.cpp index 63f7e27a64be0e..ac87333e6f1ef0 100644 --- a/src/coreclr/src/debug/daccess/task.cpp +++ b/src/coreclr/src/debug/daccess/task.cpp @@ -4091,9 +4091,11 @@ ClrDataMethodInstance::GetILOffsetsByAddress( for (ULONG32 i = 0; i < numMap; i++) { if (codeOffset >= map[i].nativeStartOffset && - (((LONG)map[i].ilOffset == ICorDebugInfo::EPILOG && - !map[i].nativeEndOffset) || - codeOffset < map[i].nativeEndOffset)) + // Found the entry if it is the epilog or the last entry and nativeEndOffset == 0. For methods that don't + // have a epilog (i.e. IL_Throw is the last instruction) the last map entry has a valid IL offset (not EPILOG) + // and nativeEndOffset == 0. + ((((LONG)map[i].ilOffset == ICorDebugInfo::EPILOG || i == (numMap - 1)) && map[i].nativeEndOffset == 0) || + codeOffset < map[i].nativeEndOffset)) { hits++; From 6dd19d28a4c4f671e727bb1b39befb77f1e57191 Mon Sep 17 00:00:00 2001 From: Mike McLaughlin Date: Mon, 27 Jan 2020 20:56:15 -0800 Subject: [PATCH 4/5] Code review feedback --- src/coreclr/src/vm/clrex.h | 6 +++--- src/coreclr/src/vm/exceptionhandling.cpp | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/coreclr/src/vm/clrex.h b/src/coreclr/src/vm/clrex.h index 14f591a865b064..943e5fb59f22b1 100644 --- a/src/coreclr/src/vm/clrex.h +++ b/src/coreclr/src/vm/clrex.h @@ -31,14 +31,14 @@ enum StackTraceElementFlags STEF_IP_ADJUSTED = 0x0002, }; -// This struct is used by SOS in the diagnostic repo and needs to remain backwards compatible. +// This struct is used by SOS in the diagnostic repo. // See: https://github.com/dotnet/diagnostics/blob/master/src/SOS/Strike/strike.cpp#L2245 struct StackTraceElement { UINT_PTR ip; UINT_PTR sp; PTR_MethodDesc pFunc; - INT flags; // This is StackTraceElementFlags but it needs to always be "int" sized for compatibility with SOS. + INT flags; // This is StackTraceElementFlags but it needs to be "int" sized for compatibility with SOS. bool operator==(StackTraceElement const & rhs) const { @@ -53,7 +53,7 @@ struct StackTraceElement } }; -// This struct is used by SOS in the diagnostic repo and needs to remain backwards compatible. +// This struct is used by SOS in the diagnostic repo. // See: https://github.com/dotnet/diagnostics/blob/master/src/SOS/Strike/strike.cpp#L2669 class StackTraceInfo { diff --git a/src/coreclr/src/vm/exceptionhandling.cpp b/src/coreclr/src/vm/exceptionhandling.cpp index a614b299b0cbb6..f7fccb89b882ff 100644 --- a/src/coreclr/src/vm/exceptionhandling.cpp +++ b/src/coreclr/src/vm/exceptionhandling.cpp @@ -1333,7 +1333,7 @@ void ExceptionTracker::InitializeCrawlFrameForExplicitFrame(CrawlFrame* pcfThisF INDEBUG(memset(pcfThisFrame, 0xCC, sizeof(*pcfThisFrame))); - // Clear various flags because the above INDEBUG sets them to true + // Clear various flags pcfThisFrame->isFrameless = false; pcfThisFrame->isInterrupted = false; pcfThisFrame->hasFaulted = false; @@ -1422,7 +1422,7 @@ void ExceptionTracker::InitializeCrawlFrame(CrawlFrame* pcfThisFrame, Thread* pT INDEBUG(memset(pcfThisFrame, 0xCC, sizeof(*pcfThisFrame))); pcfThisFrame->pRD = pRD; - // Clear various flags because the above INDEBUG sets them to true + // Clear various flags pcfThisFrame->pFunc = NULL; pcfThisFrame->isInterrupted = false; pcfThisFrame->hasFaulted = false; From 41492d01bfad447dacdde162e73159f5800caaaf Mon Sep 17 00:00:00 2001 From: Mike McLaughlin Date: Thu, 30 Jan 2020 13:20:12 -0800 Subject: [PATCH 5/5] Code review feedback --- src/coreclr/src/vm/clrex.h | 4 ++-- src/coreclr/src/vm/debugdebugger.cpp | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/coreclr/src/vm/clrex.h b/src/coreclr/src/vm/clrex.h index 943e5fb59f22b1..34ca1c0823dd8c 100644 --- a/src/coreclr/src/vm/clrex.h +++ b/src/coreclr/src/vm/clrex.h @@ -32,7 +32,7 @@ enum StackTraceElementFlags }; // This struct is used by SOS in the diagnostic repo. -// See: https://github.com/dotnet/diagnostics/blob/master/src/SOS/Strike/strike.cpp#L2245 +// See: https://github.com/dotnet/diagnostics/blob/9ff35f13af2f03a68a166cfd53f1a4bb32425f2f/src/SOS/Strike/strike.cpp#L2245 struct StackTraceElement { UINT_PTR ip; @@ -54,7 +54,7 @@ struct StackTraceElement }; // This struct is used by SOS in the diagnostic repo. -// See: https://github.com/dotnet/diagnostics/blob/master/src/SOS/Strike/strike.cpp#L2669 +// See: https://github.com/dotnet/diagnostics/blob/9ff35f13af2f03a68a166cfd53f1a4bb32425f2f/src/SOS/Strike/strike.cpp#L2669 class StackTraceInfo { private: diff --git a/src/coreclr/src/vm/debugdebugger.cpp b/src/coreclr/src/vm/debugdebugger.cpp index a0fdfa74885b6f..862082ff75f7a2 100644 --- a/src/coreclr/src/vm/debugdebugger.cpp +++ b/src/coreclr/src/vm/debugdebugger.cpp @@ -941,7 +941,6 @@ void DebugStackTrace::GetStackFramesHelper(Frame *pStartFrame, // This will compute IL offsets. for (INT32 i = 0; i < pData->cElements; i++) { - // pStartFrame is NULL when collecting frames for current thread. pData->pElements[i].InitPass2(); }