Debugger fixes for the Windows X86 EH Funclets model by davidwrighton · Pull Request #115630 · dotnet/runtime

davidwrighton · 2025-05-15T22:07:30Z

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

Copilot

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)

Copilot · 2025-05-15T22:07:57Z

+ // 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;


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.

filipnavara · 2025-05-16T10:13:06Z

 assert(header->epilogCount <= 1);
 }
 #endif
+ if (compiler->UsesFunclets() && compiler->info.compFlags & CORINFO_FLG_SYNCH && compiler->opts.compDbgEnC)


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.

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

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

filipnavara · 2025-05-16T10:17:12Z

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


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:
inlinesize_tGetParamTypeArgOffset(hdrInfo*info) { LIMITED_METHOD_DAC_CONTRACT; _ASSERTE((info->genericsContext||info->handlers) &&info->ebpFrame); #ifdefFEATURE_EH_FUNCLETSunsignedposition=info->savedRegsCountExclFP+info->localloc+ ((info->syncStartOffset!=INVALID_SYNC_OFFSET) ? 1 : 0) +// Is this method synchronized1; // For CORINFO_GENERICS_CTXT_FROM_PARAMTYPEARG#elseunsignedposition=info->savedRegsCountExclFP+info->localloc+1; // For CORINFO_GENERICS_CTXT_FROM_PARAMTYPEARG#endifreturnposition*sizeof(TADDR); }

hoyosjs · 2025-05-16T19:01:54Z

 )
 {
-PUNWIND_INFO pUnwindInfo = (PUNWIND_INFO)(ImageBase + FunctionEntry->UnwindData);
+DPTR(UNWIND_INFO) pUnwindInfo = dac_cast<DPTR(UNWIND_INFO)>(ImageBase + FunctionEntry->UnwindData);


hoyosjs · 2025-05-16T20:03:43Z


-#else // !FEATURE_EH_FUNCLETS
+#else // !TARGET_X86
 if ((pCF == NULL || !pCF->IsFrameless()) && pData->info.frame != NULL)


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

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.

filipnavara · 2025-05-18T08:19:40Z

+ // 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;


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.

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.

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

Co-authored-by: Juan Hoyos <19413848+hoyosjs@users.noreply.github.com>

This reverts commit 3708d1a.

…H_FUNCLETS"" This reverts commit b860621.

davidwrighton · 2025-05-22T22:03:32Z

/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

filipnavaraand others added 5 commits May 8, 2025 16:09

Change win-x86 configuration to build with FEATURE_EH_FUNCLETS
73c21b5

Revert "Change win-x86 configuration to build with FEATURE_EH_FUNCLETS"
3708d1a
This reverts commit 73c21b5.

Update src/coreclr/jit/codegenxarch.cpp
1e4ecc8
Co-authored-by: Filip Navara <filip.navara@gmail.com>

JIT formatting
c4710f0

CopilotAI review requested due to automatic review settings May 15, 2025 22:07

davidwrighton requested a review from MichalStrehovsky as a code owner May 15, 2025 22:07

github-actionsBot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 15, 2025

dotnet-policy-serviceBot assigned davidwrighton May 15, 2025

CopilotAI reviewed May 15, 2025
View reviewed changes

jkotas added area-ExceptionHandling-coreclr only use for closed issues and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 16, 2025

filipnavara reviewed May 16, 2025
View reviewed changes

filipnavara mentioned this pull request May 16, 2025
New exception handling on win-x86 #113985
Closed
6 tasks

hoyosjs reviewed May 16, 2025
View reviewed changes

hoyosjs approved these changes May 16, 2025
View reviewed changes

filipnavara reviewed May 18, 2025
View reviewed changes

Update src/coreclr/jit/gcencode.cpp
7c14b4e
Co-authored-by: Juan Hoyos <19413848+hoyosjs@users.noreply.github.com>

build-analysisBot mentioned this pull request May 21, 2025
The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008
Open
3 tasks

davidwrighton added 3 commits May 21, 2025 11:07

Reapply "Change win-x86 configuration to build with FEATURE_EH_FUNCLETS"
b860621
This reverts commit 3708d1a.

Fix issue around synchronized methods
d5aa015

Revert "Reapply "Change win-x86 configuration to build with FEATURE_E…
d32e3d2
…H_FUNCLETS"" This reverts commit b860621.

filipnavara approved these changes May 22, 2025
View reviewed changes

davidwrighton merged commit 1a5af14 into dotnet:mainMay 22, 2025

filipnavara mentioned this pull request May 24, 2025
Enable new exception handling on win-x86 #115957
Merged

github-actionsBot locked and limited conversation to collaborators Jun 22, 2025

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

- 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
+; // for ebpFrame

Uh oh!

Conversation

davidwrighton commented May 15, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

CopilotAIMay 15, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filipnavaraMay 16, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwrighton commented May 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

davidwrighton commented May 15, 2025•
edited
Loading

filipnavaraMay 16, 2025•
edited
Loading