[Interpreter] Raise a pending thread abort again after a catch handler

[pcshrosbree]

Fixes #135463.

When interpreted code catches a ThreadAbortException, the abort has to be raised again at the end of the catch handler. The interpreter does that in the ResumeAfterCatchException handler of InterpExecMethod, and it had two problems there:

1. It called COMPlusCheckForAbort without setting up Thread::m_OSContext, which COMPlusCheckForAbort leaves to its caller. Thread::ReadyForAsyncException asserts that the context points at managed code and walks the stack from it. EECodeManager::ResumeAfterCatch sets it up for compiled code. Now the interpreter fills it from the interpreter frame at the resume IP, with InterpreterFrame::SetContextToInterpMethodContextFrame.
2. It dispatched the abort from inside the C++ catch block. When a frame of the same InterpExecMethod catches that abort, the ResumeAfterCatchException that resumes it is thrown while the catch block is still active, so the try block does not catch it (Checked: assert pFrame->PtrNextFrame() == NULL in exceptionhandling.cpp). Now the catch block sets a flag and the check runs at the top of the try block.

The abort is still raised as a return address at the resume IP, as before, so a try block that starts right at the resume point does not cover it. That matches optimized JIT code, which resumes on a nop in front of such a try block.

Tests:

• New src/tests/Regressions/coreclr/GitHub_135463: the abort caught in the method, caught again by its caller, and kept pending at the end of a catch inside a finally. It checks the exception and which blocks ran.
• ControlledExecutionTests.CancelInTryAndExitCatchNormally is enabled again. CancelItselfOutsideOfTryCatchFinally stays disabled: under DOTNET_InterpMode=1 it fails for a different reason, an abort that compiled code raises again through ThrowControlForThread and that is then resumed in an interpreted frame. I'll open a separate issue for that.

Validation on linux-x64, osx-arm64 and windows-x64 (Checked):

	main	this PR
test135463, JIT	pass	pass
test135463, DOTNET_InterpMode 1/2/3, with and without TieredCompilation=0	assert in ReadyForAsyncException	pass
ControlledExecutionTests (9 tests), JIT	9 pass	9 pass
ControlledExecutionTests (9 tests), DOTNET_InterpMode=1	process dies (same assert)	9 pass

On windows-x64 I also ran ControlledExecutionTests one test per process under DOTNET_InterpMode 2 and 3, with a local workaround for #135535, which otherwise stops the tests during discovery. On main, CancelInTryAndExitCatchNormally hits the same assert. With this PR, all 9 pass.

The new test also passes under the JIT with TieredCompilation=0, JITMinOpts, JitStress 1 and 2, ReadyToRun=0, and GCStress 0x3 and 0xC. Each part of the change was taken out in turn and the test fails under the interpreter every time: without the m_OSContext setup it hits the assert above, without recording the resume IP it hangs, and with the check left in the catch block it hits the PtrNextFrame assert.

Note

This PR was prepared with AI assistance (Anthropic Claude, reviewed by OpenAI Codex). The builds and runs it cites were done at the PR's base commit.

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 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, @BrzVlad, @janvorli
See info in area-owners.md if you want to be subscribed.

[BrzVlad]

Ideally, we should have no special checks on this execution path and keep it as simple as possible, just so we can have the best perf possible, even if it is just a var init and a check for it.

In the context of this PR, isn't this implementation basically an implicit rethrow of the abort exception ? Sounds like we could avoid resuming altogether and throw the abort exception earlier, while executing the catch from EH, same as a rethrow I believe is processed. Maybe we could add a check for thread abort somewhere around INTOP_LEAVE_CATCH. cc @janvorli

[pcshrosbree]

Agreed on the principle, and it was a little worse than one init and one test per entry: EXIT_FRAME re-enters the try through goto MAIN_LOOP on every return from a non-recursive interpreted call, so the flag was tested about once per interpreted call, not once per entry. 35f8910 takes it off that path entirely. To answer the second half properly, on Checked linux-x64 I also prototyped the INTOP_LEAVE_CATCH version and ran both through the same tests, so here is what I found.

