Skip to content

Fix Wasm async continuation stack walk - #133995

Merged
lewing merged 6 commits into
dotnet:mainfrom
lewing:lewing-fix-wasm-async-regression
Sep 16, 2026
Merged

lewing merged 6 commits into
dotnet:mainfrom
lewing:lewing-fix-wasm-async-regression

Conversation

@lewing

@lewing lewing commented Sep 15, 2026

Copy link
Copy Markdown
Member

Summary

Root cause

The Wasm FCall supplied the managed caller's R2R shadow stack pointer but used the native ResumeInterpreterContinuation entrypoint as the transition block return address. On the warm path, stack walking paired those unrelated values and failed the GetWasmVirtualIPFromStackPointer(sp) == controlPC invariant.

#133768 also enabled the full src/tests/async runner on Wasm. Many tests intentionally block on incomplete tasks and therefore require multithreading; those tests now use PlatformDetection.IsMultithreadingSupported rather than disabling the whole runner.

Validation

  • browser-Wasm Checked CoreCLR build
  • browser-Wasm interpreter runner: 142 discovered, 42 passed, 0 failed, 100 capability/issue filtered
  • browser-Wasm R2R-CG2 runner: 142 discovered, 42 passed, 0 failed, 100 capability/issue filtered
  • WASI runner: 142 discovered, 44 passed, 0 failed, 98 capability/issue filtered
  • native macOS arm64 runner: 142 passed, 0 failed
  • the diagnostic test fails on the old runtime's warm path and passes after the fix

Resolves #133931

Note

This pull request description was generated with GitHub Copilot assistance.

Use the caller shadow stack virtual IP for the interpreter continuation transition frame and exercise both cold and warm FCall paths.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 8a57fff8-6c30-4d3c-ac72-a80975122a96
@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

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

Add the active re-enable tracker beside the project-level WebAssembly exclusion.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 8a57fff8-6c30-4d3c-ac72-a80975122a96
@lewing
lewing marked this pull request as draft September 15, 2026 22:19
@lewing

lewing commented Sep 15, 2026

Copy link
Copy Markdown
Member Author

This got opened too early, fixing

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The transition-frame virtual IP should be derived lazily during stack inspection.

Review tier: Lite
Findings: None

What changed in this PR

Fixes Wasm async continuation stack walking and adds focused regression coverage.

Changes:

  • Updates Wasm continuation transition-frame metadata.
  • Adds a Wasm-specific async test runner.
  • Runs cold and warm diagnostic stack-trace scenarios.
File Description
src/​tests/​async/​diagnostic-ip-stacktrace/​diagnostic-ip-stacktrace.cs Runs the diagnostic scenario twice.
src/​tests/​async/​async.csproj Keeps the general async runner disabled on Wasm.
src/​tests/​async/​async-wasm.csproj Adds the focused Wasm runner.
src/​coreclr/​vm/​interpexec.cpp Updates the Wasm continuation transition frame return address.

Guard synchronous-wait tests with the multithreading capability, quarantine the two exact unsupported Wasm cases, and preserve all native coverage.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 8a57fff8-6c30-4d3c-ac72-a80975122a96
@lewing
lewing marked this pull request as ready for review September 15, 2026 23:20
Copilot AI review requested due to automatic review settings September 15, 2026 23:20
@lewing lewing added the arch-wasm WebAssembly architecture label Sep 15, 2026
@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

Copy link
Copy Markdown
Contributor

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Critical findings remain in async test dependency ordering and reflection test capability gating.

Get a fresh assessment by requesting another Copilot review.

Review tier: Lite
Findings: 2 High severity

Open (2)

Comment thread src/tests/async/Directory.Build.targets Outdated
Comment thread src/tests/async/reflection/reflection.cs
@lewing
lewing requested a review from jkotas September 15, 2026 23:31

@davidwrighton davidwrighton 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.

This will do for now.

