JIT: make sure the block a catch returns to generates an instruction

[pcshrosbree]

When a catch block ends with a thread abort pending, the runtime raises the abort again from the address the catch returns to and looks the handlers up for that address (EECodeManager::ResumeAfterCatch → COMPlusCheckForAbort → ThrowControlForThread, which dispatches from a FaultingExceptionFrame at the exact resume address). The JIT accounts for that: impImportLeave places the block a catch returns to inside the EH regions that enclose the catch (see its comment on ThreadAbortException), and fgOptimizeEmptyBlock keeps that block when it is empty and in a different EH region than its successor, and adds a GT_NO_OP to it, "for re-raise of thread abort exceptions to work".

That only works if the block generates an instruction, and whether it does is only known in codegen. When it generates none, the resume address belongs to the code that follows, and the abort is dispatched as if it had been raised there. This happens in unoptimized code (tier 0, JITMinOpts, debuggable code), where fgOptimizeEmptyBlock does not run, and in optimized code too:

• the block can contain nodes that generate no code, so fgUpdateFlowGraph never passes it to fgOptimizeEmptyBlock: a block whose only statement is GC.KeepAlive of an argument;
• the block can be the first block of a try region, which fgUpdateFlowGraph skips (BBF_DONT_REMOVE): in a loop of try/catch inside another try, the catch returns to the loop head, an empty block that begins the outer try and falls into the inner one, so the abort is raised again inside the inner try and the same catch runs again;
• on xarch, the block's only instruction can be a jump that emitRemoveJumpToNextInst removes because the blocks between it and its target generate no code (the step block of LeaveOutOfOuterTry below, at tier 0).

In the shapes of #135637 a finally the catch's leave passes through does not run, an enclosing catch does not see the abort, and a try that starts right after the catch does.

This change makes genEmitEndBlock guarantee that the block generates an instruction: if a BBJ_ALWAYS block that a catch returns to has emitted nothing since its label, it emits a nop when the jump to the next block is omitted, and otherwise does not mark the jump as removable. The check does not depend on the EH region of the next block, because when that block emits nothing either, the address belongs to a later one: a variant that acted only when the next block, or the jump's target, was in another EH region left 7 catch-return targets with mismatched EH membership in the SuperPMI replay below (tier-0 instantiations of System.Numerics.Tensors.Tests.Helpers.AssertEqualWithTolerance). fgOptimizeEmptyBlock still adds its GT_NO_OP: an earlier revision of this PR removed it, and Wasm crossgen then failed because fgUpdateFlowGraph redirected the resume pad that fgWasmEhFlow adds past the now empty block, leaving it reachable only from the catch return.

What changes

Checked builds on linux-x64, the JIT from this branch against a JIT built at main 523a36f on the same runtime; three runs per configuration and shape, identical each time. The table uses standalone copies of the test's six shapes without AggressiveOptimization, so that each configuration applies to them (the copy of CatchInLoop also appends a marker before its break); "finally ran" is a call-free volatile store.

shape	tier 0 before	JITMinOpts=1 before	TieredCompilation=0 before	after
CatchThenFinally	catch outer, finally not run	same as tier 0	correct	catch finally outer in all three
LeaveOutOfOuterTry	inner	inner	correct	inner outer in all three
ConsecutiveTry	inner new catch	inner new catch	correct	inner in all three
KeepAliveAfterCatch	correct	correct	inner	inner outer in all three
KeepAliveThenFinally	correct	correct	catch outer, finally not run	catch finally outer in all three
CatchInLoop	correct	the catch runs 3 times	the catch runs 3 times, then outer	catch outer in all three

At tier 1 the two KeepAlive shapes fail as with TieredCompilation=0 and pass with this change. On osx-arm64 and windows-x64 the same shapes fail with a JIT without this change, except that LeaveOutOfOuterTry is correct on arm64, which has no late emitter pass that removes this jump across empty blocks, and all pass with this change.

