') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); })(); Enable new exception handling on win-x86 by filipnavara · Pull Request #115957 · dotnet/runtime · GitHub
Skip to content

Enable new exception handling on win-x86 - #115957

Merged
jkotas merged 9 commits into
dotnet:mainfrom
filipnavara:x86funclets
May 26, 2025
Merged

Enable new exception handling on win-x86#115957
jkotas merged 9 commits into
dotnet:mainfrom
filipnavara:x86funclets

Conversation

@filipnavara

@filipnavarafilipnavara commented May 24, 2025

Copy link
Copy Markdown
Member

Switches JIT to the funclet model that is used by other platforms and VM to use the new exception handling introduced in .NET 9.

Fixes#113985

@filipnavara
filipnavara requested a review from jkotasMay 24, 2025 04:25
@github-actionsgithub-actionsBot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 24, 2025
@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label May 24, 2025
@filipnavarafilipnavara 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 24, 2025
@jkotas

Copy link
Copy Markdown
Member

/azp run runtime-coreclr outerloop

@jkotas

Copy link
Copy Markdown
Member

/azp run runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@am11

am11 commented May 24, 2025

Copy link
Copy Markdown
Member

!CREATE_CHECK_STRING(pMT && pMT->Validate()) is #110512.

@filipnavara

Copy link
Copy Markdown
MemberAuthor

!CREATE_CHECK_STRING(pMT && pMT->Validate()) is #110512.

I'm not so sure it's the same root cause. I'll look at the dumps later on.

@filipnavara

filipnavara commented May 24, 2025

Copy link
Copy Markdown
MemberAuthor

The JIT.opt stress test is failing when reporting GC references at the nop instruction in funclet prolog that was added in #115630.

More than one value in the dump seems off, but at least it explains why I didn't see this in earlier gcstress pass.

Disassembly:

0eedf182 59 pop ecx
0eedf183 5b pop ebx
0eedf184 5e pop esi
0eedf185 5f pop edi
0eedf186 5d pop ebp
0eedf187 c3 ret 0eedf188 90 nop <--- GC stress here
0eedf189 8945f0 mov dword ptr [ebp-10h], eax
0eedf18c 8d0517a2d10e lea eax, ds:[0ED1A217h]
0eedf192 c3 ret 0eedf193 0000 add byte ptr [eax], al
0eedf195 0000 add byte ptr [eax], al
0eedf197 00cf add bh, cl

Locals in EnumGCRefsX86:

methodStart = 0x000000000eedf0d8 <-- likely wrong
curOffs = 0xb0
funcletStart = 0x000000000ed1a220

GC info:

!ip2md 0x000000000ed1a220
MethodDesc: 097098f0
Method Name: Xunit.Assert.RecordException(System.Func`1<System.Object>, System.String)
Class: 0970afb8
MethodTable: 0970afb8
mdToken: 060000CE
Module: 09707674
IsJitted: yes
Current CodeAddr: 0ed1a170
Version History:
ILCodeVersion: 00000000
ReJIT ID: 0
IL Addr: 09735450
CodeAddr: 0ed1a170 (Optimized)
NativeCodeVersion: 00000000
Source file: /_/src/arcade/src/Microsoft.DotNet.XUnitAssert/src/Record.cs @ 72
0:000> !gcinfo 097098f0
entry point 0ED1A170
Normal JIT generated code
GC info 0EEAE34C
Method info block:
method size = 00BB
prolog size = 11 epilog size = 6 epilog count = 2 epilog end = no callee-saved regs = EDI ESI EBX EBP ebp frame = yes fully interruptible= yes double align = no arguments size = 0 DWORDs
stack frame size = 1 DWORDs
untracked count = 0 var ptr tab count = 2 exception handlers = yes
epilog # 0 at 0027
epilog # 1 at 00AA
argTabOffset = 7 81 3B AB B1 9C | BF C3 CF 01 02 | 27 81 03 07 | Pointer table:
10 81 27 0A | 00A7..00B1 [EBP-10H] a pointer
10 0D 07 ...| 00B4..00BB [EBP-10H] a pointer
F0 41 4F ...| 0009 reg EAX becoming live
72 0B 52 ...| 000B reg ESI becoming live
4F 52 00 ...| 0012 reg ECX becoming live
0B 00 F0 ...| 0015 reg ECX becoming dead
52 F0 42 ...| 0017 reg EDX becoming live
00 42 10 ...| 0017 reg EAX becoming dead
F0 42 F0 ...| 0021 reg EAX becoming live
10 04 30 ...| 0021 reg EDX becoming dead
F0 04 F0 ...| 002D reg EAX becoming dead
30 42 72 ...| 002D reg ESI becoming dead
F0 42 F1 ...| 0037 reg EAX becoming live
72 49 F0 ...| 0039 reg ESI becoming live
F1 49 0B ...| 004A reg ECX becoming live
F0 0B 4A ...| 0055 reg ECX becoming dead
52 06 08 ...| 0057 reg EDX becoming live
4A 08 10 ...| 0059 reg ECX becoming live
06 10 4A ...| 005F reg EAX becoming dead
08 4A 0D ...| 005F reg ECX becoming dead
10 0D 30 ...| 005F reg EDX becoming dead
4A 30 71 ...| 0061 reg ECX becoming live
0D 71 F0 ...| 0066 reg ECX becoming dead
30 F0 42 ...| 0066 reg ESI becoming dead
71 42 7A ...| 0067 reg ESI becoming live
F0 42 F0 ...| 0071 reg EAX becoming live
7A 58 F1 ...| 0073 reg EDI becoming live
F0 58 51 ...| 007B reg EBX becoming live
F1 51 4A ...| 008C reg EDX becoming live
81 0E 10 ...| 008D push ptr 0
4A 10 18 ...| 008F reg ECX becoming live
0E 18 30 ...| 0095 reg ECX becoming dead
10 30 C8 ...| 0095 reg EDX becoming dead
18 C8 52 ...| 0095 reg EBX becoming dead
30 52 4A ...| 0095 reg ESI becoming dead
C8 4A 06 ...| 0095 pop 1 ptrs
52 06 08 ...| 0097 reg EDX becoming live
4A 08 10 ...| 0099 reg ECX becoming live
06 10 4A ...| 009F reg EAX becoming dead
08 4A 0D ...| 009F reg ECX becoming dead
10 0D 38 ...| 009F reg EDX becoming dead
4A 38 44 ...| 00A1 reg ECX becoming live
0D 44 F1 ...| 00A6 reg ECX becoming dead
38 F1 00 ...| 00A6 reg EDI becoming dead
44 00 FF ...| 00AA reg EAX becoming live
F1 00 4C ...| 00BA reg EAX becoming dead
FF 00 00 ...| 

Presumably it's trying to report 10 81 27 0A | 00A7..00B1 [EBP-10H] a pointer at the time of the crash which is indeed not valid yet. It only becomes valid on the next instruction.

@filipnavara

Copy link
Copy Markdown
MemberAuthor

@filipnavara

Copy link
Copy Markdown
MemberAuthor

I'm starting to suspect that the liveness of the variable may be misreported on other platforms too. We just get away with it because the funclet prolog is marked as nogc which excludes it from GC stress. Until recently we didn't have the capability to report these no-GC regions on x86 and to keep the GC info small I opted to never emit it for any prolog/epilog, main function or funclet. We can change that to emit it for funclet prologs/epilogs but I would like someone to look at the liveness issue first before patching it up with a bandaid. /cc @dotnet/jit-contrib

x86 JIT dump: https://gist.githubusercontent.com/filipnavara/87489fc9b6224706111055ce1ee79583/raw/80122358359dbe95e53fd7708c59611f7024aa87/gistfile1.txt
x64 JIT dump: https://gist.githubusercontent.com/filipnavara/c76975c784519ed718e1701a24e6a458/raw/88a2fc3e1eddf932d62065cd63d578a38eca3a11/gistfile1.txt

@SingleAccretion

SingleAccretion commented May 24, 2025

Copy link
Copy Markdown
Contributor

We just get away with it because the funclet prolog is marked as nogc which excludes it from GC stress. Until recently we didn't have the capability to report these no-GC regions on x86 and to keep the GC info small I opted to never emit it for any prolog/epilog, main function or funclet. We can change that to emit it for funclet prologs/epilogs but I would like someone to look at the liveness issue first before patching it up with a bandaid.

It is an expected invariant that prolog/epilog are no-GC regions. Why did this issue not manifest for filter funclets in the old scheme (because filter-live variable are always pinned?)?

@filipnavara

filipnavara commented May 24, 2025

Copy link
Copy Markdown
MemberAuthor

It is an expected invariant that prolog/epilog are no-GC regions.

Yeah, and that's fine. x86 GC info always encoded the (non-funclet) prologs/epilogs and treated them as no-GC on the VM side.

Until #115630 the funclet prolog was empty on win-x86 so we didn't run into this problem because there was no code in the no-GC region. Likewise, the funclet epilog is single "ret" instruction so we don't really run into problem either. (These assumptions are not true for linux-x86 which has additional stack alignment instruction and which would have likely run into this problem if someone ever got far enough to run GC stress tests.)

I did, however, expect that calling gcInfo.gcResetForBB(); from genFuncletProlog would reset the liveness of the variables. I'm not sure that's actually happening (properly). Observably, the variable slot gets invalidated only before the IN002d: 0000B1 mov gword ptr [V06 ebp-0x10], eax instruction and then it becomes valid again. I would have expected the previous validity of the slot to be already reset at the beginning of genFuncletProlog and then the new one to start at 0xB4 as it does. (UPD: Or rather, I would have expected the genEpilog to end the liveness; whoever does it, it's unexpected that the variable is alive past the ret instruction and into the following block.)

Why did this issue not manifest for filter funclets in the old scheme (because filter-live variable are always pinned?)?

Not sure I know the precise answer. I remember stumbling upon some specific JIT32_GCENCODER blocks but cannot find it right now.

@SingleAccretion

SingleAccretion commented May 24, 2025

Copy link
Copy Markdown
Contributor

(UPD: Or rather, I would have expected the genEpilog to end the liveness; whoever does it, it's unexpected that the variable is alive past the ret instruction and into the following block.)

Looks like we have a comment that this is indeed intentional:

voidCodeGen::genReserveFuncletProlog(BasicBlock* block)
{
assert(compiler->UsesFunclets());
assert(block != nullptr);
/* Currently, no registers are live on entry to the prolog, except maybe the exception object. There might be some live stack vars, but they cannot be accessed until after the frame pointer is re-established. In order to potentially prevent emitting a death before the prolog and a birth right after it, we just report it as live during the prolog, and rely on the prolog being non-interruptible. Trust genCodeForBBlist to correctly initialize all the sets. We might need to relax these asserts if the VM ever starts restoring any registers, then we could have live-in reg vars...*/noway_assert((gcInfo.gcRegGCrefSetCur & RBM_EXCEPTION_OBJECT) == gcInfo.gcRegGCrefSetCur);
noway_assert(gcInfo.gcRegByrefSetCur == 0);
JITDUMP("Reserving funclet prolog IG for block "FMT_BB"\n", block->bbNum);
GetEmitter()->emitCreatePlaceholderIG(IGPT_FUNCLET_PROLOG, block, gcInfo.gcVarPtrSetCur, gcInfo.gcRegGCrefSetCur,
gcInfo.gcRegByrefSetCur, false);
}

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Looks like we have a comment that this is indeed intentional:

Thanks. I adjusted emitting the no-GC regions for funclet prologs/epilogs but I will look into this at some later point to see if we can improve the codegen. We no longer establish the frame pointer in funclet prologs on any platform so the reasoning makes little sense.

@jkotas

Copy link
Copy Markdown
Member

/azp run runtime-coreclr gcstress0x3-gcstress0xc

@jkotas

Copy link
Copy Markdown
Member

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

Comment threadsrc/coreclr/jit/emitinl.h
@jkotas

jkotas commented May 25, 2025

Copy link
Copy Markdown
Member

BestFitMapping

Log https://helixr1107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-115957-merge-05d454263dee421ea0/Interop.0.1/1/console.b2222a36.log

Failure:

Assert failure(PID 9828 [0x00002664], Thread: 7420 [0x1cfc]): SyncTableEntry::GetSyncTableEntry()[sbIndex].m_Object == obj
CORECLR! ObjHeader::Validate + 0x147 (0x73f2686f)
CORECLR! Object::ValidateInner + 0x20F (0x73ed9e1f)
CORECLR! Object::Validate + 0x98 (0x73ed9bd8)
CORECLR! WKS::GCHeap::Promote + 0x8F (0x741fedaf)
CORECLR! GcEnumObject + 0x6F (0x73ff522f)
CORECLR! EnumGcRefsX86 + 0x10C6 (0x73e00776)

This assert is one of the typical GC hole symptoms. It is likely to be hit anytime we miss reporting an object with a sync block. The offending object in this case is a marshalled delegate. Marshalled delegates have syncblocks.

The object is valid but it has a header pointing to sync block.

It likely means that the object was collected, but the space was not reused by the GC for a new object yet. We likely missed reporting the slot in some earlier GC but reporting it again.

The callstack looks like this: BestFitMapping!ILStubClass.IL_STUB_PInvoke -> native code -> BestFitMapping!ILStubClass.IL_STUB_ReversePInvoke

  • IL_STUB_ReversePInvoke has finally block. We just returned from calling the finally block (non-exceptionally).
  • IL_STUB_PInvoke has a local that stores the invalid delegate object.

Let's take a look what !DumpLog says about the previous GC:

1cfc 1.930619591 : `GC`GCROOTS` Ending scan of Thread 02FAD110 ID = 0x2 {
1cfc 1.930618407 : `GCROOTS` STACKWALK: SKIPPING_TO_FUNCLET_PARENT: not making callback for this frame, SPOfParent = 00B7E8D4, isILStub = 0, m_crawl.pFunc = 08AE0314 (__GeneratedMainWrapper.Main())
1cfc 1.930618280 : `EH`GCROOTS` CrawlFrame (00B7BE54): Frameless: Yes CallerSP: 00B7E980
1cfc 1.930615468 : `GCROOTS` STACKWALK: SKIPPING_TO_FUNCLET_PARENT: not making callback for this frame, SPOfParent = 00B7E8D4, isILStub = 0, m_crawl.pFunc = 08AE2960 (BestFitMapping.TestEntryPoint())
1cfc 1.930615330 : `EH`GCROOTS` CrawlFrame (00B7BE54): Frameless: Yes CallerSP: 00B7E968
1cfc 1.930612350 : `GCROOTS` STACKWALK: SKIPPING_TO_FUNCLET_PARENT: not making callback for this frame, SPOfParent = 00B7E8D4, isILStub = 1, m_crawl.pFunc = 08DA84E4 (ILStubClass.IL_STUB_PInvoke(SCallBackInOutByRef))
1cfc 1.930612200 : `EH`GCROOTS` CrawlFrame (00B7BE54): Frameless: Yes CallerSP: 00B7E938
1cfc 1.930608805 : `GC`GCROOTS` GC Root 00B7E8B8 RELOCATED 0553E480 -> 05538754 MT = 035B8FC4 (System.String)
1cfc 1.930606588 : `GCROOTS` Scanning Frameless method 08DA89DC (ILStubClass.IL_STUB_ReversePInvoke(IntPtr*)) EIP = 04D973C7 &EIP = 00B7DE90
1cfc 1.930606158 : `EH`GCROOTS` CrawlFrame (00B7BE54): Frameless: Yes CallerSP: 00B7E87C
1cfc 1.930605891 : `GCROOTS` STACKWALK: Found Non-Filter funclet @ SP: 00B7E878, m_crawl.pFunc = 08DA89DC; FuncletParentCallerSP: 00B7E8D4
1cfc 1.930605836 : `EH`GCROOTS` CrawlFrame (00B7BE54): Frameless: Yes CallerSP: 00B7E87C
1cfc 1.930599724 : `GCROOTS` Scanning ExplicitFrame 00B7D0E8 AssocMethod = 00000000 FrameIdentifier = ResumableFrame

This GC was triggered in the finally funclet inside IL_STUB_ReversePInvoke. But then the stackwalk got confused and skipped reporting for ILStubClass.IL_STUB_PInvoke. That's the GC hole.

@am11

am11 commented May 25, 2025

Copy link
Copy Markdown
Member

BestFitMapping was fixed by b9cd5ac. Could you rerun gcstress?

@jkotas

Copy link
Copy Markdown
Member

BestFitMapping was fixed by b9cd5ac.

This was a fix for Interop/COM/NETClients/IDispatch/NETClientIDispatch/NETClientIDispatch. I do not think BestFitMapping is fixed.

@jkotas

Copy link
Copy Markdown
Member

/azp run runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@filipnavara

filipnavara commented May 25, 2025

Copy link
Copy Markdown
MemberAuthor

I have a fix locally for Methodical_r1.0.1. I'll push it after going through more tests and once the current GCStress run finishes.

UPD: Hmm, still needs some refining.

UPD2: As usual, the refining consisted of actually saving a file before starting the compilation... (ac4373f)

@jkotas

Copy link
Copy Markdown
Member

/azp run runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

Comment threadsrc/coreclr/vm/gcenv.ee.common.cpp Outdated
#if defined(FEATURE_EH_FUNCLETS)
if (pCF->ShouldParentToFuncletUseUnwindTargetLocationForGCReporting())
{
bool wantsReportOnlyLeaf = true;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: This is always set by RyuJIT. We can delete all places that check it in the runtime and change this

#ifdef TARGET_AMD64

to #if defined(TARGET_AMD64) && defined(DECODE_OLD_FORMATS).

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Sounds reasonable. It will likely need some documentation/comments update too.

@filipnavara

filipnavara commented May 25, 2025

Copy link
Copy Markdown
MemberAuthor

The BestFitMapping fails in like every 6th try on my machine when running in isolation (w/ GCStress=0xC and TieredCompilation=0). I am trying to capture a time travel trace to investigate it but didn't have luck so far.

UPD: Got lucky.

UPD2: When the ILStubClass.IL_STUB_PInvoke frame is skipped the value of m_sfParent.SP == 0xbadddc (the value is actually a valid one)

UPD3: I found what is the difference in behavior between x86 and x64. Need to run full battery of tests before I change it.

@filipnavara

filipnavara commented May 25, 2025

Copy link
Copy Markdown
MemberAuthor

The issue with BestFitMapping is the following:

  • Let's start with the analysis in Enable new exception handling on win-x86 #115957 (comment)
  • There are two frames of the ILStubClass.IL_STUB_ReversePInvoke on the stack. The bottom one is an out-of-line finally, the one above is the trampoline that jumps to it. Next frame is the unmanaged code.
  • When we start unwinding, we end up in the StackFrameIterator::Filter method in the branch that sets m_sfParent = m_sfFuncletParent;. The expectation is that this will turn a state where some frames are skipped for reporting until m_sfParent is reached. It will then resume the normal reporting. Unfortunately, the parent frame never gets iterated so this mechanism fails.
  • The reason why the frame doesn't get iterated is the code introduce in Fix x86 EH for UnmanagedCallersOnly methods #58796:
    #ifdef TARGET_X86
    hdrInfo *gcHdrInfo;
    m_crawl.codeInfo.DecodeGCHdrInfo(&gcHdrInfo);
    bool hasReversePInvoke = gcHdrInfo->revPInvokeOffset != INVALID_REV_PINVOKE_OFFSET;
    #endif// TARGET_X86
    ProcessIp(GetControlPC(m_crawl.pRD));
    // if we have unwound to a native stack frame, stop and set the frame state accordingly
    if (!m_crawl.isFrameless)
    {
    m_frameState = SFITER_NATIVE_MARKER_FRAME;
    m_crawl.isNativeMarker = true;
    }
    #ifdef TARGET_X86
    elseif (hasReversePInvoke)
    {
    // The managed frame we've unwound from had reverse PInvoke frame. Since we are on a frameless
    // frame, that means that the method was called from managed code without any native frames in between.
    // On x86, the InlinedCallFrame of the pinvoke would get skipped as we've just unwound to the pinvoke IL stub and
    // for this architecture, the inlined call frames are supposed to be processed before the managed frame they are stored in.
    // So we force the stack frame iterator to process the InlinedCallFrame before the IL stub.
    _ASSERTE(InlinedCallFrame::FrameHasActiveCall(m_crawl.pFrame));
    m_crawl.isFrameless = false;
    }
    #endif

    The assumption is that if previous frame was "reverse P/Invoke" and next frame is managed that we skipped over the InlinedCallFrame. Unfortunately, the condition triggers early for the finally jump trampoline, so it reports the InlinedCallFrame but never reports the finally jump trampoline itself and that happens to be the one in m_sfParent.

I am not sure how to adequately detect the condition. I'm not inside a funclet so I cannot condition it on that (on second thought, maybe I should see it as funclet?!). I could save mCrawl.pFunc and detect that it didn't change but that would still be broken if [UnmanagedCallersOnly] method somehow managed to call itself.

@filipnavara

filipnavara commented May 25, 2025

Copy link
Copy Markdown
MemberAuthor

Hmm, this does actually make a difference:

--- a/src/coreclr/vm/stackwalk.cpp+++ b/src/coreclr/vm/stackwalk.cpp@@ -3153,9 +3153,19 @@ void StackFrameIterator::PostProcessingForManagedFrames(void)
#endif // ELIMINATE_FEF
#ifdef TARGET_X86
+#ifdef FEATURE_EH_FUNCLETS+ bool hasReversePInvoke = false;+ if (!m_crawl.codeInfo.IsFunclet())+ {+ hdrInfo *gcHdrInfo;+ m_crawl.codeInfo.DecodeGCHdrInfo(&gcHdrInfo);+ hasReversePInvoke = gcHdrInfo->revPInvokeOffset != INVALID_REV_PINVOKE_OFFSET;+ }+#else
hdrInfo *gcHdrInfo;
m_crawl.codeInfo.DecodeGCHdrInfo(&gcHdrInfo);
bool hasReversePInvoke = gcHdrInfo->revPInvokeOffset != INVALID_REV_PINVOKE_OFFSET;
+#endif // FEATURE_EH_FUNCLETS
#endif // TARGET_X86
ProcessIp(GetControlPC(m_crawl.pRD));

...but I still get a GC hole. In fact, I get it even more deterministically.

@filipnavara

Copy link
Copy Markdown
MemberAuthor

I think this time it's the environment variable CORE_ROOT that I wasted hours on 😅

@filipnavara

Copy link
Copy Markdown
MemberAuthor

I think it's ready for another pass.

@jkotas

Copy link
Copy Markdown
Member

/azp run runtime-coreclr outerloop

@jkotas

Copy link
Copy Markdown
Member

/azp run runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@filipnavara

Copy link
Copy Markdown
MemberAuthor

We finally have a clean x86 run of the tests 🥳

@jkotasjkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for making this happen! Great work!

@jkotas

Copy link
Copy Markdown
Member

/ba-g infrastructure and known issues

@jkotas
jkotas merged commit be67510 into dotnet:mainMay 26, 2025
@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks for the help diagnosing the issues, and thanks everyone for helping to make this happen!

@filipnavara
filipnavara deleted the x86funclets branch May 26, 2025 08:42
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jun 25, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-ExceptionHandling-coreclronly use for closed issuescommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

New exception handling on win-x86

4 participants

@filipnavara@jkotas@am11@SingleAccretion