Why it is not a rethrow. A rethrow re-raises the caught exception from inside the handler while its dispatch is still in flight. The abort re-raise is defined to happen after the catch has completed and its EH state is gone, at the resume point: CallCatchFunclet calls the funclet, then ExInfo::PopExInfos, then ResumeAfterCatch, and COMPlusCheckForAbort relies on that order. For a non-rude abort it returns early while GetThrowable() is non-null, which it is for the whole catch funclet, and ReadyForAbort rejects a context classified inside a catch handler on the aborting thread itself (TAStackCrawlCallBackWorker skips non-finally clauses only when the aborter is another thread). So a check at INTOP_LEAVE_CATCH that calls the existing helper never fires: with a trace line at the raise site it raised zero times in every run, and CancelInTryAndExitCatchNormally fails because the code after the catch runs.

Making it fire needs the parent InterpreterFrame plumbed into ExceptionClauseArgs for the resume-point context, a variant of the check without the pending-throwable clause, and a different walk start frame (the funclet's frames sit below the resume SP, so the existing computation lands on the owning InterpreterFrame and StackFrameIterator::Init asserts). With all of that the abort does get raised from the funclet, and then it is caught by the same catch again: the leavecatch island sits at the try's end offset, the new first pass starts from there, the handler runs again, raises again, and the native stack overflows after about four hundred nested dispatches. The same happens if the check is reduced to IsAbortRequested(). Where an enclosing try exists, its catch takes the abort first; the shape, with the traces as measured:

try
{
    try { RunInfiniteLoop(); }
    catch (ThreadAbortException) { s_trace += "inner catch "; goto Done; }
}
catch (ThreadAbortException) { s_trace += "outer catch "; }
Done:
s_after = true;

	JIT (tiered)	flag at the try top (previous revision)	check on the resume path (this revision)	raise at INTOP_LEAVE_CATCH
trace	inner catch	inner catch	inner catch	inner catch outer catch outer catch ... until the watchdog fires

This revision matches the tiered JIT in that example, but other JIT configurations also differ from each other there (with tiering off the outer catch runs), so I would not call it a universal compiled-code comparison. What does hold from the code is that raising before the funclet returns bypasses the handler's normal post-return callbacks and keeps each re-raise nested on the native stack, and the loop above shows the dispatch has no notion of a completed handler to skip. I favour keeping the check at the resume point; the funclet prototype and its plumbing are on hand if you want to look at that route anyway.

What this revision does instead. The catch block for ResumeAfterCatchException only records the resume SP and IP and jumps to a label outside any try, which does the unwind as before, describes the resume point in m_OSContext, and calls the NOTHROW COMPlusCheckForAbort. If the abort is due it jumps to a second, cold try with its own dispatcher and unwind handler, calling DispatchManagedException(kThreadAbortException); that block's catch funnels back into the same label, so a further catch in the same InterpExecMethod that swallows the abort again is handled the same way. The resumedAfterCatch local and its test are gone; the main loop's entry and return paths are what they were before the first revision of this PR. The abort is still raised at the resume point after the EH state has been popped, where EECodeManager::ResumeAfterCatch raises it for compiled code.

Measured on Checked runtimes: test135463's three cases pass under the JIT and DOTNET_InterpMode 1, 2 and 3 on linux-x64, osx-arm64 and windows-x64, and ControlledExecutionTests passes under modes 1, 2 and 3 on linux-x64, 1 and 3 on osx-arm64 and 1 on windows-x64, with Linux interpreter marker counts confirming the new raise site. On linux-x64, in interpreter modes 1 and 3, nine abort shapes (the one above, catch inside finally, catch then finally, abort requested during a catch, a filter on an enclosing catch, consecutive trys, the stack-trace shape, and the test's three) give identical traces for the previous revision and this one in every cell. On the entry-path cost, in a linux-x64 Release build the instruction path from an interpreted return back to the first opcode dispatch goes from 37 to 28 instructions, and the function-entry path is unchanged within one instruction (register allocation moved); on linux-x64 Checked and Release, InterpMode 1 with tiering disabled, a 20M-iteration interpreted call/return loop and a 2M-iteration native-to-interpreter entry loop show median differences between the two revisions within the run-to-run spread, so I would not claim a measurable win, only that nothing abort-specific executes there any more.

