Repository navigation
[Interpreter] Raise a pending thread abort again at the resume point after a catch - #135939
Merged
jkotas merged 3 commits intoOct 19, 2026
Merged
Conversation
When interpreted code catches a ThreadAbortException, raise the abort again where the execution resumes after the catch, as EECodeManager::ResumeAfterCatch does for compiled code: - Set up Thread::m_OSContext for the resume point before calling COMPlusCheckForAbort, which expects its caller to do that (dotnet#135463). - Dispatch the abort from a try block of its own, outside the C++ catch block for ResumeAfterCatchException, so that the execution can resume again when the abort is caught again in the same InterpExecMethod. - Make the catch leave island the resume point. Within a leave, every edge into a catch leave island uses INTOP_LEAVE_CATCH; every other edge, including the final edge to the leave target, uses INTOP_BR. The catch therefore ends with INTOP_LEAVE_CATCH to its island. The island is inside every try region that encloses the catch's try region. The leave target need not be inside all of those regions (dotnet#135871). - Mark the frame faulting before the dispatch, as INTOP_THROW does, so that the handlers are looked up at the resume IP itself rather than at the instruction before it. Add the test GitHub_135463. Its two dotnet#135871 cases run only when the test is interpreted, because jitted code gets them wrong until dotnet#135637 is fixed. Enable ControlledExecutionTests.CancelInTryAndExitCatchNormally again, and stop skipping test135530 under the interpreter. Fixes dotnet#135463 Fixes dotnet#135871
|
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. |
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @BrzVlad, @janvorli |
janvorli
reviewed
Oct 16, 2026
Remove the goto at the end of the catch of ResumeAfterCatchException, which only jumped to the next statement. Move the block that raises the abort again to just after the resume code, so that a pending abort falls through into it instead of jumping to a label after EXIT_FRAME. No change in behavior.
Contributor
Author
|
For context, here is where the related thread-abort work stands:
Note This comment was prepared with AI assistance (Anthropic Claude, checked by OpenAI Codex) at my request. I reviewed it before posting it. |
This was referenced Oct 16, 2026
Closed
Contributor
Author
|
@jkotas janvorli approved this, but Build Analysis is still red from the run on October 16. The CI failures I checked include:
|
BrzVlad
approved these changes
Oct 19, 2026
Contributor
Author
|
Thank you for the review and the merge. |
lewing
pushed a commit
that referenced
this pull request
Oct 20, 2026
…ormally (#136116) > **TL;DR:** Since #135939 merged, `System.Runtime.Tests` times out in browser-wasm CI. The likely cause is a test that needs threads but now runs on single-threaded wasm, where it cannot finish. This one-line change skips that test, `ControlledExecutionTests.CancelInTryAndExitCatchNormally`, when threads are unavailable, as the other `Task.Run` tests in its class do. This PR's own browser CI run will show whether that resolves the timeout. The test cancels its action from a `Task.Run` task while the action spins. #135915 and #135939 merged a minute apart, and each changed this class: - #135915 moved `IsMultithreadingSupported` from the class onto the four enabled tests that cancel from `Task.Run`. - #135939 enabled `CancelInTryAndExitCatchNormally` again, but its branch predated #135915, so on main the test has no such condition. On single-threaded wasm, the canceling task cannot run while the action spins. In the browser-wasm `LibraryTestsCoreCLR` leg, `System.Runtime.Tests` had these results: - Without #135939: passed in 872 s (build [1641014](https://dev.azure.com/dnceng-public/public/_build/results?buildId=1641014)) and 972 s (build [1641108](https://dev.azure.com/dnceng-public/public/_build/results?buildId=1641108)). - With #135939: timed out at 5402 s in both builds [1641112](https://dev.azure.com/dnceng-public/public/_build/results?buildId=1641112) and [1641117](https://dev.azure.com/dnceng-public/public/_build/results?buildId=1641117). Under `src/libraries`, #135939 changed only this test's attribute. The log does not name the test that was running. Both PRs are mine, and I missed the interaction before they merged. > [!NOTE] > This PR was prepared with AI assistance (Anthropic Claude, checked by OpenAI Codex) at my request. I reviewed it before posting it.
janvorli
pushed a commit
that referenced
this pull request
Nov 2, 2026
…orThread (#135533) > **TL;DR:** With `DOTNET_InterpMode=1` (only some methods interpreted), a `ControlledExecution` action that cancels its own token terminates the process or hits an assert. This PR fixes that and adds a regression scenario. It also enables an existing self-cancellation test again, except on browser CoreCLR, where #135914 still applies. It is rebased onto main. Fixes #135532. **Rebased onto main now that #135939 is in.** The first commit is the fix. The second skips `CancelItselfOutsideOfTryCatchFinally` on browser CoreCLR. Since #135915 removed the class-level multithreading requirement from `ControlledExecutionTests`, this test would otherwise also run on single-threaded browser-wasm, where #135914 still applies. When compiled code raises a thread abort again at the end of a catch handler, it does so through `ThrowControlForThread`. Before this change, when an interpreted frame further up caught the abort, the `ResumeAfterCatchException` used to resume that frame encountered the managed frames present when the thread was redirected. Now `ThrowControlForThread` installs the resume-after-catch handler, like `IL_Throw` and `HandleManagedFault`. On Windows x64 with shadow stacks, the handler's rethrow searched for a "precise" SSP on the assumption that the slot below the SSP holds a return address in the context's frame. A redirected thread's frame was interrupted rather than stopped at a call, and its recorded SSP is exact already, so the search moved it past the correct position. The new `INSTALL_RESUME_AFTER_CATCH_HANDLER_WITH_INTERRUPTED_CONTEXT` marks the context as interrupted, so that `RethrowResumeAfterCatchExceptionSkipManagedFrames` uses the recorded SSP without searching for another one. The same mismatch caused #135530, fixed by #135531. Tests: a `CancelItself` scenario in `src/tests/Regressions/coreclr/GitHub_135463` (the action cancels its own token), and `ControlledExecutionTests.CancelItselfOutsideOfTryCatchFinally` enabled again, except on browser CoreCLR. The new scenario was tested before the rebase, with this PR stacked on #135939's head (Checked builds; linux-x64, osx-arm64, windows-x64 with CET enabled). The rebase applied without conflicts and did not change the first commit's diff. | | #135939 alone | #135939 + this PR | |---|---|---| | JIT | pass | pass | | `DOTNET_InterpMode=1`, with and without `TieredCompilation=0` | linux/osx: terminate with `ResumeAfterCatchException`; windows: assert | pass | | `DOTNET_InterpMode` 2 and 3 | pass | pass | Additional validation, on the same builds: - Under the JIT, the new scenario also passes with `TieredCompilation=0`, `JITMinOpts`, `JitStress` 1 and 2, `ReadyToRun=0`, and `GCStress` 0x3 and 0xC on all three platforms. Its `TestEnclosingTry` is skipped under the JIT, as in #135939. - With this PR, all 10 `ControlledExecutionTests` pass when run as a class in one process, and `CancelItselfOutsideOfTryCatchFinally` also passes when run alone in its own process. Both runs pass on all three platforms under the JIT and `DOTNET_InterpMode` 1, 2 and 3. - With #135939 alone, `CancelItselfOutsideOfTryCatchFinally` fails under mode 1: it terminates with `ResumeAfterCatchException` on linux-x64 and osx-arm64, and fails a Checked consistency check on windows-x64. - With the handler but without the interrupted-context marking, windows-x64 asserts `*(ULONG64*)targetSSP == pContext->Rip` in `ExecuteFunctionBelowContext`. > [!NOTE] > This PR was prepared with AI assistance (Anthropic Claude, checked by OpenAI Codex) at my request. I reviewed it before posting it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #135463
Fixes #135871
This supersedes #135520, as @janvorli suggested there: the whole change in one PR, including the flip of the catch leave island.
InterpExecMethodraises a pending thread abort again at the resume point, asEECodeManager::ResumeAfterCatchdoes for compiled code. It sets upThread::m_OSContextfirst, and it dispatches the abort from a try block of its own, outside the C++ catch ofResumeAfterCatchException.INTOP_LEAVE_CATCH; every other edge, including the final edge to the leave target, usesINTOP_BR. The catch therefore ends withINTOP_LEAVE_CATCHto its island. The resume point is then the island, which is inside every try region that encloses the catch's try region. The leave target need not be inside all of those regions, which is why the abort missed the enclosing catch in [Interpreter] A thread abort raised again after a catch skips the catch of the enclosing try #135871.INTOP_THROW, so the handlers are looked up at the resume point itself rather than at the instruction before it.GitHub_135463, with seven cases. The two from [Interpreter] A thread abort raised again after a catch skips the catch of the enclosing try #135871 run only when the test is interpreted, because jitted code gets them wrong until ThreadAbortException raised again after a catch can skip a finally and reaches different handlers in tier-0 and optimized code #135637 is fixed.ControlledExecutionTests.CancelInTryAndExitCatchNormallyis enabled again, andGitHub_135530is no longer skipped under the interpreter.I validated with Checked runtimes on linux-x64, osx-arm64 and windows-x64, built from main 24543dd with and without this PR. Both columns run this PR's tests; the main column runs them on main's runtime. The
Regressionsrow is the exception: it compares each version's own runner. The results were the same on all three platforms.GitHub_135463, JITGitHub_135463,DOTNET_InterpMode1/2/3, with and withoutTieredCompilation=0ReadyForAsyncExceptionGitHub_135530, the same six interpreter configurationsControlledExecutionTests(9 tests), JIT /InterpMode1, 2, 3Regressionsmerged runner, JIT and the six interpreter configurationsJIT/interpreter,InterpMode1 and 3GitHub_135463needs both the flip and the change that marks the frame faulting: without the flip, the two #135871 cases fail; without marking the frame faulting, the abort is caught by the same handler again and again, and the test hangs.Note
This PR was prepared with AI assistance (Anthropic Claude, checked by OpenAI Codex) at my request. I reviewed it before opening it.