Comment thread src/coreclr/vm/interpexec.cpp
Enable the full runner, gate multithreading-dependent tests by capability, and quarantine only exact unsupported Wasm scenarios.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 8a57fff8-6c30-4d3c-ac72-a80975122a96
Keep the shared TestLibrary reference on standalone async children while relying on the parent test targets for the merged runner.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 8a57fff8-6c30-4d3c-ac72-a80975122a96
Copilot AI review requested due to automatic review settings September 15, 2026 23:48
@lewing
lewing enabled auto-merge (squash) September 15, 2026 23:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate test-gating issues remain in pinvoke.cs and simple-eh.cs.

Get a fresh assessment by requesting another Copilot review.

Review tier: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (2)

Comment thread src/tests/async/pinvoke/pinvoke.cs
@lewing
lewing merged commit 76e6281 into dotnet:main Sep 16, 2026
113 of 116 checks passed
public class AwaitNotAsync
{
[Fact]
[ConditionalFact(typeof(TestLibrary.PlatformDetection), nameof(TestLibrary.PlatformDetection.IsMultithreadingSupported))]

@jakobbotsch jakobbotsch Sep 16, 2026

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.

Is this really how we want to deal with this for wasm? I do not think it is very reasonable to have to mark all async tests this way if they don't finish synchronously....

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.

Browser has single thread.

.Wait() is or should be PNSE.

The better way is to make the whole unit test method async and await instead of .Wait().

I'm not sure if some of the tests actually validate .Wait() specifically, those should stay ConditionalFact ... IsMultithreadingSupported

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.

The better way is to make the whole unit test method async and await instead of .Wait().

Yes, I agree. I think this requires work in the test generator to support Task-returning tests and to hook that up in the way WASM expects.

I'm not sure if some of the tests actually validate .Wait() specifically, those should stay ConditionalFact ... IsMultithreadingSupported

We may start and run async tasks in parallel, but I do not think any runtime async test should be validating Task.Wait() specifically. It is all about waiting for the tasks to finish.

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.

@lewing could you please ask your copilot to do that as follow up?

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.

I am working on it in #134042

@dotnet-milestone-bot dotnet-milestone-bot Bot added this to the 12.0-preview1 milestone Sep 17, 2026
jtschuster pushed a commit to jtschuster/runtime that referenced this pull request Sep 18, 2026
## Summary

- use the caller shadow stack's virtual IP as the return address for the
Wasm `ResumeInterpreterContinuation` transition frame
- enable the full runtime-async runner on Wasm while filtering only
tests that require multithreading
- use class-level capability conditions where every test in a class
requires multithreading and method-level conditions for mixed classes
- quarantine only the exact unsupported cases:
`Async2PInvoke.TestEntryPoint` under dotnet#124219 and `Async2CollectibleAlc`
under dotnet#134001
- run the diagnostic stack-trace scenario twice to cover cold and warm
FCall paths
- make reflection method ordering deterministic with
`StringComparer.Ordinal`

## Root cause

The Wasm FCall supplied the managed caller's R2R shadow stack pointer
but used the native `ResumeInterpreterContinuation` entrypoint as the
transition block return address. On the warm path, stack walking paired
those unrelated values and failed the
`GetWasmVirtualIPFromStackPointer(sp) == controlPC` invariant.

dotnet#133768 also enabled the full `src/tests/async` runner on Wasm. Many
tests intentionally block on incomplete tasks and therefore require
multithreading; those tests now use
`PlatformDetection.IsMultithreadingSupported` rather than disabling the
whole runner.

## Validation

- browser-Wasm Checked CoreCLR build
- browser-Wasm interpreter runner: 142 discovered, 42 passed, 0 failed,
100 capability/issue filtered
- browser-Wasm R2R-CG2 runner: 142 discovered, 42 passed, 0 failed, 100
capability/issue filtered
- WASI runner: 142 discovered, 44 passed, 0 failed, 98 capability/issue
filtered
- native macOS arm64 runner: 142 passed, 0 failed
- the diagnostic test fails on the old runtime's warm path and passes
after the fix

Resolves dotnet#133931

> [!NOTE]
> This pull request description was generated with GitHub Copilot
assistance.

---------

Copilot-Session: 8a57fff8-6c30-4d3c-ac72-a80975122a96
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AF: GetWasmVirtualIPFromStackPointer(sp) == controlPC in diagnostic-ip-stacktrace test

5 participants