One thing the comparison turned up about the test itself: under InterpMode 2 and 3 the s_trace += "unreachable " after each catch is an interpreted String.Concat, and a runtime that never re-raised at the end of the catch still passed those three cases there, consistent with a safepoint of that call raising the pending abort before the store (measured with the check removed, Checked linux-x64; the same runtime fails under mode 1 and in ControlledExecutionTests). 7e70fe0 records that point with a call-free store and asserts on it; with the re-raise removed the test now fails in all tested interpreter configurations on Checked linux-x64.

For completeness I also tried raising from InterpreterCodeManager::ResumeAfterCatch directly, mirroring EECodeManager: the interpreter's frame chain is unwound only in InterpExecMethod's catch, so the new first pass starts at the original throw site, the same catch handles the abort again, and CatchInsideOfFinally runs its finally twice.

Note

This reply and the two commits were prepared with AI assistance (Anthropic Claude, checked by OpenAI Codex) at my request. I reviewed the change and the measurements before posting.

[pcshrosbree]

A clarification of the LeaveOutOfOuterTry comparison in my previous comment, where this revision matched the tiered JIT and I noted that the JIT configurations differ from each other there. The match with tier-0 code was real, but that tier-0 result comes from a JIT issue I have since reported as #135637, with a proposed fix in #135638: in tier 0 the block the catch returns to generates no code, so the abort is dispatched as if it had been raised in the code after the enclosing try. With #135638, tier 0, DOTNET_JITMinOpts=1 and DOTNET_TieredCompilation=0 all give inner outer. That is also the behaviour the JIT intends: when a catch's leave exits an enclosing try that has a catch, impImportLeave makes the catch return to a step block inside that try, and its comment says why (importer.cpp):

If a try/catch catches a ThreadAbortException (which might be because it catches a parent, e.g. System.Exception), and the catch doesn't call System.Threading.Thread::ResetAbort(), then when the catch returns to the VM, the VM will automatically re-raise the ThreadAbortException. When it does this, it uses the target address of the catch return as the new exception address. That is, the re-raised exception appears to occur at the catch return address. If this exception return address skips an enclosing try/catch that catches ThreadAbortException, then the enclosing try/catch will not catch the exception, as it should.

[...]

Note that this pattern isn't theoretical: it occurs in ASP.NET, in IL code generated by the Roslyn C# compiler.

Compared with the JIT built with #135638, the interpreter with this PR differs on that shape: inner under DOTNET_InterpMode=1 and 3, where the JIT gives inner outer (Checked linux-x64; builds of this PR's runtime change at 35f8910 and at the previous revision 08e3dfb, each with a trace line added at the raise site; three runs per mode and build, all identical). The other two shapes of #135637 match the fixed JIT (catch finally outer, with the finally run, and inner).

From the source and an InterpDump, the difference comes from where the interpreter compiler sends the catch, not from this PR's raise. CreateLeaveChainIslandBasicBlocks creates a leave island for each catch handler and each finally-protected try that a leave exits, and none for an enclosing try protected by a catch, so the inner catch's leavecatch goes straight to the leave target, past the outer try. DOTNET_InterpDump=LeaveOutOfOuterTry with DOTNET_InterpMode=1 and this PR (excerpt, comments mine):

0018: IR_0004: call           [0 <- 16], .AbortReraise:RunInfiniteLoop()
0028: IR_0008: br             [nil <- nil], IR_000c
0030: IR_000a: leavecatch     [nil <- nil], IR_0010    ; end of the inner catch
0038: IR_000c: br             [nil <- nil], IR_0010
0040: IR_000e: leavecatch     [nil <- nil], IR_0010    ; end of the outer catch
0048: IR_0010: ldc.i4         [0 <- nil], 1            ; Done:
...
  try [IR_0004(0), IR_000a(7)) catch handler [IR_001c(7), IR_0035(1e))
  try [IR_0004(0), IR_000e(20)) catch handler [IR_0035(20), IR_004e(37))

