[wasm] Split CoreCLR thunks by target OS (browser vs wasi)#129549
Open
lewing wants to merge 4 commits into
Open
[wasm] Split CoreCLR thunks by target OS (browser vs wasi)#129549lewing wants to merge 4 commits into
lewing wants to merge 4 commits into
Conversation
Browser and WASI CoreLib expose different sets of P/Invokes and UCO
callbacks:
* WASI CoreLib pulls in the Unix Interop.Libraries.cs path, so it
references Sys (sockets, fcntl, host entry, etc.) symbols that
browser CoreLib does not.
* Browser CoreLib references JS-interop pinvokes/callbacks
(SystemJS_*, SystemInteropJS_*, JavaScriptExports.*) that WASI
does not.
A single generated callhelpers-{pinvoke,reverse,interp-to-managed}.cpp
cannot be correct for both. Split the checked-in baseline copies into
src/coreclr/vm/wasm/{browser,wasi}/ and pick the appropriate subdir
in vm/CMakeLists.txt based on CLR_CMAKE_TARGET_WASI.
Also parameterize the WasmAppBuilder.CoreCLR generator with a TargetOS
property (default "browser" to preserve existing behavior). The
generator now filters DllImport and UnmanagedCallersOnly methods by
the target's [SupportedOSPlatform] / [UnsupportedOSPlatform] attributes
so a future regen against a wasi build naturally drops browser-only
entries and vice-versa. BrowserWasmApp.CoreCLR.targets explicitly
passes TargetOS="browser".
Contributor
|
Tagging subscribers to this area: @agocke |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR separates WebAssembly CoreCLR “callhelpers” thunk sources for browser vs WASI so the runtime can compile against the correct P/Invoke + reverse-P/Invoke surface for each target, and wires the VM build to pick the appropriate thunk set based on CLR_CMAKE_TARGET_WASI. It also extends the WasmAppBuilder CoreCLR table generator pipeline with a TargetOS input intended to filter platform-specific entries.
Changes:
- Split checked-in WASM thunk baselines into
src/coreclr/vm/wasm/{browser,wasi}/and select the subdir fromsrc/coreclr/vm/CMakeLists.txt. - Add a
TargetOSMSBuild task property and propagate it into the CoreCLR P/Invoke collection pipeline. - Update browser CoreCLR app build targets to pass
TargetOS="browser"explicitly.
Reviewed changes
Copilot reviewed 8 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tasks/WasmAppBuilder/coreclr/PInvokeTableGenerator.cs | Passes targetOS through to the collector used when scanning assemblies. |
| src/tasks/WasmAppBuilder/coreclr/PInvokeCollector.cs | Adds TargetOS-aware filtering logic for platform attributes (type-level). |
| src/tasks/WasmAppBuilder/coreclr/ManagedToNativeGenerator.cs | Introduces a TargetOS property and forwards it into the generator pipeline. |
| src/mono/browser/build/BrowserWasmApp.CoreCLR.targets | Sets TargetOS="browser" when invoking ManagedToNativeGenerator. |
| src/coreclr/vm/wasm/wasi/callhelpers-pinvoke.cpp | WASI-specific checked-in baseline P/Invoke override table. |
| src/coreclr/vm/wasm/wasi/callhelpers-interp-to-managed.cpp | WASI-specific checked-in baseline interp-to-managed thunk table. |
| src/coreclr/vm/wasm/browser/callhelpers-reverse.cpp | Browser reverse-thunk baseline updated (includes a manual addition). |
| src/coreclr/vm/wasm/browser/callhelpers-pinvoke.cpp | Browser-specific checked-in baseline P/Invoke override table. |
| src/coreclr/vm/wasm/browser/callhelpers-interp-to-managed.cpp | Browser-specific checked-in baseline interp-to-managed thunk table. |
| src/coreclr/vm/CMakeLists.txt | Selects browser vs WASI thunk sources via WASM_THUNK_SUBDIR. |
Comments suppressed due to low confidence (2)
src/coreclr/vm/wasm/browser/callhelpers-reverse.cpp:1189
- This manual reverse-thunk addition references
System.Threading.WasiFinalizerScheduler.ScheduleFinalization, but there is no such managed type/method anywhere in this repo. At runtimeLookupUnmanagedCallersOnlyMethodByNamethrows if the type can’t be found (TypeName::GetTypeFromAsmQualifiedName(..., TRUE)), so this would hard-fail. Unless the managed implementation is added in this PR, these manual entries need to be removed (and ideally generated instead of hand-maintained).
src/coreclr/vm/wasm/browser/callhelpers-reverse.cpp:1282 g_ReverseThunksalso contains a manual entry forWasiFinalizerScheduler.ScheduleFinalization. If the managed type/method isn’t added, this mapping should be removed as it can trigger the same hard failure when resolved.
Comment on lines
231
to
235
| foreach (CustomAttributeData cattr in CustomAttributeData.GetCustomAttributes(type)) | ||
| { | ||
| try | ||
| { | ||
| if (cattr.AttributeType.FullName == "System.Runtime.Versioning.UnsupportedOSPlatformAttribute" && |
Member
Author
There was a problem hiding this comment.
Addressed in e05eefd — IsUnsupportedOnPlatform now walks method-level, then up through declaring types, then falls back to the assembly. Attribute evaluation is factored into a shared helper (EvaluatePlatformAttributes returning Supported/Unsupported/Unknown) so each scope can short-circuit cleanly, and assembly results are cached separately from the type cache. Assemblies like System.Runtime.InteropServices.JavaScript that carry [assembly: SupportedOSPlatform("browser")] are now correctly excluded from a TargetOS="wasi" regen.
Note
AI-generated content by GitHub Copilot CLI.
The 'MANUAL ADDITION' block in src/coreclr/vm/wasm/browser/callhelpers-reverse.cpp
that wired up Call_/MD_WasiFinalizerScheduler_ScheduleFinalization was a
leftover from when browser and WASI shared a single checked-in copy of
the generated thunks. It is not produced (and not needed) by a fresh
browser-targeted regen because:
* WasiFinalizerScheduler.cs lives inside an
<ItemGroup Condition="'$(TargetsWasi)' == 'true'"> in
System.Private.CoreLib.Shared.projitems, so it is only compiled
into the WASI CoreLib.
* Browser CoreCLR uses the JS-host-provided
SystemJS_ScheduleFinalization import (setTimeout(callback, 0));
no managed callback registration is involved on browser.
* Browser CoreLib does not contain a [UnmanagedCallersOnly]
WasiFinalizerScheduler.ScheduleFinalization method, so the
reverse thunk has no managed target on browser.
After the thunk split (1d75035) the WASI side now regenerates this
entry naturally (visible in wasi/callhelpers-reverse.cpp with no
'MANUAL' marker). Remove the dead block + map entry from the browser
copy. Verified browser and WASI corerun both still build.
…rm attrs
Two review fixes for the CoreCLR WasmAppBuilder generator changes:
* ManagedToNativeGenerator now validates and normalizes the TargetOS
property. Empty/whitespace and unknown values fail the task with an
actionable error instead of silently producing empty filtered tables;
the value is lower-cased so casing differences in the MSBuild input
don't cause every IsSupportedOSPlatform check to miss.
* PInvokeCollector.IsUnsupportedOnPlatform now considers method-level
and assembly-level [SupportedOSPlatform] / [UnsupportedOSPlatform]
attributes in addition to the type's own attributes (walking up
through declaring types first, then falling back to the assembly).
Assemblies like System.Runtime.InteropServices.JavaScript that carry
[assembly: SupportedOSPlatform("browser")] are now correctly excluded
from a TargetOS="wasi" regen and vice versa. Assembly-level results
are cached separately from the type cache.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
@copilot review Pushed e05eefd in response to this review:
The two suppressed/low-confidence comments about the Note AI-generated content by GitHub Copilot CLI. |
maraf
approved these changes
Jun 18, 2026
Per Marek's review feedback. With the TargetOS validation now in ManagedToNativeGenerator.Execute(), the default "browser" on the PInvokeCollector and PInvokeTableGenerator constructors is dead weight that would also hide future misuse. Make it a required parameter everywhere; the only existing call site (ManagedToNativeGenerator.ExecuteInternal) already passes TargetOS explicitly so no behaviour changes. PInvokeTableGenerator's isLibraryMode optional parameter was reordered to come before the now-required targetOS (C# disallows a required parameter after an optional one). Reverting to required-required order matches the only caller and keeps the API symmetric with PInvokeCollector. Co-authored-by: Copilot <223556219+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.
Browser and WASI CoreLib expose different sets of P/Invokes and UCO callbacks:
A single generated callhelpers-{pinvoke,reverse,interp-to-managed}.cpp cannot be correct for both. Split the checked-in baseline copies into src/coreclr/vm/wasm/{browser,wasi}/ and pick the appropriate subdir in vm/CMakeLists.txt based on CLR_CMAKE_TARGET_WASI.
Also parameterize the WasmAppBuilder.CoreCLR generator with a TargetOS property (default "browser" to preserve existing behavior). The generator now filters DllImport and UnmanagedCallersOnly methods by the target's [SupportedOSPlatform] / [UnsupportedOSPlatform] attributes so a future regen against a wasi build naturally drops browser-only entries and vice-versa. BrowserWasmApp.CoreCLR.targets explicitly passes TargetOS="browser".