-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Debugger fixes for the Windows X86 EH Funclets model #115630
Conversation
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.
This reverts commit 73c21b5.
Co-authored-by: Filip Navara <filip.navara@gmail.com>
There was a problem hiding this 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; |
There was a problem hiding this comment.
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.
src/coreclr/jit/gcencode.cpp
Outdated
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
info->genericsContext + // For CORINFO_GENERICS_CTXT_FROM_PARAMTYPEARG | ||
((info->syncStartOffset != INVALID_SYNC_OFFSET) ? 1 : 0) // Is this method synchronized | ||
+ 1; // for ebpFrame |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/coreclr/jit/gcencode.cpp
Outdated
// 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
/ba-g the android build issue is unrelated to this failure and already known. |
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
Notable areas improved
Contributes to #113985