Code review feedback

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/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:

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

src/coreclr/src/vm/debugdebugger.h

@@ -96,25 +96,19 @@ 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.
         void InitPass1(
             DWORD dwNativeOffset,
             MethodDesc *pFunc,
             PCODE ip,
-            BOOL fIsLastFrameFromForeignStackTrace = FALSE,
-            BOOL fAdjustOffset = FALSE
+            INT flags       // StackStackElementFlags
         );
 
         // Initialization done outside the TSL.

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;

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