Skip to content

Debugger fixes for the Windows X86 EH Funclets model #115630

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
May 22, 2025

Conversation

davidwrighton
Copy link
Member

@davidwrighton davidwrighton commented May 15, 2025

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

filipnavara and others added 5 commits May 8, 2025 16:09
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. Also NOTE: Before enabling funclets by default, manually test a set of scenarios which do EnC with various frame layouts. Notably, frame layouts with localloc, frame layouts with locals that trigger GSCookies to be used, frame layouts of static generic methods.
Co-authored-by: Filip Navara <filip.navara@gmail.com>
@Copilot Copilot AI review requested due to automatic review settings May 15, 2025 22:07
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 15, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR improves the debugger support for Windows X86 EH funclets by updating exception handling and stack frame management logic. Key changes include:

  • Adjustments to stack frame header and frame pointer calculations to support EH funclets.
  • Enhanced exception handling logic on X86 with funclets and updates for DAC pointer usage.
  • Minor logging improvements and documentation update for EnC and funclet support.

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/coreclr/vm/gc_unwind_x86.inl Added funclet-specific frame header calculation logic
src/coreclr/vm/exceptionhandling.cpp Adjusted SP computation for X86 with funclets
src/coreclr/vm/excep.cpp Refined exception handling conditionals and DAC handling
src/coreclr/vm/eetwain.cpp Prevented EnC remapping inside funclets
src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/StackFrameIterator.cs Inserted a stack trace hiding attribute
src/coreclr/jit/lclvars.cpp Updated comments for funclet support conditions
src/coreclr/jit/gcencode.cpp Encoded synchronization markers for EnC when using funclets
src/coreclr/jit/codegenxarch.cpp Added a NOP in funclet prologs to ensure correct IL mapping
src/coreclr/inc/regdisp.h Modified frame pointer retrieval to account for funclets
src/coreclr/inc/eetwain.h Wrapped synchronized region functions with preprocessor guards
src/coreclr/inc/clrnt.h Updated unwind info pointer to a DAC pointer
src/coreclr/debug/ee/frameinfo.cpp Adjusted frame pointer logic conditionals for X86 funclets
src/coreclr/debug/ee/debugger.cpp Enhanced logging details in exception handling
Comments suppressed due to low confidence (2)

src/coreclr/vm/excep.cpp:2959

  • [nitpick] The conditional now excludes FEATURE_EH_FUNCLETS when adjusting the SP for X86. Please confirm that this change correctly handles breakpoint and single-step exceptions in funclet mode.
TADDR sp = CallerStackFrame::FromRegDisplay(pRD).SP;

src/coreclr/debug/ee/frameinfo.cpp:1268

  • [nitpick] Please verify that switching the conditional from FEATURE_EH_FUNCLETS to !TARGET_X86 is intentional and correctly supports the intended behavior for X86, ensuring consistent frame pointer retrieval.
#if !defined(TARGET_X86)

// in funclets mode, we do need to know if the code is synchronized if we are generating
// and edit and continue method, so that we can properly manage the stack during a Remap
// operation. Instead of inventing a new encoding, just encode some non-0 offsets into these fields.
header->syncStartOffset = 1;
Copy link
Preview

Copilot AI May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a comment to explain the rationale behind using the literal offsets (1 and 2) for syncStartOffset and syncEndOffset to improve clarity and future maintainability.

Copilot uses AI. Check for mistakes.

@jkotas jkotas added area-ExceptionHandling-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 16, 2025
@@ -1578,6 +1578,15 @@ size_t GCInfo::gcInfoBlockHdrSave(
assert(header->epilogCount <= 1);
}
#endif
if (compiler->UsesFunclets() && compiler->info.compFlags & CORINFO_FLG_SYNCH && compiler->opts.compDbgEnC)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (compiler->UsesFunclets() && compiler->info.compFlags & CORINFO_FLG_SYNCH && compiler->opts.compDbgEnC)
if (compiler->UsesFunclets() && compiler->info.compFlags & CORINFO_FLG_SYNCH)

We need this information even for non-EnC to get position of generic parameters.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, this is exposed with the Pri1 tests with DOTNET_GCStress=0x3 in JIT\Generics\Fields\getclassfrommethodparam\getclassfrommethodparam.dll.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've addressed this issue as well as an issue where the method is both synchronized and has a stack allocation in it.

Comment on lines +660 to +662
info->genericsContext + // For CORINFO_GENERICS_CTXT_FROM_PARAMTYPEARG
((info->syncStartOffset != INVALID_SYNC_OFFSET) ? 1 : 0) // Is this method synchronized
+ 1; // for ebpFrame
Copy link
Member

@filipnavara filipnavara May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
info->genericsContext + // For CORINFO_GENERICS_CTXT_FROM_PARAMTYPEARG
((info->syncStartOffset != INVALID_SYNC_OFFSET) ? 1 : 0) // Is this method synchronized
+ 1; // for ebpFrame
((info->syncStartOffset != INVALID_SYNC_OFFSET) ? 1 : 0) + // Is this method synchronized
info->genericsContext + // For CORINFO_GENERICS_CTXT_FROM_PARAMTYPEARG
1; // for ebpFrame

Let's use an order that's closer to the reality.

We also need to apply the same fix to GetParamTypeArgOffset in gc_unwind_x86.inl:

inline
size_t GetParamTypeArgOffset(hdrInfo * info)
{
    LIMITED_METHOD_DAC_CONTRACT;

    _ASSERTE((info->genericsContext || info->handlers) && info->ebpFrame);

#ifdef FEATURE_EH_FUNCLETS
    unsigned position = info->savedRegsCountExclFP +
                        info->localloc +
                        ((info->syncStartOffset != INVALID_SYNC_OFFSET) ? 1 : 0) + // Is this method synchronized
                        1;  // For CORINFO_GENERICS_CTXT_FROM_PARAMTYPEARG
#else
    unsigned position = info->savedRegsCountExclFP +
                        info->localloc +
                        1;  // For CORINFO_GENERICS_CTXT_FROM_PARAMTYPEARG
#endif

    return position * sizeof(TADDR);
}

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ouch :)

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of curiosity for me - why is this branch only taken on non-x86? I'd expect this to be a funclet path.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The x86 unwinder continues to have a subtly different set of interactions with the rest of the codebase compared to the other architectures. Its... much closer than it was before, but the frame pointer concepts are somewhat different, and when I wrote this it appeared that it is related to the nature of X86 codegen where the sp moves during code execution in a frame, and how the unwinder updates somewhat different structures as it runs.

// and edit and continue method, so that we can properly manage the stack during a Remap
// operation. Instead of inventing a new encoding, just encode some non-0 offsets into these fields.
header->syncStartOffset = 1;
header->syncEndOffset = 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to set syncEndOffset to 1 as well? That way the region becomes effectively empty and invalid. It would make it easier to detect this abuse of the encoding in diagnostic tools for both FEATURE_EH_FUNCLETS and legacy builds.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a decent point. I don't think we have any diagnostic tools that care, but data that is patently invalid would be easier to check for. As I recall, I'd need to relax an assert or two, but that's no big deal.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Looks like it was just the one assert which was a problem.

Co-authored-by: Juan Hoyos <19413848+hoyosjs@users.noreply.github.com>
@davidwrighton
Copy link
Member Author

/ba-g the android build issue is unrelated to this failure and already known.

@davidwrighton davidwrighton merged commit 1a5af14 into dotnet:main May 22, 2025
107 of 109 checks passed
SimaTian pushed a commit that referenced this pull request May 27, 2025
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants