Fix for stress failure when adjusting effective IP while stackwalking…

… may put it on a wrong instruction. (#100376)

* fix

* same change for x86 and NativeAOT, get rid of ICodeManagerFlags::AbortingCall

* Apply suggestions from code review

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

---------

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

src/coreclr/inc/eetwain.h

@@ -89,7 +89,6 @@ enum ICodeManagerFlags
     ExecutionAborted =  0x0002, // execution of this function has been aborted
                                     // (i.e. it will not continue execution at the
                                     // current location)
-    AbortingCall    =   0x0004, // The current call will never return
     UpdateAllRegs   =   0x0008, // update full register set
     CodeAltered     =   0x0010, // code of that function might be altered
                                     // (e.g. by debugger), need to call EE

src/coreclr/inc/gcinfodecoder.h

@@ -179,7 +179,6 @@ enum ICodeManagerFlags
     ExecutionAborted  =  0x0002, // execution of this function has been aborted
                                  // (i.e. it will not continue execution at the
                                  // current location)
-    AbortingCall      =  0x0004, // The current call will never return
     ParentOfFuncletStackFrame
                       =  0x0040, // A funclet for this frame was previously reported
 

src/coreclr/jit/codegenlinear.cpp

@@ -712,7 +712,9 @@ void CodeGen::genCodeForBBlist()
 
                     if ((call != nullptr) && (call->gtOper == GT_CALL))
                     {
-                        if ((call->AsCall()->gtCallMoreFlags & GTF_CALL_M_DOES_NOT_RETURN) != 0)
+                        if ((call->AsCall()->gtCallMoreFlags & GTF_CALL_M_DOES_NOT_RETURN) != 0 ||
+                            ((call->AsCall()->gtCallType == CT_HELPER) &&
+                             Compiler::s_helperCallProperties.AlwaysThrow(call->AsCall()->GetHelperNum())))
                         {
                             instGen(INS_BREAKPOINT); // This should never get executed
                         }
@@ -756,6 +758,20 @@ void CodeGen::genCodeForBBlist()
 
             case BBJ_ALWAYS:
             {
+                GenTree* call = block->lastNode();
+                if ((call != nullptr) && (call->gtOper == GT_CALL))
+                {
+                    if ((call->AsCall()->gtCallMoreFlags & GTF_CALL_M_DOES_NOT_RETURN) != 0 ||
+                        ((call->AsCall()->gtCallType == CT_HELPER) &&
+                         Compiler::s_helperCallProperties.AlwaysThrow(call->AsCall()->GetHelperNum())))
+                    {
+                        // NOTE: We should probably never see a BBJ_ALWAYS block ending with a throw in a first place.
+                        //       If that is fixed, this condition can be just an assert.
+                        //       For the reasons why we insert a BP, see the similar code in "case BBJ_THROW:" above.
+                        instGen(INS_BREAKPOINT); // This should never get executed
+                    }
+                }
+
                 // If this block jumps to the next one, we might be able to skip emitting the jump
                 if (block->CanRemoveJumpToNext(compiler))
                 {

src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp

@@ -212,14 +212,11 @@ void UnixNativeCodeManager::EnumGcRefs(MethodInfo *    pMethodInfo,
     ASSERT(((uintptr_t)codeOffset & 1) == 0);
 #endif
 
-    if (!isActiveStackFrame)
+    bool executionAborted = ((UnixNativeMethodInfo*)pMethodInfo)->executionAborted;
+
+    if (!isActiveStackFrame && !executionAborted)
     {
-        // If we are not in the active method, we are currently pointing
-        // to the return address. That may not be reachable after a call (if call does not return)
-        // or reachable via a jump and thus have a different live set.
-        // Therefore we simply adjust the offset to inside of call instruction.
-        // NOTE: The GcInfoDecoder depends on this; if you change it, you must
-        // revisit the GcInfoEncoder/Decoder
+        // the reasons for this adjustment are explained in EECodeManager::EnumGcRefs
         codeOffset--;
     }
 
@@ -230,7 +227,7 @@ void UnixNativeCodeManager::EnumGcRefs(MethodInfo *    pMethodInfo,
     );
 
     ICodeManagerFlags flags = (ICodeManagerFlags)0;
-    if (((UnixNativeMethodInfo*)pMethodInfo)->executionAborted)
+    if (executionAborted)
         flags = ICodeManagerFlags::ExecutionAborted;
 
     if (IsFilter(pMethodInfo))

src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp

@@ -435,8 +435,10 @@ void CoffNativeCodeManager::EnumGcRefs(MethodInfo *    pMethodInfo,
     PTR_uint8_t gcInfo;
     uint32_t codeOffset = GetCodeOffset(pMethodInfo, safePointAddress, &gcInfo);
 
+    bool executionAborted = ((CoffNativeMethodInfo *)pMethodInfo)->executionAborted;
+
     ICodeManagerFlags flags = (ICodeManagerFlags)0;
-    if (((CoffNativeMethodInfo *)pMethodInfo)->executionAborted)
+    if (executionAborted)
         flags = ICodeManagerFlags::ExecutionAborted;
 
     if (IsFilter(pMethodInfo))
@@ -446,14 +448,9 @@ void CoffNativeCodeManager::EnumGcRefs(MethodInfo *    pMethodInfo,
         flags = (ICodeManagerFlags)(flags | ICodeManagerFlags::ActiveStackFrame);
 
 #ifdef USE_GC_INFO_DECODER
-    if (!isActiveStackFrame)
+    if (!isActiveStackFrame && !executionAborted)
     {
-        // If we are not in the active method, we are currently pointing
-        // to the return address. That may not be reachable after a call (if call does not return)
-        // or reachable via a jump and thus have a different live set.
-        // Therefore we simply adjust the offset to inside of call instruction.
-        // NOTE: The GcInfoDecoder depends on this; if you change it, you must
-        // revisit the GcInfoEncoder/Decoder
+        // the reasons for this adjustment are explained in EECodeManager::EnumGcRefs
         codeOffset--;
     }
 

src/coreclr/vm/eetwain.cpp

@@ -1466,17 +1466,15 @@ bool EECodeManager::EnumGcRefs( PREGDISPLAY     pRD,
     }
     else
     {
-        /* However if ExecutionAborted, then this must be one of the
-         * ExceptionFrames. Handle accordingly
-         */
-        _ASSERTE(!(flags & AbortingCall) || !(flags & ActiveStackFrame));
-
-        if (flags & AbortingCall)
-        {
-            curOffs--;
-            LOG((LF_GCINFO, LL_INFO1000, "Adjusted GC reporting offset due to flags ExecutionAborted && AbortingCall. Now reporting GC refs for %s at offset %04x.\n",
-                methodName, curOffs));
-        }
+        // Since we are aborting execution, we are either in a frame that actually faulted or in a throwing call.
+        // * We do not need to adjust in a leaf
+        // * A throwing call will have unreachable <brk> after it, thus GC info is the same as before the call.
+        // 
+        // Either way we do not need to adjust.
+
+        // NOTE: only fully interruptible methods may need to report anything here as without
+        //       exception handling all current local variables are already unreachable.
+        //       EnumerateLiveSlots will shortcircuit the partially interruptible case just a bit later.
     }
 
     // Check if we have been given an override value for relOffset

src/coreclr/vm/gc_unwind_x86.inl

@@ -3686,13 +3686,7 @@ bool EnumGcRefsX86(PREGDISPLAY     pContext,
     }
     else
     {
-        /* However if ExecutionAborted, then this must be one of the
-         * ExceptionFrames. Handle accordingly
-         */
-        _ASSERTE(!(flags & AbortingCall) || !(flags & ActiveStackFrame));
-
-        newCurOffs = (flags & AbortingCall) ? curOffs-1 // inside "call"
-                                            : curOffs;  // at faulting instr, or start of "try"
+        newCurOffs = curOffs;
     }
 
     ptrOffs    = 0;

src/coreclr/vm/stackwalk.h

@@ -284,7 +284,6 @@ class CrawlFrame
             if (!HasFaulted() && !IsIPadjusted())
             {
                 _ASSERTE(!(flags & ActiveStackFrame));
-                flags |= AbortingCall;
             }
         }