[wasm][RyuJIT] Restore __stack_pointer in reverse P/Invoke epilogs - #134686
Merged
Merged
Conversation
Managed Wasm code keeps the shadow SP in a local and leaves the __stack_pointer global stale; inlined P/Invokes (JIT_PInvokeBegin and the SuppressGCTransition publish) lower the global to the caller's SP and leave it there. A reverse P/Invoke (UnmanagedCallersOnly) method is called with the native ABI, so it must return with __stack_pointer at its entry value. In genFnEpilog, for reverse P/Invoke methods, emit local.get <FP or SP local>; i32.const frameSize; i32.add; global.set __stack_pointer before the return, replacing the shadow stack maintenance TODO. Fixes #134681. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: eea66043-479c-4397-ae20-f4c776bc3782
|
Azure Pipelines: Successfully started running 6 pipeline(s). 10 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, @jakobbotsch |
lewing
marked this pull request as ready for review
September 25, 2026 22:07
Contributor
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
An R2R caller does a delegate* unmanaged calli into a UCO that itself does a calli into a second UCO. Each calli is an inlined P/Invoke; the inner one lowers the __stack_pointer global, and the outer one's JIT_PInvokeEnd checks that the global is back at the caller's SP. On main (without the epilog restore) this hits the sp == stack_pointer_global_value assert in vm/wasm/helpers.cpp. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: eea66043-479c-4397-ae20-f4c776bc3782
lewing
requested review from
AndyAyersMS,
davidwrighton,
jkotas and
pavelsavara
September 25, 2026 22:27
AndyAyersMS
approved these changes
Sep 25, 2026
Co-authored-by: Andy Ayers <andya@microsoft.com>
Member
Author
|
@AndyAyersMS fixed the comment, reapprove when you get a chance |
lewing
enabled auto-merge (squash)
September 25, 2026 23:37
AndyAyersMS
approved these changes
Sep 25, 2026
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.
Problem
R2R Wasm code keeps the shadow SP in a local and leaves the
__stack_pointerglobal stale.JIT_PInvokeBeginand the SuppressGCTransition publish (#130924) set__stack_pointerto the caller's shadow SP and leave it there. So when a reverse P/Invoke (UnmanagedCallersOnly) method's body, or any R2R code it calls, did an inlined P/Invoke, the method returned to its native caller with the global still lowered. That breaks the native ABI, which requires__stack_pointerto be restored on return.In the observed case the P/Invoke was CoreLib's
CastHelpers.ChkCastAny_NoCacheLookupunderGetUserData<T>. When the UCO was reached through an R2R inlined P/Invoke (adelegate* unmanagedcalli), the Debug check inJIT_PInvokeEndImplfired:sp == stack_pointer_global_valueatsrc/coreclr/vm/wasm/helpers.cpp:420. After that came GetFrame asserts and an out-of-bounds access. See #134681 for the full analysis.The bug is reachable on main whenever an R2R method calls a UCO that itself does an inlined P/Invoke (see the test below). #134606 exposes it more broadly by making
TestEntryPointinreadytorun/wasm/WasmInterpreterTransitionsrun as R2R instead of interpreted, which also routes the existingStreamLengthProxycase through R2R.Fix
In
CodeGen::genFnEpilog(codegenwasm.cpp), reverse P/Invoke methods now emit this sequence beforereturn/end:This restores the global to its entry value, the post-prolog SP plus the frame size. The sequence leaves the Wasm operand stack unchanged, so an already-pushed return value is preserved. It replaces the
TODO-WASM: shadow stack maintenancecomment. The emit pattern matches the SuppressGCTransition publish from #130924.Test
WasmInterpreterTransitionsgainsR2RCallsNestingUco: an R2R method does adelegate* unmanagedcalli intoUcoWithInlinedPInvoke, which does a calli into a second UCO,UcoLeaf. Each calli is an inlined P/Invoke. The inner one lowers__stack_pointer, and the outer one'sJIT_PInvokeEndasserts that the global is back at the caller's SP.Run against the Checked browser Core_Root from main CI build 1612746 (without #134606), with crossgen2 and the universal wasm JIT built from main:
WasmInterpreterTransitionsR2RCallsNestingUcosp == stack_pointer_global_value(helpers.cpp:420), then GetFrame assertThe test IL for this run was compiled locally with csc rather than through the repo's test build.
Validation
clrjit_universal_wasm_arm64, swapped it into crossgen2, and recompiledWasmInterpreterTransitionsagainst the CI Checked browser runtime from build 1612477, which includes [wasm][coreclr][R2R] Publish existing method bodies before interpreter fallback #134606's runtime changes.composite-r2r.wasmshowed exactly two changes: the two UCO epilogs (StreamLengthProxyandUnmanagedCallerCallsInterpreted), each with the 4-instruction restore added. Everything else was identical.readytorunwork item pass: readytorun, fieldlayout, crossgen2smoke_donotalwaysusecrossgen2, Breadth1Test, Depth1Test, DynamicMethodGCStress../build.sh clr.alljits -c checkedon osx-arm64, which includes the universal wasm JIT, builds cleanly, and jit-format reports no changes.Related
WasmInterpreterTransitionsfailure in [wasm][coreclr][R2R] Publish existing method bodies before interpreter fallback #134606. This PR is independent of [wasm][coreclr][R2R] Publish existing method bodies before interpreter fallback #134606 and has its own regression test on main, and [wasm][coreclr][R2R] Publish existing method bodies before interpreter fallback #134606 doesn't need to be stacked on it.__stack_pointerfor the native caller.Resolves #134681
Note
This PR description was generated with the help of GitHub Copilot.