[wasm] Fix reverse P/Invoke thunk key for nested [UnmanagedCallersOnly] types (CoreCLR + Mono)#130740
Merged
Merged
Conversation
…y] types
The CoreCLR wasm reverse-P/Invoke thunk table is keyed by
"{Method}#{argCount}:{Assembly}:{Namespace}:{Type}". The generator
(PInvokeCollector) used Type.Namespace, which for a nested type returns the
enclosing namespace (e.g. "System.Tests" for
System.Tests.TimeZoneInfoTests+WindowsUILanguageHelper). The runtime lookup
(src/coreclr/vm/wasm/helpers.cpp GetHashCode -> GetFullyQualifiedNameInfo)
reports an empty namespace for nested types, so the emitted key never matched
at lookup time: LookupThunk returned null, EnsureCodeForUnmanagedCallersOnly
left the portable entry point without code, and GetActualCode asserted
(precode_portable.cpp:35) on the first cold ldftn of a nested UnmanagedCallersOnly
callback.
Match the runtime by emitting an empty namespace for nested types. With this,
a nested [UnmanagedCallersOnly] callback (e.g. the EnumUILanguages callback used
by globalization/culture code) resolves its reverse thunk and the interpreter
runs it instead of asserting.
Contributes to CoreCLR-WASI library test bring-up (dotnet#130129).
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ea71f7e7-5477-4e58-bf1a-ca27d4b23d74
Add a reference to dotnet#130739 documenting that the nested-type reverse-thunk key drops the enclosing-type chain (collisions are caught as a build-time duplicate-key error). The generator is expected to be superseded once R2R generates the reverse thunks directly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ea71f7e7-5477-4e58-bf1a-ca27d4b23d74
|
Azure Pipelines: Successfully started running 3 pipeline(s). 12 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: @agocke |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the CoreCLR Wasm WasmAppBuilder reverse P/Invoke callback key generation so nested [UnmanagedCallersOnly] methods use an empty namespace (matching the runtime’s metadata-based namespace for nested types), preventing lookup misses at runtime.
Changes:
- Emit
Namespace = string.Emptyfor nested callback declaring types when constructingPInvokeCallbackkeys. - Add explanatory comments documenting why nested namespaces must be empty and the existing collision behavior.
Address PR review feedback for the nested reverse-P/Invoke thunk key fix: - Add a dedicated positive Wasm.Build.Tests case (UnmanagedCallersOnly_Nested) that cold-ldftns and invokes [UnmanagedCallersOnly] callbacks declared on nested types inside a namespace, asserting they run. This covers the empty-metadata-namespace path the fix enables on both browser and wasi (unlike the incidental, wasi-only TimeZoneInfoTests coverage). - Clarify the PInvokeCollector comment: the duplicate-key check lives in PInvokeTableGenerator.EmitNativeToInterp, not below the constructor line. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5abc3144-1f18-4321-98b4-d7b5c0627e9e
Encode the residual reverse-P/Invoke thunk key limitation as executable documentation: two nested types sharing a simple name under different enclosing types collide because the key drops the enclosing-type chain. The test asserts the desired behavior (both callbacks resolve and run) and is marked [ActiveIssue(dotnet#130739)] so it is skipped until the limitation is removed, at which point the attribute can be dropped to lock the fix. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5abc3144-1f18-4321-98b4-d7b5c0627e9e
lewing
added a commit
to lewing/runtime
that referenced
this pull request
Jul 15, 2026
… issue link Copilot PR review flagged leftover comments from the earlier prebuilt-corerun approach. CoreCLR-WASI now relinks the corerun host per-app; update the comments in WasiApp.InTree.props/.targets and sendtohelix-wasi.targets accordingly. Also fix the CI template to reference dotnet#130740 (the nested-type thunk-key fix) instead of dotnet#130739 (which tracks the remaining enclosing-type collision limitation). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ea71f7e7-5477-4e58-bf1a-ca27d4b23d74
maraf
approved these changes
Jul 15, 2026
The new UnmanagedCallersOnly_Nested test caught the same bug on Mono: the native-to-interp key mismatched for nested [UnmanagedCallersOnly] types, producing a "null function" trap. The Mono runtime computes the lookup key with mono_class_get_namespace (browser/runtime/runtime.c get_native_to_interp), which is empty for nested types, but the generator emitted the reflection (enclosing) namespace. The token+name fallback in wasm_dl_get_native_to_interp cannot recover this because bsearch compares the key first. Emit the metadata namespace (empty for nested) to match, mirroring the CoreCLR fix. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5abc3144-1f18-4321-98b4-d7b5c0627e9e
The Wasm.Build.Tests run path (RunForPublishWithWebServer + BrowserRunOptions) executes on browser-wasm only; the wasi leg is build-only in CI. Note the execution boundary in the test comment so it does not imply wasi run coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5abc3144-1f18-4321-98b4-d7b5c0627e9e
lewing
enabled auto-merge (squash)
July 15, 2026 17:32
pavelsavara
approved these changes
Jul 15, 2026
lewing
added a commit
to lewing/runtime
that referenced
this pull request
Jul 17, 2026
… issue link Copilot PR review flagged leftover comments from the earlier prebuilt-corerun approach. CoreCLR-WASI now relinks the corerun host per-app; update the comments in WasiApp.InTree.props/.targets and sendtohelix-wasi.targets accordingly. Also fix the CI template to reference dotnet#130740 (the nested-type thunk-key fix) instead of dotnet#130739 (which tracks the remaining enclosing-type collision limitation). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ea71f7e7-5477-4e58-bf1a-ca27d4b23d74
lewing
added a commit
that referenced
this pull request
Jul 21, 2026
#130745) Draft. Stands up a CoreCLR-WASI library-test leg (smoke scope: `System.Runtime.Tests`), addressing part of #130129. Builds on the CoreCLR-WASI `wasihost` corehost (#130816, merged), which ships `libWasiHost.a` in the runtime pack. This PR generates each test's own reverse P/Invoke thunks and links them into that host per-app. ## What this does Adds a per-app native link of the shipping `wasihost` corehost (`libWasiHost.a`) so a test's own `[UnmanagedCallersOnly]` reverse P/Invoke thunks are covered — mirroring the browser-CoreCLR relink. The baked callhelpers table (`libcoreclr_gen_static.a`) only covers framework top-level UCO callbacks; library/test callbacks (e.g. a nested `WindowsUILanguageHelper.EnumUiLanguagesCallback`) are unknown to it and trap at first use without the per-app link. - **Host**: consumes `libWasiHost.a` from the `wasihost` corehost (#130816) — the leg does not manufacture its own host. - **Per-app link** (`src/mono/wasi/build/WasiApp.CoreCLR.targets`): `ManagedToNativeGenerator` (TargetOS=wasi) → compile the generated callhelpers with the wasi-sdk clang → link `libWasiHost.a` from the runtime-pack static archives + the app callhelper `.o` (replacing `libcoreclr_gen_static.a`) via `wasm-component-ld`. The `wasi:http` import that `System.Net.*` pulls in is declared with `-Wl,--component-type` (`WasiHttpWorld_component_type.wit`, as Mono does); wasmtime is given `-S http` by the test targets. - **Generator** (`ManagedToNativeGenerator.cs` / `PInvokeTableGenerator.cs`): a `WarnOnUnresolvedPInvokeModules` flag downgrades `WASM0066` to a message for untrimmed library-test closures (whose foreign-platform P/Invokes are unresolved on wasm and never called), so they don't fail the build under warn-as-error. - **Test/CI wiring**: `tests.wasi.targets`, `sendtohelix-wasi.targets`, `tests.proj` smoke set, and a `wasi-wasm-coreclr-library-tests` CI template (non-gating, rolling). The leg runs with full ICU globalization (non-invariant): the `wasihost` corehost preloads `icudt.dat` from `CORE_ROOT`. ## Validation Locally (macOS arm64, wasi-sdk + wasmtime): the baseline (`clr+libs+host+packs -os wasi -c Release`) builds green and stages `libWasiHost.a` from the `wasihost` corehost into the runtime pack. The `System.Runtime.Tests` bundle links the host per-app and discovers + runs the full suite: ``` Discovered: managed/System.Runtime.Tests.dll (found 9966 of 9966 test cases) Starting: managed/System.Runtime.Tests.dll ``` Managed exception dispatch through the interpreter EH + reverse UCO thunks works with no precode assert. A `System.Tests.VersionTests` subset ran 251/251 passing (`WASM EXIT 0`). ## Relationship to other work - **#130816** (merged) — the `wasihost` corehost this leg links. - **#130740** (merged) — the nested-UCO thunk-key generator fix the interpreter needs to resolve nested reverse thunks. - **#130634** stays scoped to the R2R `call_indirect` path (not this pure-interpreter leg). ## Tracked follow-ups (not in this PR) - **#130742** — build-switchable `wasi:http` capability (no-http vs http-capable host), and the longer-term "statify the app-declared imports" direction. - **#130739** — token-based reverse-thunk key robustness. - **#128362** — relocate `coreclr_compat.h` to a neutral shared location + stage the vm/minipal headers and the wit for out-of-tree/Helix runs (the per-app link currently uses in-repo fallbacks in-tree). - Polish: `-Wl,-u,__main_void` in place of `--whole-archive`, and incremental `Inputs`/`Outputs` on the link target. > [!NOTE] > This pull request was authored with GitHub Copilot. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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.
Why
On wasm, the first cold
ldftnof a nested[UnmanagedCallersOnly]callback failed to resolve its native-to-interp / reverse-thunk entry:precode_portable.cpp:35.MONO_WASM: null function.Both native-to-interp tables are keyed by
"{Method}#{argCount}:{Assembly}:{Namespace}:{Type}", and on both runtimes the two sides of that key disagreed for nested types:PInvokeCollector, one per runtime) used reflection'sType.Namespace, which for a nested type returns the enclosing namespace.helpers.cppGetHashCode->GetFullyQualifiedNameInfobrowser/runtime/runtime.cget_native_to_interp->mono_class_get_namespaceBecause the keys never matched, the lookup returned null and the callback had no code.
Note the Mono token+name fallback in
wasm_dl_get_native_to_interpdoes not recover this:bsearchcompares the key first, so a key mismatch defeats both the (key, token) and key-only lookups. The token only disambiguates within a matching key (the trimmed case).Fix
Emit the metadata namespace (empty for nested types) in both generators so the generated key matches what each runtime computes at lookup time:
Tests
UnmanagedCallersOnly_Nested(newWasm.Build.Testscase +UnmanagedCallbackNested.csasset): cold-ldftns and invokes[UnmanagedCallersOnly]callbacks on nested (and doubly-nested) types inside a namespace, asserting they run. It executes on browser-wasm for both the Mono and CoreCLR generators (the wasi WBT leg is build-only in CI). This test is what surfaced the Mono half of the bug (no prior test exercised nested UCO). The only pre-existing execution coverage was incidental and wasi/CoreCLR-only (a nested UCO helper inTimeZoneInfoTests, compiled out on browser).UnmanagedCallersOnly_NestedConflict(new,[ActiveIssue(#130739)], skipped): documents the residual limitation where two nested types sharing a simple name under different enclosing types collide because the key drops the enclosing-type chain. It encodes the desired behavior so the attribute can be removed once [wasm] reverse P/Invoke thunk key drops enclosing type for nested [UnmanagedCallersOnly] methods #130739 is fixed.Notes
The key still drops the enclosing-type chain, so nested types sharing a simple name in different enclosing types collide. Tracked by #130739 (which also records the proven Mono remedy: key+token lookup with a name-only fallback, per #107113). Related: #130129.
Files changed
src/tasks/WasmAppBuilder/coreclr/PInvokeCollector.csandsrc/tasks/WasmAppBuilder/mono/PInvokeCollector.cs(the fix, one per runtime)src/mono/wasm/Wasm.Build.Tests/PInvokeTableGeneratorTests.csand twosrc/mono/wasm/testassets/EntryPoints/PInvoke/*.csassets (tests)Note
This PR description was generated with the assistance of GitHub Copilot.