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 dotnet#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 dotnet#3 all this happens in the GetStackFramesInternal FCALL called from the managed constructor
building the GetStackFramesData/DebugStackTraceElement structs directly.

Fixes issues dotnet#27765 and dotnet#25740.

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
     {

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

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),

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

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());

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,41 +349,47 @@ class CrawlFrame
     GCInfoToken GetGCInfoToken()
     {
         LIMITED_METHOD_DAC_CONTRACT;
+        _ASSERTE((int)isFrameless != 0xcc);
         _ASSERTE(isFrameless);
         return codeInfo.GetGCInfoToken();
     }
 
     PTR_VOID GetGCInfo()
     {
         LIMITED_METHOD_DAC_CONTRACT;
+        _ASSERTE((int)isFrameless != 0xcc);
         _ASSERTE(isFrameless);
         return codeInfo.GetGCInfo();
     }
 
     const METHODTOKEN& GetMethodToken()
     {
         LIMITED_METHOD_DAC_CONTRACT;
+        _ASSERTE((int)isFrameless != 0xcc);
         _ASSERTE(isFrameless);
         return codeInfo.GetMethodToken();
     }
 
     unsigned GetRelOffset()
     {
         LIMITED_METHOD_DAC_CONTRACT;
+        _ASSERTE((int)isFrameless != 0xcc);
         _ASSERTE(isFrameless);
         return codeInfo.GetRelOffset();
     }
 
     IJitManager*  GetJitManager()
     {
         LIMITED_METHOD_DAC_CONTRACT;
+        _ASSERTE((int)isFrameless != 0xcc);
         _ASSERTE(isFrameless);
         return codeInfo.GetJitManager();
     }
 
     ICodeManager* GetCodeManager()
     {
         LIMITED_METHOD_DAC_CONTRACT;
+        _ASSERTE((int)isFrameless != 0xcc);
         _ASSERTE(isFrameless);
         return codeInfo.GetCodeManager();
     }