Skip to content

Do not adjust wasm virtual IPs in the thread abort stack walk - #135915

Merged
jkotas merged 3 commits into
dotnet:mainfrom
pcshrosbree:fix/wasm-ta-virtual-ip
Oct 19, 2026
Merged

jkotas merged 3 commits into
dotnet:mainfrom
pcshrosbree:fix/wasm-ta-virtual-ip

Conversation

@pcshrosbree

Copy link
Copy Markdown
Contributor

Thread::ReadyForAsyncException refuses to raise a thread abort inside a finally or catch handler. Its walk, TAStackCrawlCallBackWorker, moves the offset of every non-active frame back by one before comparing it with the EH clauses, because the frame's control PC is a return address.

On wasm, the control PC of a ReadyToRun frame is a virtual IP instead. fgWasmVirtualIP gives each region of a method a number and stores the current region's number in the frame before each call. A new region starts at every try, handler and filter boundary, so every call in the first region of a handler reports exactly the handler's start. Moved back by one, that is the previous region, outside the handler. With the default ReadyToRun CoreLib, ControlledExecution.Run then gets the abort raised again inside its own finally and catch (ThreadAbortException) handlers, at their first call through an import cell, and throws ThreadAbortException instead of OperationCanceledException.

SfiInit and SfiNext already skip the adjustment for virtual IPs. This change does the same in TAStackCrawlCallBackWorker. A virtual IP never needs the adjustment: it names the region of the call in progress, and EH boundaries are region boundaries. For this walk, which checks only handler ranges, the adjustment changes the answer only at a handler's first region, where it is wrong; crossing a try boundary does not change handler membership. Active frames were never adjusted. Interpreter frames are not virtual IPs and keep the adjustment.

The second commit lets the ControlledExecutionTests that need no second thread run on the single-threaded wasm legs. The whole class was conditioned on IsMultithreadingSupported, so none of it ran there. The condition is now on the four tests that cancel from Task.Run. CancelBeforeRun covers this fix. #135520 re-enables CancelInTryAndExitCatchNormally, which also uses Task.Run; whichever of the two lands second needs to add the condition to it.

Measured with browser-wasm Checked builds under node, single-threaded, default ReadyToRun CoreLib, using a program with the same shapes as the tests:

case main 3994df1ceee1 this PR
CancelBeforeRun (token cancelled before Run) the interpreter's #135463 assert in the caller's catch, before it reports the exception type OperationCanceledException
action with no handler, thread aborts itself through Canceler.Cancel ThreadAbortException OperationCanceledException
action with only try/finally, same ThreadAbortException, finally ran OperationCanceledException, finally ran
CancelItselfFromFinally passes passes
any of these with DOTNET_ReadyToRun=0 unchanged unchanged

On linux-x64, osx-arm64 and windows-x64 (Checked), the runtime's code differs from main only in line-number constants after the change. On linux-x64, restoring the original line numbers makes the code identical to main. On osx-arm64 and windows-x64, it is identical to a control with the wasm-only lines removed and their line positions retained. ControlledExecutionTests passes 8/8 on all three with the test change. I did not run the library tests on wasm locally or test a Release runtime.

This fixes the first problem in #135914. Token cancellation from inside Run still reaches the second problem: after CTS's ReadyToRun catch, the end-of-catch abort check asserts on wasm in Checked builds. I'm asking there how you'd like it done.

Partially addresses #135914 (the second problem stays open there).

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.

On wasm, the control PC of a ReadyToRun frame is a virtual IP: the JIT
numbers the regions of a method (fgWasmVirtualIP) and stores the number
of the current region in the frame before each call. It is not a return
address. TAStackCrawlCallBackWorker moves the offset of every non-active
frame back by one before comparing it with the EH clauses, so a call in
the first region of a finally or catch handler was treated as being just
before the handler. Thread::ReadyForAsyncException then let a thread
abort be raised inside the handler.

Skip the adjustment for virtual IPs, as SfiInit and SfiNext already do.
ControlledExecutionTests was conditioned on IsMultithreadingSupported
as a whole, so none of its tests ran on the single-threaded wasm legs.
Only the tests that cancel from Task.Run need a second thread; move the
condition to them. CancelBeforeRun then covers the wasm ReadyToRun case
in which the abort was raised again inside ControlledExecution.Run's
catch and finally.
@azure-pipelines

Copy link
Copy Markdown
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 dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Oct 16, 2026
@jkotas jkotas added the arch-wasm WebAssembly architecture label Oct 16, 2026
@jkotas
jkotas requested a review from davidwrighton October 16, 2026 04:20
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara
See info in area-owners.md if you want to be subscribed.

@lewing

lewing commented Oct 16, 2026

Copy link
Copy Markdown
Member

/azp run runtime-coreclr outer loop

@azure-pipelines

Copy link
Copy Markdown
No pipelines are associated with this pull request.

@lewing

lewing commented Oct 17, 2026

Copy link
Copy Markdown
Member

thanks for tracking down the misclassification here

@pcshrosbree

Copy link
Copy Markdown
Contributor Author

@jkotas lewing approved this, but Build Analysis is still red from the run on October 16. The check summary shows these unmatched failures:

Would you prefer a rerun, or /ba-g?

@pcshrosbree

Copy link
Copy Markdown
Contributor Author

Thank you for the review and the merge.

pcshrosbree added a commit to pcshrosbree/runtime that referenced this pull request Oct 19, 2026
The previous commit enables this test again. Since dotnet#135915 removed the
class-level IsMultithreadingSupported condition from
ControlledExecutionTests, the test now also runs on single-threaded
browser-wasm CoreCLR. There, with the default ReadyToRun CoreLib, an
action that cancels itself still leaves the abort pending at the end of
CancellationTokenSource's ReadyToRun catch (dotnet#135914). Checked runtimes
assert there. Skip the test on browser CoreCLR until dotnet#135914 is fixed.
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.
@dotnet-milestone-bot dotnet-milestone-bot Bot added this to the 12.0-preview1 milestone Oct 20, 2026
pcshrosbree added a commit to pcshrosbree/runtime that referenced this pull request Oct 29, 2026
The previous commit enables this test again. Since dotnet#135915 removed the
class-level IsMultithreadingSupported condition from
ControlledExecutionTests, the test now also runs on single-threaded
browser-wasm CoreCLR. There, with the default ReadyToRun CoreLib, an
action that cancels itself still leaves the abort pending at the end of
CancellationTokenSource's ReadyToRun catch (dotnet#135914). Checked runtimes
assert there. Skip the test on browser CoreCLR until dotnet#135914 is fixed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arch-wasm WebAssembly architecture area-VM-coreclr community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants