Fix unhandled exception line number issues (dotnet#2269)

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 dotnet#27765 and dotnet#25740.

Fix IL offset map search.

Author: Mike McLaughlin

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

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

src/coreclr/src/vm/clrex.h

@@ -22,14 +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.
+// See: https://github.com/dotnet/diagnostics/blob/9ff35f13af2f03a68a166cfd53f1a4bb32425f2f/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;
+    INT             flags;      // This is StackTraceElementFlags but it needs to be "int" sized for compatibility with SOS.
 
     bool operator==(StackTraceElement const & rhs) const
     {
@@ -44,6 +53,8 @@ struct StackTraceElement
     }
 };
 
+// This struct is used by SOS in the diagnostic repo.
+// See: https://github.com/dotnet/diagnostics/blob/9ff35f13af2f03a68a166cfd53f1a4bb32425f2f/src/SOS/Strike/strike.cpp#L2669
 class StackTraceInfo
 {
 private:

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;
@@ -939,7 +939,7 @@ 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++)
     {
         pData->pElements[i].InitPass2();
     }
@@ -1026,14 +1026,17 @@ StackWalkAction DebugStackTrace::GetStackFramesCallback(CrawlFrame* pCf, VOID* d
         dwNativeOffset = 0;
     }
 
+    // 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);
+            ip,
+            flags);
 
     // We'll init the IL offsets outside the TSL lock.
 
-
     ++pData->cElements;
 
     // Since we may be asynchronously walking another thread's stack,
@@ -1109,7 +1112,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;
                 }
@@ -1134,9 +1137,11 @@ 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.flags);
 #ifndef DACCESS_COMPILE
                 pData->pElements[i].InitPass2();
 #endif
@@ -1156,8 +1161,8 @@ void DebugStackTrace::GetStackFramesFromException(OBJECTREF * e,
 void DebugStackTrace::DebugStackTraceElement::InitPass1(
     DWORD dwNativeOffset,
     MethodDesc *pFunc,
-    PCODE ip
-    , BOOL fIsLastFrameFromForeignStackTrace /*= FALSE*/
+    PCODE ip,
+    INT flags
 )
 {
     LIMITED_METHOD_CONTRACT;
@@ -1169,7 +1174,7 @@ void DebugStackTrace::DebugStackTraceElement::InitPass1(
     this->pFunc = pFunc;
     this->dwOffset = dwNativeOffset;
     this->ip = ip;
-    this->fIsLastFrameFromForeignStackTrace = fIsLastFrameFromForeignStackTrace;
+    this->flags = flags;
 }
 
 #ifndef DACCESS_COMPILE
@@ -1188,14 +1193,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 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->dwOffset, &this->dwILOffset);
+            pFunc,
+            (LPCBYTE)this->ip,
+            fAdjustOffset ? this->dwOffset - STACKWALK_CONTROLPC_ADJUST_OFFSET : this->dwOffset,
+            &this->dwILOffset);
     }
 
 #endif // !DEBUGGING_SUPPORTED

src/coreclr/src/vm/debugdebugger.h

@@ -96,22 +96,20 @@ 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;
+        INT flags;          // StackStackElementFlags
 
         // 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,
+            INT flags       // StackStackElementFlags
+        );
 
         // Initialization done outside the TSL.
         // This will init the dwILOffset field (and potentially anything else
@@ -131,7 +129,7 @@ class DebugStackTrace
         DebugStackTraceElement* pElements;
         THREADBASEREF   TargetThread;
         AppDomain *pDomain;
-        BOOL	fDoWeHaveAnyFramesFromForeignStackTrace;
+        BOOL fDoWeHaveAnyFramesFromForeignStackTrace;
 
 
         GetStackFramesData() :  skip(0),

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,14 +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;
+        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 (!(pCf->HasFaulted() || pCf->IsIPadjusted()) && pStackTraceElem->ip != 0)
         {
             pStackTraceElem->ip -= 1;
+            pStackTraceElem->flags |= STEF_IP_ADJUSTED;
         }
 
         ++m_dFrameCount;

src/coreclr/src/vm/exceptionhandling.cpp

@@ -1333,7 +1333,12 @@ void ExceptionTracker::InitializeCrawlFrameForExplicitFrame(CrawlFrame* pcfThisF
 
     INDEBUG(memset(pcfThisFrame, 0xCC, sizeof(*pcfThisFrame)));
 
+    // Clear various flags
     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
+    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.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);