Debugger fixes for the Windows X86 EH Funclets model (#115630)

Notable areas improved
- The funclets model uses the vectored exception handler to funnel debug events to the debugger
- FramePointer details for the debugger are inscrutable and weird. I've made it work, but enough of this is driven by subtleties in the stackwalker that I would not be surprised if there are additional issues here
- RtlpGetFunctionEndAddress needs to use a DAC pointer to read the unwind info.
- Funclet prologs for CoreCLR X86 Funclets need to have at least 1 instruction in them so that the Native->IL mapping is correct. Otherwise it gets shadowed by a mapping which says it is a PROLOG instruction. This is probably fixable by allowing an Native -> IL mapping for both the PROLOG as well as the IL offset in the funclet, but a nop here is both easy to generate an unlikely to be a major cost.
- Likewise, EnC frame generation is no longer able to rely on the shadow stack pointer logic injected by EH handling, and needed 1 bit of info not in the current emitted stack information. Notably, that there is synchronized codegen in the method. Instead of modifying the gcinfo to include that data, we just put in fake offsets for where the synchronized region is, and use the existence of any data as a flag in the EnC layout. NOTE: I also removed the general purpose logic to read the synchronized range from the data. The fix for this also needs to fix the runtime behavior around stackwalking, and in another case in the presence of a locallloc instruction in the method.

Contributes to #113985

Author: davidwrighton

src/coreclr/debug/ee/debugger.cpp

@@ -7569,7 +7569,8 @@ HRESULT Debugger::SendException(Thread *pThread,
     }
     CONTRACTL_END;
 
-    LOG((LF_CORDB, LL_INFO10000, "D::SendException\n"));
+    LOG((LF_CORDB, LL_INFO10000, "D::SendException pThread=0x%p fFirstChance=%s currentIP=0x%p currentSP= 0x%p fContinuable=%s fAttaching=%s fForceNonInterceptable=%s\n",
+        pThread, fFirstChance ? "true" : "false", (void*)currentIP, (void*)currentSP, fContinuable ? "true" : "false", fAttaching ? "true" : "false", fForceNonInterceptable ? "true" : "false"));
 
     if (CORDBUnrecoverableError(this))
     {

src/coreclr/debug/ee/frameinfo.cpp

@@ -1265,7 +1265,7 @@ FramePointer GetFramePointerForDebugger(DebuggerFrameData* pData, CrawlFrame* pC
 
     FramePointer fpResult;
 
-#if defined(FEATURE_EH_FUNCLETS)
+#if !defined(TARGET_X86)
     if (pData->info.frame == NULL)
     {
         // This is a managed method frame.
@@ -1277,7 +1277,7 @@ FramePointer GetFramePointerForDebugger(DebuggerFrameData* pData, CrawlFrame* pC
         fpResult = FramePointer::MakeFramePointer((LPVOID)(pData->info.frame));
     }
 
-#else  // !FEATURE_EH_FUNCLETS
+#else  // !TARGET_X86
     if ((pCF == NULL || !pCF->IsFrameless()) && pData->info.frame != NULL)
     {
         //
@@ -1299,7 +1299,7 @@ FramePointer GetFramePointerForDebugger(DebuggerFrameData* pData, CrawlFrame* pC
         fpResult = FramePointer::MakeFramePointer((LPVOID)GetRegdisplayStackMark(&(pData->regDisplay)));
     }
 
-#endif // !FEATURE_EH_FUNCLETS
+#endif // !TARGET_X86
 
     LOG((LF_CORDB, LL_INFO100000, "GFPFD: Frame pointer is 0x%p\n", fpResult.GetSPValue()));
 

src/coreclr/inc/clrnt.h

@@ -211,7 +211,7 @@ RtlpGetFunctionEndAddress (
     _In_ TADDR ImageBase
     )
 {
-    PUNWIND_INFO pUnwindInfo = (PUNWIND_INFO)(ImageBase + FunctionEntry->UnwindData);
+    DPTR(UNWIND_INFO) pUnwindInfo = dac_cast<DPTR(UNWIND_INFO)>(ImageBase + FunctionEntry->UnwindData);
 
     return FunctionEntry->BeginAddress + pUnwindInfo->FunctionLength;
 }

src/coreclr/inc/eetwain.h

@@ -259,14 +259,15 @@ virtual void * GetGSCookieAddr(PREGDISPLAY     pContext,
 virtual bool IsInPrologOrEpilog(DWORD  relPCOffset,
                                 GCInfoToken gcInfoToken,
                                 size_t* prologSize) = 0;
-
+#ifndef FEATURE_EH_FUNCLETS
 /*
   Returns true if the given IP is in the synchronized region of the method (valid for synchronized methods only)
 */
 virtual bool IsInSynchronizedRegion(
                 DWORD       relOffset,
                 GCInfoToken gcInfoToken,
                 unsigned    flags) = 0;
+#endif // FEATURE_EH_FUNCLETS
 #endif // !USE_GC_INFO_DECODER
 
 /*
@@ -513,6 +514,7 @@ bool IsInPrologOrEpilog(
                 GCInfoToken gcInfoToken,
                 size_t*     prologSize);
 
+#ifndef FEATURE_EH_FUNCLETS
 /*
   Returns true if the given IP is in the synchronized region of the method (valid for synchronized functions only)
 */
@@ -521,6 +523,7 @@ bool IsInSynchronizedRegion(
                 DWORD       relOffset,
                 GCInfoToken gcInfoToken,
                 unsigned    flags);
+#endif // FEATURE_EH_FUNCLETS
 #endif // !USE_GC_INFO_DECODER
 
 /*
@@ -710,6 +713,7 @@ bool IsInPrologOrEpilog(
     return false;
 }
 
+#ifndef FEATURE_EH_FUNCLETS
 virtual
 bool IsInSynchronizedRegion(
                 DWORD       relOffset,
@@ -720,6 +724,7 @@ bool IsInSynchronizedRegion(
     _ASSERTE(FALSE);
     return false;
 }
+#endif // FEATURE_EH_FUNCLETS
 #endif // !USE_GC_INFO_DECODER
 
 virtual

src/coreclr/inc/regdisp.h

@@ -132,7 +132,11 @@ inline TADDR GetRegdisplayFP(REGDISPLAY *display) {
 inline LPVOID GetRegdisplayFPAddress(REGDISPLAY *display) {
     LIMITED_METHOD_CONTRACT;
 
+#ifdef FEATURE_EH_FUNCLETS
+    return &display->pCurrentContext->Ebp;
+#else
     return (LPVOID)display->GetEbpLocation();
+#endif
 }
 
 inline TADDR GetRegdisplayPCTAddr(REGDISPLAY *display)

src/coreclr/jit/codegenxarch.cpp

@@ -11049,6 +11049,13 @@ void CodeGen::genFuncletProlog(BasicBlock* block)
 #ifdef UNIX_X86_ABI
     // Add a padding for 16-byte alignment
     inst_RV_IV(INS_sub, REG_SPBASE, 12, EA_PTRSIZE);
+#else
+    if (!compiler->IsTargetAbi(CORINFO_NATIVEAOT_ABI))
+    {
+        // Funclet prologs need to have at least 1 byte or the IL->Native mapping data will not
+        // include the first IL instruction in the funclet.
+        instGen(INS_nop);
+    }
 #endif
 
     genClearAvxStateInProlog();

src/coreclr/jit/gcencode.cpp

@@ -1606,6 +1606,22 @@ size_t GCInfo::gcInfoBlockHdrSave(
         assert(header->epilogCount <= 1);
     }
 #endif
+    if (compiler->UsesFunclets() && compiler->info.compFlags & CORINFO_FLG_SYNCH)
+    {
+        // While the sync start offset and end offset are not used by the stackwalker/EH system
+        // in funclets mode, we do need to know if the code is synchronized if we are generating
+        // an edit and continue method, so that we can properly manage the stack during a Remap
+        // operation, for determining the ParamTypeArg for collectible generics purposes, and
+        // for determining the offset of the localloc variable in the stack frame.
+        // Instead of inventing a new encoding, just encode some non-0 offsets into these fields.
+        // to indicate that the method is synchronized.
+        //
+        // Use 1 for both offsets, since that doesn't actually make sense and implies that the
+        // sync region is 0 bytes long. The JIT will never emit a sync region of 0 bytes in non-
+        // funclet mode.
+        header->syncStartOffset = 1;
+        header->syncEndOffset   = 1;
+    }
 
     header->revPInvokeOffset = INVALID_REV_PINVOKE_OFFSET;
     if (compiler->opts.IsReversePInvoke())

src/coreclr/jit/lclvars.cpp

@@ -3673,6 +3673,9 @@ PhaseStatus Compiler::lvaMarkLocalVars()
     //      saved EBP                       <-- EBP points here
     //      other callee-saved registers    // InfoHdrSmall.savedRegsCountExclFP specifies this size
     //      optional GS cookie              // InfoHdrSmall.security is 1 if this exists
+    // if FEATURE_EH_FUNCLETS
+    //      issynchronized bool if it is a synchronized method
+    // endif // FEATURE_EH_FUNCLETS
     //      LocAllocSP slot
     //      -- lower addresses --
     //
@@ -4085,13 +4088,15 @@ unsigned Compiler::lvaGetMaxSpillTempSize()
  *      |   security object     |
  *      |-----------------------|
  *      |     ParamTypeArg      |
+// If funclet support is disabled
  *      |-----------------------|
  *      |  Last-executed-filter |
  *      |-----------------------|
  *      |                       |
  *      ~      Shadow SPs       ~
  *      |                       |
  *      |-----------------------|
+// Endif funclet support is disabled
  *      |                       |
  *      ~      Variables        ~
  *      |                       |

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/StackFrameIterator.cs

@@ -1,6 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Diagnostics;
 using System.Runtime.InteropServices;
 #if !NATIVEAOT
 using System.Runtime.ExceptionServices;
@@ -60,6 +61,7 @@ internal unsafe struct StackFrameIterator
 #pragma warning restore CA1822
 #endif // NATIVEAOT
 
+        [StackTraceHidden]
         internal bool Init(EH.PAL_LIMITED_CONTEXT* pStackwalkCtx, bool instructionFault = false, bool* fIsExceptionIntercepted = null)
         {
             return InternalCalls.RhpSfiInit(ref this, pStackwalkCtx, instructionFault, fIsExceptionIntercepted);

src/coreclr/vm/eetwain.cpp

@@ -161,7 +161,7 @@ bool        VarIsInReg(ICorDebugInfo::VarLoc varLoc)
  *  could easily duplicate what they do, which is why we're calling into them.
  */
 
-HRESULT EECodeManager::FixContextForEnC(PCONTEXT         pCtx,
+ HRESULT EECodeManager::FixContextForEnC(PCONTEXT         pCtx,
                                         EECodeInfo *     pOldCodeInfo,
                    const ICorDebugInfo::NativeVarInfo *  oldMethodVars,
                                         SIZE_T           oldMethodVarsCount,
@@ -206,6 +206,11 @@ HRESULT EECodeManager::FixContextForEnC(PCONTEXT         pCtx,
         return E_FAIL; // stack should be empty - <TODO> @TODO : Barring localloc</TODO>
     }
 
+#ifdef FEATURE_EH_FUNCLETS
+    // EnC remap inside handlers is not supported
+    if (pOldCodeInfo->IsFunclet() || pNewCodeInfo->IsFunclet())
+        return CORDBG_E_ENC_IN_FUNCLET;
+#else
     if (oldInfo->handlers)
     {
         bool      hasInnerFilter;
@@ -229,13 +234,16 @@ HRESULT EECodeManager::FixContextForEnC(PCONTEXT         pCtx,
             }
         }
     }
+#endif // FEATURE_EH_FUNCLETS
 
     /* @TODO: Check if we have grown out of space for locals, in the face of localloc */
     _ASSERTE(!oldInfo->localloc && !newInfo->localloc);
 
+#ifndef FEATURE_EH_FUNCLETS
     // @TODO: If nesting level grows above the MAX_EnC_HANDLER_NESTING_LEVEL,
     // we should return EnC_NESTED_HANLDERS
     _ASSERTE(oldInfo->handlers && newInfo->handlers);
+#endif
 
     LOG((LF_ENC, LL_INFO100, "EECM::FixContextForEnC: Checks out\n"));
 
@@ -1552,7 +1560,7 @@ OBJECTREF EECodeManager::GetInstance( PREGDISPLAY    pContext,
 #endif
 
 #else // FEATURE_EH_FUNCLETS
-    if (pCodeInfo->GetMethodDesc()->AcquiresInstMethodTableFromThis()) // Generic Context is "this"
+    if (pCodeInfo->GetMethodDesc()->AcquiresInstMethodTableFromThis() && (hdrInfoBody->genericsContext)) // Generic Context is "this"
     {
         // Untracked table must have at least one entry - this pointer
         _ASSERTE(hdrInfoBody->untrackedCnt > 0);
@@ -1807,6 +1815,7 @@ bool EECodeManager::IsInPrologOrEpilog(DWORD       relPCoffset,
             (info.epilogOffs != hdrInfo::NOT_IN_EPILOG));
 }
 
+#ifndef FEATURE_EH_FUNCLETS
 /*****************************************************************************
  *
  *  Returns true if the given IP is in the synchronized region of the method (valid for synchronized functions only)
@@ -1837,6 +1846,7 @@ bool  EECodeManager::IsInSynchronizedRegion(DWORD       relOffset,
         // Everything after the epilog is also in synchronized region.
         (info.epilogCnt != 0 && info.syncEpilogStart + info.epilogSize <= relOffset);
 }
+#endif // FEATURE_EH_FUNCLETS
 #endif // !USE_GC_INFO_DECODER
 
 /*****************************************************************************