The outer try is [IR_0004, IR_000e). The inner catch resumes at IR_0010, and the instruction just before that is the outer catch's leavecatch at IR_000e, already outside the range, so for this shape the outer catch is skipped whether handlers are looked up at the resume address or just before it.

I don't think this should hold up this PR, and I don't know whether the interpreter means to follow the JIT here. If it does, a step island inside the enclosing try for the catch to return to, as the JIT has, looks like the natural shape. I'm happy to open an issue for it or try a change, whichever you prefer. Until then the regression test in #135638 runs on jitted code only, for this reason.

Note

This reply was prepared with AI assistance (Anthropic Claude, checked by OpenAI Codex) at my request. I reviewed it before posting.

[janvorli]

@pcshrosbree I guess I can see the problem with the leave island you have shown. I think my original idea was that the catch leave island would be the resume after catch target. So the catch would end with INTOP_LEAVE_CATCH pointing to that island and that island would contain plain INTOP_BR. Then we would resume after catch there and rethrowing of the thread abort exception from that place would still be covered by the right try block.
So I've essentially flipped the INTOP_LEAVE_CATCH and INTOP_BR at the end of the catch and the catch island.
I guess that then what @BrzVlad has suggested in #135520 (comment) would work.

[pcshrosbree]

@janvorli @BrzVlad, the flip gives the layout you describe. On its own, though, it makes this PR loop, so the abort raise has to change with it. I prototyped two ways to do that.

Which do you want: raise at the resume point, or raise at leavecatch? This PR already raises on the resume path: the first option keeps that and marks the frame faulting first, and the second moves the raise to the end of the catch. And should the flip land in this PR or separately? If the flip lands after this PR as it stands, it needs a faulting fix like the resume-point option's, or it loops. The leavecatch raise, as prototyped, needs the flip.