In the tier-0 JitDump of CatchThenFinally, the catch-return block BB03 is now a one-byte nop emitted in codegen at 0x1B, and the finally's try range covers it (EH#1: try [0014..001C); before: [0014..001B), no code for BB03). In LeaveOutOfOuterTry the step block keeps its two-byte jmp (EH#1: try [0014..001E); before, emitRemoveJumpToNextInst removed it and the range was [0014..001C)). In optimized code the first three shapes compile as before; for KeepAliveAfterCatch the block gets a nop that the enclosing try covers (EH#1: try [000E..001D); before: [000E..001C)), and in CatchInLoop the catch returned to 0x0F, the first instruction of its own try (EH#0: try [000F..0017)), and now returns to a nop there, with the try starting at 0x10.

Tests

Regressions/coreclr/GitHub_135637/test135637 has the six shapes. KeepAliveAfterCatch, KeepAliveThenFinally and CatchInLoop are marked AggressiveOptimization, so the default configuration compiles them optimized; JITMinOpts=1 still exercises MinOpts. With the current JIT the test fails under the default configuration, JITMinOpts=1 and TieredCompilation=0; with the first version of this PR it fails under the default configuration and TieredCompilation=0; with this change it passes in all three, on linux-x64, osx-arm64 and windows-x64 (Windows needs #135531, now merged, on machines with CET shadow stacks). Removing either half of the check makes the test fail: without the nop under all three configurations (linux-x64 and osx-arm64), and with the jump left removable under the default configuration and JITMinOpts=1 (at LeaveOutOfOuterTry, linux-x64).

Under the interpreter the test is skipped with an [ActiveIssue] for #135871: there the outer catch does not see the abort in LeaveOutOfOuterTry and CatchInLoop (measured with #135520 applied; without it, Checked builds stop at the assert of #135463).

JIT/Methodical (Methodical_d2, _r2, _do, _ro, _others), JIT/jit64 (jit64_5) and baseservices/exceptions on linux-x64 under the default configuration, TieredCompilation=0 and JITMinOpts=1, with the JIT before and after the change: no result differs. 74 of the 79 runner scripts pass with both JITs, and the same five (baseservices/exceptions/inproccrashreport/*) fail with both on that machine.

To look for the problem more widely I used a local diagnostic (not part of this PR). For methods without hot/cold splitting, it compares each catch-return target's address after codegen with the reported try and handler ranges in the same funclet. Replaying the linux-x64 SuperPMI collections records 2,421 mismatches with the current JIT: 2,411 in unoptimized code, and 10 in optimized code in 6 methods. In the optimized cases inspected, including WebSocketHandle.ConnectAsync, Kestrel's SocketConnectionListener.AcceptAsync and OpenTelemetry's ProtobufOtlpResourceSerializer.SerializeResourceToBytes, the catch returns to the first instruction of a try that does not contain the block. With this change there are no recorded mismatches. Over the EH test trees above the diagnostic records 724 mismatches under the default configuration and 738 under JITMinOpts=1 with the current JIT, and none with this change.

SuperPMI asm diffs, Checked JITs: on linux-x64, 1,591 of 3,267,793 contexts change, MinOpts +2,659 bytes and FullOpts +10 bytes, +2,669 in total; on linux-arm64 with the cross-compiled JIT, 1,488 of 3,124,627 contexts change, MinOpts +8,796 and FullOpts +32, +8,828 in total; browser-wasm has no replay failures and no code-size change. Tier-0 code size, jitting with ReadyToRun=0 (CoreLib and the repro program, three runs per JIT, linux-x64): of 2,468 tier-0 methods whose size is stable across the runs, 4 grow (the three shapes of the repro and System.Diagnostics.StackFrameHelper:InitializeSourceInfo, by one byte each except LeaveOutOfOuterTry, which keeps a two-byte jmp); with TieredCompilation=0 the 958 compared methods are unchanged. On arm64 the nop is four bytes.

Resolves #135637

Note

This PR was prepared with AI assistance (Anthropic Claude, checked by OpenAI Codex) at my request. I reviewed the change, the test and the measurements before opening it.

[azure-pipelines]

Azure Pipelines:
Successfully started running 5 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[JulieLeeMSFT]

@EgorBo PTAL.

[EgorBo]

@jkotas what is the status of ControlledExecution.Run ? I see it's marked as ControlledExecution.Run method may corrupt the process and should not be used in production code. in the docs

[jkotas]

what is the status of ControlledExecution.Run ?

#135463 (comment)

[EgorBo]

This change moves the check into a helper, fgKeepCatchRetTargetNonEmpty, and runs it from fgPostImportationCleanup when optimizations are disabled.

I don't think this belongs in the IR, especially not right after import. Checking isEmpty() plus the region of the jump target only guesses whether the block will produce code. What we need is that the catchret target emits at least one instruction, and only codegen knows that for sure. The check can go in genEmitEndBlock: if a BBJ_ALWAYS catchret target hasn't emitted anything, emit a nop when the jump to the next block is dropped. Otherwise, keep the jump: on xarch, emitRemoveJumpToNextInst can remove it later and leave the block empty again.

fgOptimizeEmptyBlock calls the same helper, so optimized code is unchanged.

Optimized code has the same bug, I don't see why not (meaning this PR doesn't fix it).

[pcshrosbree]

Thanks @EgorBo, I've moved the check to genEmitEndBlock. If a BBJ_ALWAYS catchret target has emitted nothing since its label, codegen emits a nop when omitting its jump to the next block; otherwise it keeps the jump out of xarch's emitRemoveJumpToNextInst candidates. Both cases occur in the test. The post-import GT_NO_OP is gone, as is the one from fgOptimizeEmptyBlock; the latter still preserves the block.

You're right about optimized code too. The test now covers a continuation containing only GC.KeepAlive of an argument, which is nonempty in the IR but emits no code, and an empty loop head at the start of an enclosing try, which fgUpdateFlowGraph skips. Both fail with the previous version of this PR and pass with this change. The three added methods use AggressiveOptimization to cover optimized code in the default run.

I pushed the codegen change, the added test shapes, and an [ActiveIssue] for the separate interpreter failure (#135871). The test passes under the default configuration, JITMinOpts=1 and TieredCompilation=0 on linux-x64, windows-x64 and osx-arm64. The PR description has the measurements and code-size results.

[jkotas]

It is better to use ConditionalFact for this (nameof(PlatformDetection.IsCoreCLR)). RequiresProcessIsolation tests are much more expensive to run

[pcshrosbree]

Done in 6dc56e4: the test now skips non-CoreCLR runtimes with ConditionalFact and runs in the merged Regressions runner. Since it aborts threads in that shared process, I ran the whole Regressions group with and without it on linux-x64, osx-arm64 and windows-x64: no other test changed outcome.

[pcshrosbree]

Also removed the remaining NativeAotIncompatible in eef3171; the IsCoreCLR condition already excludes NativeAOT.

[pcshrosbree]

The codegen rework broke the CoreCLR browser-wasm and wasi-wasm builds, so I've restored the GT_NO_OP that fgOptimizeEmptyBlock adds; my comment above said it was gone. Without it the catch continuation is empty in the IR. On Wasm, fgWasmEhFlow adds a resume pad for each catch continuation, and the fgUpdateFlowGraph call in fgLateLiveness then redirected the pad past the empty block, leaving it reachable only from the catch return. A local Checked-JIT reproduction of the CoreLib crossgen failure hit NYI_WASM: Method has blocks only reachable via EH; CI's browser-wasm SuperPMI replay hit the same assertion. My earlier SuperPMI runs had covered only x64 and arm64.

With 98d74ae the net JIT change is only the check in genEmitEndBlock and its helper. Locally, crossgen of all of browser CoreLib succeeds, as does compiling the failing methods with --targetos:wasi using browser CoreLib IL. The browser-wasm SuperPMI replay has no failures and no code-size change. The test still passes under default, JITMinOpts=1 and TieredCompilation=0 on linux-x64. I've updated the code-size figures in the description: x64 is now +2,669 bytes and arm64 +8,828 bytes, instead of -37 and -52.