[wasm][coreclr] Dispatch R2R-compiled UnmanagedCallersOnly callbacks to their native entrypoint - #134355
[wasm][coreclr] Dispatch R2R-compiled UnmanagedCallersOnly callbacks to their native entrypoint#134355pavelsavara wants to merge 15 commits into
Conversation
…to their native entrypoint A partial R2R image can crossgen an [UnmanagedCallersOnly] callback to native code. The generated native->interpreter reverse thunk routed it unconditionally through ExecuteInterpretedMethodFromUnmanaged, which either handed a null interpreter body to the interpreter (fatal 'Unimplemented or invalid interpreter opcode') or, via InvokeManagedMethod, hit the interp->R2R 'null function or function signature mismatch' trap (dotnet#134200). Repro: the browser event loop calls the SystemJS_ExecuteBackgroundJobCallback export -> ThreadPool.BackgroundJobHandler, which is R2R-compiled under a partial image. Fix: the call-helpers generator now emits a direct-R2R dispatch in each reverse thunk - resolve the R2R native entrypoint via GetR2RNativeCodeForUnmanagedCallersOnly and call it with the native ABI, falling back to the interpreter only when there is no R2R body. GetUnmanagedCallersOnlyThunk is refactored onto the same shared helper. Regenerated the checked-in browser and wasi call helpers.
|
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. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @BrzVlad, @janvorli |
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
|
you'll want to remove runtime/src/libraries/tests.proj Lines 91 to 93 in 3cd7514 |
That's still failing after this PR, it's slightly different. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Address the lazy-R2R retry issue and add generator-level coverage for direct dispatch generation.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Fixes WebAssembly reverse thunks so R2R-compiled [UnmanagedCallersOnly] callbacks dispatch directly to native entrypoints, with interpreter fallback.
Changes:
- Adds shared R2R entrypoint resolution.
- Updates callback generation and regenerates WASI helpers.
- Preserves interpreter fallback for non-R2R callbacks.
| File | Summary |
|---|---|
src/coreclr/vm/wasm/wasi/callhelpers-reverse.cpp |
Regenerated WASI reverse callback helpers. |
src/coreclr/vm/wasm/helpers.cpp |
Adds shared R2R callback resolution. Moderate concern (1 vote): lazy R2R may not be retried after interpreter preparation. |
src/coreclr/tools/aot/ILCompiler.ReadyToRun/PortableCallHelpers/PInvokeTableGenerator.cs |
Emits direct R2R dispatch. Nit (3 votes): add generator coverage for emitted R2R and fallback branches. |
Addresses PR review: cache GetR2RNativeCodeForUnmanagedCallersOnly result in a per-callback static (like MD_*) so it runs at most once, and move the blank line to separate the R2R fast path from the interpreter fallback.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical ABI and concurrency findings, plus missing regression coverage, remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
Resolved since last review (1)
…sh cache Addresses PR review: skip the direct R2R call for callbacks whose return is passed by a hidden buffer (their wasm function type would not match the declared cast), and publish the cached entrypoint with VolatileLoad/VolatileStore so concurrent first calls cannot tear the pointer.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Direct dispatch can mishandle multi-slot Wasm signatures, and null R2R results may be cached permanently.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Resolved since last review (2)
Reject UnmanagedCallersOnly callbacks whose aggregate return requires a hidden return buffer instead of emitting an ABI-incompatible native wrapper, and cover the diagnostic in generator tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical ABI validation is missing for v128 callback signatures, risking invalid calls or corrupted data.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Resolved since last review (1)
Reject Vector128 UnmanagedCallersOnly parameters and returns before generating native wrappers whose C declarations cannot represent the v128 ABI, and cover both signature positions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Generate acquire/release builtins directly so both VM-compiled and app-linked reverse helpers publish cached R2R entrypoints atomically. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
User NativeFileReference .c/.cpp now use a dedicated corerun-compile-user.rsp that carries only the shared compile flags (optimization, exception model, SIMD, GEN_PINVOKE) and omits the coreclr_compat.h force-include and kit header path, mirroring the app build's _EmccCFlags/_EmccCFlagsGenerated split so raw sources don't inherit CoreCLR typedefs/macros. Also correct a stale generator comment that still referenced a volatile store. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The generated wasm reverse thunk cannot model hidden-return-buffer struct-returning UnmanagedCallersOnly callbacks (GetSize -> SizeF), so crossgen2 now rejects them and fails the whole test build leg. Opt these four callconv tests out of the test-specific corerun on browser-coreclr, matching the existing EmptyThisCallTest precedent (dotnet#131811). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…134686) ## Problem R2R Wasm code keeps the shadow SP in a local and leaves the `__stack_pointer` global stale. `JIT_PInvokeBegin` and the SuppressGCTransition publish (#130924) set `__stack_pointer` to 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_pointer` to be restored on return. In the observed case the P/Invoke was CoreLib's `CastHelpers.ChkCastAny_NoCacheLookup` under `GetUserData<T>`. When the UCO was reached through an R2R inlined P/Invoke (a `delegate* unmanaged` calli), the Debug check in `JIT_PInvokeEndImpl` fired: `sp == stack_pointer_global_value` at `src/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 `TestEntryPoint` in `readytorun/wasm/WasmInterpreterTransitions` run as R2R instead of interpreted, which also routes the existing `StreamLengthProxy` case through R2R. ## Fix In `CodeGen::genFnEpilog` (`codegenwasm.cpp`), reverse P/Invoke methods now emit this sequence before `return`/`end`: ``` local.get <FP local, or SP local if there is no frame pointer> i32.const genTotalFrameSize() i32.add global.set __stack_pointer ``` 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 maintenance` comment. The emit pattern matches the SuppressGCTransition publish from #130924. ## Test `WasmInterpreterTransitions` gains `R2RCallsNestingUco`: an R2R method does a `delegate* unmanaged` calli into `UcoWithInlinedPInvoke`, 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's `JIT_PInvokeEnd` asserts 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: | test | main JIT | fixed JIT | |---|---|---| | main `WasmInterpreterTransitions` | pass | pass | | with `R2RCallsNestingUco` | **assert** `sp == stack_pointer_global_value` (helpers.cpp:420), then GetFrame assert | pass (exit 100) | The test IL for this run was compiled locally with csc rather than through the repo's test build. ## Validation - Rebuilt `clrjit_universal_wasm_arm64`, swapped it into crossgen2, and recompiled `WasmInterpreterTransitions` against the CI Checked browser runtime from build 1612477, which includes #134606's runtime changes. - Old JIT: reproduces the CI failure (the helpers.cpp:420 assert, then GetFrame asserts and OOB). - New JIT: passes with exit 100 and no asserts. - Before the new test was added, a disassembly diff of `composite-r2r.wasm` showed exactly two changes: the two UCO epilogs (`StreamLengthProxy` and `UnmanagedCallerCallsInterpreted`), each with the 4-instruction restore added. Everything else was identical. - The other 6 tests in the Helix `readytorun` work item pass: readytorun, fieldlayout, crossgen2smoke_donotalwaysusecrossgen2, Breadth1Test, Depth1Test, DynamicMethodGCStress. - `./build.sh clr.alljits -c checked` on osx-arm64, which includes the universal wasm JIT, builds cleanly, and jit-format reports no changes. ## Related - Unblocks the `WasmInterpreterTransitions` failure in #134606. This PR is independent of #134606 and has its own regression test on main, and #134606 doesn't need to be stacked on it. - Helps the open #134355, where native callers dispatch directly to R2R UCO bodies. There the UCO epilog is the only place that can restore `__stack_pointer` for the native caller. Resolves #134681 > [!NOTE] > This PR description was generated with the help of GitHub Copilot. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Andy Ayers <andya@microsoft.com> Copilot-Session: eea66043-479c-4397-ae20-f4c776bc3782
radekdoulik
left a comment
There was a problem hiding this comment.
The MD assignment is pre-existing and can be done as followup.
| // Cache the resolved entrypoint in a per-callback static, published with acquire/release | ||
| // atomics: these are native entry points that can be entered concurrently, and the value is | ||
| // computed identically on every call, so the racing read/write is benign but must not tear. | ||
| string r2rStaticDecl = $"{w.NewLine}static void* {r2rVar} = (void*)(intptr_t)-1;"; |
There was a problem hiding this comment.
could the new r2r section stuff not be inline below where it is much more readable?



Problem
Under a partial ReadyToRun image, an
[UnmanagedCallersOnly]callback can be crossgen'd to native (R2R) code. The generated native-to-interpreter reverse thunk (callhelpers-reverse.cpp) routed it unconditionally throughExecuteInterpretedMethodFromUnmanaged, which:Fatal error. Unimplemented or invalid interpreter opcode(INTOP_INVALID), orInvokeManagedMethod, hit the interp-to-R2RRuntimeError: null function or function signature mismatchtrap ([wasm][R2R] Interpreter→R2R call to a shared-generic method with no R2R body traps with "null function or function signature mismatch" at startup #134200).The design already intends R2R-compiled
[UnmanagedCallersOnly]methods to be dispatched directly to their native entrypoint (GetUnmanagedCallersOnlyThunkdoes this), but generated export reverse thunks bypassed that path. The browser event loop calls native C exports directly, for exampleSystemJS_ExecuteBackgroundJobCallbacktoSystem.Threading.ThreadPool.BackgroundJobHandler, which can be R2R-compiled under a partial image.Fix
The call-helpers generator now emits a direct-R2R dispatch in each supported reverse thunk: resolve and cache the R2R native entrypoint with
GetR2RNativeCodeForUnmanagedCallersOnly, call it with the native ABI, and fall back to the interpreter path only when there is no R2R body.Additional hardening from review:
Int128parameters or returns are explicitly rejected before generating a wrapper whose C function type would not match the lowered WASM signature.Vector128<T>parameters and returns using thev128ABI are explicitly rejected because the generated C wrapper does not yet model that value type.HasNativeEntryPointexcludes interpreter-preferred entrypoints, so the generated interpreter fallback thunk cannot be returned recursively as R2R code.GetUnmanagedCallersOnlyThunkis refactored onto the same shared lookup helper. The checked-in browser and WASI call helpers are regenerated.End-to-end coverage
WasmInterpreterTransitionsnow exercises:The native round trips cover:
int32_t -> int32_tint64_t, float, double -> doubleint32_t*, int32_t -> voidint32_tstruct return, scalarized toi32by the WASM C ABIRuntime-test support now compiles raw
.c/.cppNativeFileReferenceitems, includes their module names in generated P/Invoke tables, and links their objects into the test-specific browser corerun.Validation
ExecuteInterpretedMethodFromUnmanagedwithtargetIp != NULL; restoring it passes.clr+libs+hostbuild: zero warnings and errors.ILCompiler.ReadyToRun.TestsWasmArgumentLayoutTests: 72 passed, 0 failed.WasmInterpreterTransitionsbuild: zero warnings and errors.WasmInterpreterTransitions: expected 100, actual 100.Main files
src/coreclr/tools/aot/ILCompiler.ReadyToRun/PortableCallHelpers/PInvokeTableGenerator.cs- emits direct R2R dispatch and rejects unsupported multi-slot callbackssrc/coreclr/vm/wasm/helpers.cpp- shared R2R UCO entrypoint lookupsrc/mono/browser/build/coreclr_compat.h- app-link compatibility declarations for generated helperssrc/coreclr/vm/wasm/browser/callhelpers-reverse.cpp,src/coreclr/vm/wasm/wasi/callhelpers-reverse.cpp- regenerated helperssrc/tests/Common/CLRTest.WasmCorerun.targets- native source support for Browser/CoreCLR runtime testssrc/tests/readytorun/wasm/WasmInterpreterTransitions/- managed and native end-to-end coverageRelated
Addresses the native-export reverse-thunk manifestation of #134200 by keeping R2R-compiled
[UnmanagedCallersOnly]callbacks off the interp-to-R2R path entirely. Part of the browser-wasm CoreCLR ReadyToRun work (#134337).Note
This PR description was updated with GitHub Copilot assistance and reviewed by the author.