From 100333f74ad6a4e4eb0cd2ce84de81990573bd08 Mon Sep 17 00:00:00 2001 From: Mike McLaughlin Date: Tue, 7 Jan 2020 17:25:19 -0800 Subject: [PATCH] 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. The changes for cases #1 and 2 start with StackTraceInfo/StackTraceElement structs (adding the "fAdjustOffset" field) when the stack trace for an exception is generated and is put in the private _stackTrace Exception object field. When the stack trace for an exception is rendered to a string (via the GetStackFramesInternal FCALL) the internal GetStackFramesData/DebugStackTraceElement structs are initialized and the new fAdjustOffset field there is set from the one in StackTraceElement. That is where the IL offset is calculated and this flag is used to decrement the native offset enough to point to actual instruction that caused the exception. 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 c715c54cbb6ae..fdb95a97db91c 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 9cb271b840a2d..a4833f95d5339 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 e8334ff85545b..d807896887342 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 ab1bde1c8c180..a93ea5722d832 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 37e85b28d00c5..a614b299b0cbb 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 77f62647e6477..e9a28f509d8d8 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(); }