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 dotnet#1 and dotnet#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 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();
     }