Option	What it changes, relative to this PR's head (7e70fe0)	Test outcome	Size
Flip alone (not a candidate)	the flip, with this PR's existing raise on the resume path unchanged	loops	n/a
Raise at the resume point	the flip, plus pInterpreterFrame->SetIsFaulting(true); before this PR's existing dispatch, as INTOP_THROW and INTOP_RETHROW do; the rest of this PR stays	passes, with the coverage limits below	2 files, +11/−4
Raise at leavecatch	the flip, plus raising the abort at INTOP_LEAVE_CATCH (BrzVlad's suggestion), and the plumbing it requires	passes, with the coverage limits below	7 files, +97/−39

I prototyped all three on Checked runtimes on linux-x64, osx-arm64 and windows-x64. Sizes exclude trace lines.

Two other observations relevant to the choice:

• Stack trace. I checked this with a test case where an outer catch catches the re-raised abort. For the first frame of that abort's stack trace, the leavecatch raise reports a location inside the catch handler. The resume-point raise and this PR's head report the try's closing brace, and the JIT reports the call inside the try. The second frame agrees in all of them.
• Dispatch path. Neither option adds an instruction on the return path to the opcode dispatch (Checked disassembly). I draw no performance conclusion from Checked timings.

The flip

In both test cases from #135871, the catch now ends with leavecatch to its island. The island holds br and lies inside the enclosing try, so the catch resumes inside the outer try. Only opcodes swap: in the compared dumps, no IR offset or EH range moves.

Why the flip alone loops

With the flip alone, test135463 hangs in all six of its interpreter configurations (the seventh runs under the JIT), 8 of 10 abort test cases loop, and one more runs a finally twice, on all three platforms.

My reading of the code, consistent with the traces (I did not trace the lookup itself): the resume path clears the frame's faulting flag, so handlers are looked up at the instruction before the island. When the island comes right after a try body, that instruction is the try's last one, so the same catch takes the abort again. Marking the frame faulting before the raise, which is what the resume-point option adds, removes the loops.

What the leavecatch raise needs

With the flip, the loop from my 10-12 comment is gone, because the leavecatch now ends the handler. The other two obstacles from that comment remain:

1. These runs do not use a rude abort. The existing COMPlusCheckForAbort never raises at INTOP_LEAVE_CATCH, because the thread's throwable is set for the whole funclet. So the option needs a variant of the check without the pending-throwable clause. Most of the option's +51 lines in excep.cpp are that variant, an adapted copy of COMPlusCheckForAbort.
2. It needs more plumbing:
   • m_OSContext has to describe the resume point in the parent frame, which means passing the parent InterpreterFrame in ExceptionClauseArgs;
   • ReadyForAsyncException needs a start-frame fix. If I leave out only that fix, StackFrameIterator asserts (m_crawl.pFrame > pOwning).

The leavecatch raise also removes this PR's resume-path check and the ABORT_AFTER_CATCH block.

Test results

On all three platforms, the two candidates:

• pass test135463 in all seven configurations;
• pass the nine enabled ControlledExecutionTests, except under InterpMode 2 and 3 on windows-x64, where xunit discovery hits a pre-existing Thread::IsObjRefValid assert (with this PR's head too);
• give the expected traces under InterpMode 1 and 3 in the ten abort test cases and both nested-catch test cases. This includes the two [Interpreter] A thread abort raised again after a catch skips the catch of the enclosing try #135871 test cases that this PR's head misses.

I also tested a try/finally inside a catch with a leave out of both, and an empty catch. For each, this PR's head and the two candidates match the Linux and macOS JIT reference on all three platforms. With the flip, the empty catch's handler is load.exception; leavecatch.

I have not tested Release runtimes.

Other findings

• linux-x64, InterpMode 1: a thread cancelling itself still terminates. That is pre-existing, [Interpreter] ResumeAfterCatchException escapes ThrowControlForThread when compiled code raises a thread abort again and an interpreted frame catches it #135532. [Interpreter] Install the resume-after-catch handler in ThrowControlForThread #135533's handler commit fixes the termination in my runs on this PR's head and with the leavecatch raise; I have not run it with the resume-point raise.
• wasm. I used single-threaded self-abort versions of the test cases.
  • With everything interpreted, the wasm configurations I compared match linux-x64 InterpMode 3.
  • With browser's default ReadyToRun CoreLib, this PR's head, the two candidates and main at 3994df1ceee1 all let ThreadAbortException escape in the two test cases where the thread aborts itself directly, with no interpreted catch. The walk that decides whether the thread can take the abort moves a ReadyToRun wasm frame's virtual IP back by one and misses the handler. I filed this separately as [wasm] Thread abort is raised inside finally and catch handlers of R2R code, so ControlledExecution.Run lets ThreadAbortException escape #135914, and Do not adjust wasm virtual IPs in the thread abort stack walk #135915 fixes this part of it. The issue also describes a second wasm problem, the abort re-raise at the end of a catch in ReadyToRun code, where I asked for direction.
  • I have not tested wasi with a ReadyToRun CoreLib.

I'll update the PR to the option you choose and add the #135871 test cases to its test.

Note

This reply was prepared with AI assistance (Anthropic Claude, checked by OpenAI Codex) at my request. I reviewed the measurements before posting. No commits are pushed with it; the prototypes are local.

[jkotas]

Use ConditionalFact instead

[pcshrosbree]

Done in 0125aaa.

[BrzVlad]

I think it would be easier to see the 2 implementations side by side, rethrowing at resume point vs rethrowing during catch leave, while also addressing any other abort related issues (like #135871). Unless @janvorli has a strong preference. It's getting painful to follow this text.

[janvorli]

I agree with @BrzVlad that it is hard to reason about a combination of changes. Maybe you could create a new PR with the flip and extra changes needed with that so we can easily see it in its completeness.

And I've realized that re-raising thread abort should really happen at the resume point, so that it behaves like JITted code. After the flip, the INTOP_LEAVE_CATCH is in the catch itself and that place is not protected by any try region (which is intentional).

[pcshrosbree]

@janvorli, @BrzVlad: #135939 has the whole change in one commit based on main: the flip, the abort raised again at the resume point with the frame marked faulting, and the tests, including both cases from #135871. I'll close this PR in its favour.

I had also prepared the two versions side by side as draft PRs, one raising the abort at the resume point and one at the INTOP_LEAVE_CATCH that ends the catch. I can post those as well if you'd still like to compare them.