Skip to content

[wasm] Compute CoreCLR P/Invoke struct sizes with crossgen2's type system - #131877

Merged
radekdoulik merged 87 commits into
dotnet:mainfrom
radekdoulik:radekdoulik-wasm-struct-sizes-from-crossgen2
Sep 4, 2026
Merged

[wasm] Compute CoreCLR P/Invoke struct sizes with crossgen2's type system#131877
radekdoulik merged 87 commits into
dotnet:mainfrom
radekdoulik:radekdoulik-wasm-struct-sizes-from-crossgen2

Conversation

@radekdoulik

@radekdoulik radekdoulik commented Aug 5, 2026

Copy link
Copy Markdown
Member

Replaces the hardcoded struct-size table in the CoreCLR wasm P/Invoke generator with crossgen2's real field-layout engine.

The problem

ManagedToNativeGenerator computed wasm ABI signature strings from System.Reflection.MetadataLoadContext, which has no field-layout engine. Struct sizes came from a 7-entry hardcoded table, and anything outside it was a hard build error:

error WASM0067: SignatureMapper: unknown multi-field struct 'X' (fields: N)
- add its size to s_knownStructSizes in SignatureMapper.cs

Size matters because the CoreCLR interpreter lays struct arguments out inline across 8-byte slots — TokenToSlotCount returns max((size + 7) / 8, 1) for an S<N> token. A wrong N misaligns the interpreter frame.

Mono's generator needs none of this: its alphabet has no S, and it encodes every struct as a pointer.

The change

crossgen2 gains --generate-portable-callhelpers <dir>, which writes the three C++ call-helper files directly. It sets up its type system as for a real wasm compilation, scans the input assemblies and emits — no JIT, no R2R image. The option requires --targetarch wasm with --targetos browser|wasi.

The CoreCLR half of the MSBuild task is deleted rather than adapted: ManagedToNativeGenerator, PInvokeCollector, PInvokeTableGenerator, SignatureMapper, InternalCallSignatureCollector, InterpToNativeGenerator. _CoreCLRGenerateManagedToNative keeps its name and position in the target graph; its final step changes from <UsingTask> to <Exec>. The regeneration scripts move next to their output under src/coreclr/vm/wasm/ and drive generate-coreclr-helpers.proj. Mono's generator is untouched.

−2269 lines under src/tasks, +1541 under ILCompiler.ReadyToRun/PortableCallHelpers. A move, not an addition: the second implementation of wasm ABI lowering is gone, and the one that remains is the one the JIT interface itself calls. Sizes are computed, not enumerated. The only change to WasmLowering is widening WasmValueTypeToSigChar from private to internal.

Naming

Portable entry points exist for any platform that cannot generate code at run time; wasm is the only one today. Per review feedback nothing in this functionality is named after wasm. Symbols shared by the runtime and the generated tables were renamed on both sides at once:

before after
StringToWasmSigThunk StringToPortableSigThunk
g_wasmThunks[Count] g_portableCallHelperThunks[Count]
wasm_ret_S<n> portable_callhelper_ret_S<n>

What keeps wasm in its name is what is genuinely about wasm: the ABI in WasmLowering, the --targetos browser|wasi requirement, and the wasm-specific corerun the runtime tests link.

Finding crossgen2

Three paths, tried in order:

  • Override$(PortableCallHelpersGeneratorPath), which must name a crossgen2 executable.
  • In repo$(Crossgen2InBuildDir); crossgen2 is built unconditionally by the clr subset.
  • Out of repo — the wasm-tools workload declares the existing Microsoft.NETCore.App.Crossgen2.<host-rid> pack, whose Sdk/Sdk.props defines $(Crossgen2ToolPath). ~12.5 MB.

The SDK resolves this pack only when PublishReadyToRun is set, which wasm CoreCLR apps never set — hence the workload. dotnet/sdk#56119 proposes acquiring it directly instead, which would let the workload entry go. If none of the three resolve, the targets error rather than passing an empty path down.

The pack is named for the machine that runs crossgen2, not the target: generation never loads the JIT, so a host-targeting crossgen2 answers wasm ABI questions correctly.

The workload-testing legs do not set $(BuildHostTools), so nothing produced a crossgen2 pack for their local package feed. (The perf browser-wasm leg does produce one, but only because it opts in — #133143.) Microsoft.NETCore.App.Crossgen2.Host.sfxproj pins the RID to the build host, and is now built by the CoreCLR browser-wasm leg behind $(BuildCrossgen2HostPackForWorkloadTesting), guarded on $(BuildHostTools) being unset so the two paths can never emit the same package id twice. The official build is untouched — it already publishes this pack from the host platform legs.

Behaviour changes

WASM0066 is removed. The old task warned for every DllImport whose module did not resolve to a linked-in native library — a CoreCLR-only divergence that fires on ordinary cross-platform code never executed on wasm (#131874 reports ten from SkiaSharp alone on a shipped Preview 7 SDK). In-tree it had already accumulated two NoWarn suppressions and a WarnOnUnresolvedPInvokeModules=false; all three go, along with the --no-warn-unresolved-directpinvoke opt-out that existed only to silence it. An unresolved module is not knowably wrong at build time: callhelpers_pinvoke_override returns nullptr on a miss, so a call that actually happens throws DllNotFoundException naming the module, as on every other platform. Dropping a warning is strictly loosening.

WASM0065 is added, as a message. Per module, when it declares P/Invokes without [assembly: DisableRuntimeMarshalling], since the generated helpers assume signatures cross unmarshalled. A message rather than a warning: it reports something the app author often cannot fix, and as a warning it would fail -warnaserror builds. Four fire across the 181 framework assemblies.

Exported callbacks with an ambiguous name are rejected. An export wrapper resolves its MethodDesc through LookupUnmanagedCallersOnlyMethodByName, which takes the first [UnmanagedCallersOnly] method of matching name and compares no signature — so two exported overloads resolve to the same method and one wrapper calls it with the wrong arguments. Everything the generator controls carries the arity, so the existing duplicate-key and duplicate-symbol checks both pass. Generation now fails instead, naming both signatures. Only exports: a non-exported callback is found by the arity-aware key and never reaches the name lookup.

Known limitations

Verification

  • Regeneration reproduces the committed helpers byte for byte, apart from the rename above, with zero WASM0001/WASM0060/WASM0061/WASM0062 warnings across a full CoreLib+libraries scan. (The checked-in P/Invoke table is already slightly stale against main independently of this PR; that drift is left alone.)
  • WasmArgumentLayoutTests goes from 17 to 22 test methods. The two covering the rejection above were checked against a disabled check, so they test it rather than agree with it.
  • clr+libs builds clean for browser and wasi; WasmAppBuilder still builds for both net11.0 and net472.
  • Both flavors build end to end from the in-tree samples, with per-architecture native payloads, a non-PE file and duplicate-culture satellites injected into the bundle.
  • The renamed runtime contract was checked by building: libcoreclr_static.a exports g_portableCallHelperThunks and no g_wasmThunks, and the browser sample links its generated tables against it.

Contributes to #131811, closing blocking gap #1 and the struct half of gap #2: a 3-int and a 5-double struct in [UnmanagedFunctionPointer] signatures now resolve to vS12 / S12i / vS40i, where all three previously threw NotSupportedException.

Note

This pull request description was drafted with the help of GitHub Copilot.

radekdoulik and others added 2 commits August 5, 2026 13:38
The CoreCLR wasm P/Invoke generator computed ABI signatures from
System.Reflection.MetadataLoadContext, which has no field-layout engine.
Struct sizes therefore came from a 7-entry hardcoded table
(s_knownStructSizes) and anything else was a hard error (WASM0067).

Replace that table with crossgen2's own field-layout algorithms, so the
S<N> encoding is computed rather than looked up.

The size crossgen2 encodes is TypeDesc.GetElementSize(), backed by
Internal.TypeSystem. That is not a separable formula, so the change
reuses the type system itself:

- Split WasmTypes.cs and WasmLowering.cs so the ABI-lowering half no
  longer pulls in ILCompiler.ObjectWriter and Internal.JitInterface, and
  introduce IWasmTypeCacheContext to replace hard casts to
  CompilerTypeSystemContext.
- Extract crossgen2's wasm-aware VectorOfTFieldLayoutAlgorithm from
  ReadyToRunCompilerContext.cs into its own file. It differs from ILC's
  copy in that Vector<T> keeps 16-byte alignment on Wasm32, which only
  shows up in the layout of containing structs.
- Add ILCompiler.Wasm.Lowering, a small tool with its own
  MetadataTypeSystemContext that links those algorithms.

WasmAppBuilder multi-targets net11.0 and net472, and Sdk.targets.in loads
the net472 copy under MSBuild.exe, where a netcoreapp type-system
assembly cannot load. The tool therefore runs out of process and answers
one metadata token per line. The task locates it by probing two paths
relative to its own directory, which covers the in-tree, Helix and SDK
pack layouts without any consumer passing a path.

WasmLoweringParityTests loads both stacks side by side and asserts they
agree on the formerly hardcoded structs, on every CoreLib value type, and
on generic instantiations.

Single-field structs with trailing padding now correctly encode as S<N>;
the old code recursed into the field and returned a primitive char.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f6d6e5a4-5b25-4198-b42d-d5b2dc781f47
The generator asked the resolver one question per parameter type, naming
each type by metadata token. A token names a TypeDef row, so a constructed
generic — a TypeSpec, which has no row — could not be named at all:
Nullable<int> and Nullable<long> both report the token of Nullable`1. The
generator therefore refused generic types outright.

Ask for the whole signature per method instead. Parameter types then come
out of the method's signature blob, where instantiations are spelled in
full, and the string is produced by WasmLowering.GetSignature — the same
call crossgen2 makes — rather than by a second encoder here that had to be
kept in agreement with it by hand.

The stdin protocol grows a verb: 't' for the existing per-type query, 'm'
for a method plus its lowering flags. Fields are parsed right to left so
the assembly name, being the leftover, may contain spaces.

Two call sites needed care. The lowering appends the trailing 'p' and the
instance 'T' only for a managed signature, so InternalCall scanning passes
None and drops its manual += "p", while P/Invoke and icall scanning pass
IsUnmanagedCallersOnly and get neither.

Both scans now skip open generics, which have no single signature. That
was previously a warning for InternalCalls, and for a generic delegate
carrying UnmanagedFunctionPointerAttribute it silently encoded the type
parameter itself as a pointer — right only by accident, and now a hard
error from the lowering, on a path with no catch.

Regenerating src/coreclr/vm/wasm/ produces byte-identical output. The
parity test gains a sweep of 35,236 CoreLib method signatures through both
stacks, 12,270 of which name a constructed generic type.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f6d6e5a4-5b25-4198-b42d-d5b2dc781f47
Copilot AI lite review requested due to automatic review settings August 5, 2026 14:42
@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.

@radekdoulik

Copy link
Copy Markdown
Member Author

This should resolve #131874

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.

Pull request overview

This PR updates the CoreCLR WebAssembly P/Invoke helper generation pipeline to compute wasm ABI struct sizing and signature lowering via crossgen2’s type system (hosted in a new out-of-process tool), replacing the previous hardcoded/reflective size logic that couldn’t reliably handle field layout or constructed generics.

Changes:

  • Add ILCompiler.Wasm.Lowering as an out-of-proc “signature resolver” tool and wire ManagedToNativeGenerator to query it for ABI tokens and full method signatures.
  • Refactor wasm lowering/shared helpers in the compiler toolchain (type-cache interface, moved wasm type encoding, split MethodDesc-facing lowering).
  • Add parity tests to ensure the standalone resolver produces the same signatures as crossgen2.

Reviewed changes

Copilot reviewed 32 out of 32 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/tasks/WasmAppBuilder/WasmAppBuilder.csproj Builds the resolver tool in RunGenerator and passes its path into ManagedToNativeGenerator.
src/tasks/WasmAppBuilder/IcallTableGenerator.cs Requires an injected CoreCLR SignatureMapper when generating CoreCLR icall signatures.
src/tasks/WasmAppBuilder/coreclr/WasmLoweringFlags.cs Task-local copy of lowering flags (mirrors compiler enum values).
src/tasks/WasmAppBuilder/coreclr/WasmAbiTypeResolver.cs New resolver client that runs ILCompiler.Wasm.Lowering once and queries over stdin/stdout.
src/tasks/WasmAppBuilder/coreclr/SignatureMapper.cs Converts from static helper + hardcoded struct sizes to instance-based mapper backed by resolver.
src/tasks/WasmAppBuilder/coreclr/PInvokeTableGenerator.cs Routes signature/name token decisions through the new SignatureMapper instance.
src/tasks/WasmAppBuilder/coreclr/PInvokeCollector.cs Uses resolver-backed signature computation; skips open generic callback delegates.
src/tasks/WasmAppBuilder/coreclr/ManagedToNativeGenerator.cs Adds SignatureResolverPath/DotNetHostPath and initializes resolver + mapper for generation.
src/tasks/WasmAppBuilder/coreclr/IWasmAbiTypeResolver.cs New abstraction for “type token” and “method signature” ABI queries.
src/tasks/WasmAppBuilder/coreclr/InternalCallSignatureCollector.cs Uses resolver-based lowering for InternalCall signatures; skips generic InternalCalls.
src/mono/nuget/Microsoft.NET.Runtime.WebAssembly.Wasi.Sdk/Microsoft.NET.Runtime.WebAssembly.Wasi.Sdk.pkgproj Ships the resolver tool in the SDK pack output layout.
src/mono/nuget/Microsoft.NET.Runtime.WebAssembly.Sdk/Microsoft.NET.Runtime.WebAssembly.Sdk.pkgproj Ships the resolver tool in the SDK pack output layout.
src/coreclr/tools/Common/JitInterface/WasmLowering.MethodDesc.cs New split file for MethodDesc-based lowering + flag computation.
src/coreclr/tools/Common/JitInterface/WasmLowering.cs Refactors to use IWasmTypeCacheContext and narrows API surface in this file.
src/coreclr/tools/Common/Compiler/IWasmTypeCacheContext.cs New interface for caching/round-tripping wasm-lowered struct/v128 types.
src/coreclr/tools/Common/Compiler/DependencyAnalysis/Target_Wasm/WasmTypes.Encoding.cs Splits encoding/mangling/JIT interface conversions out of WasmTypes.cs.
src/coreclr/tools/Common/Compiler/DependencyAnalysis/Target_Wasm/WasmTypes.cs Keeps the wasm type model “type-system only” and makes types partial to split helpers.
src/coreclr/tools/Common/Compiler/CompilerTypeSystemContext.Wasm.cs Implements IWasmTypeCacheContext on the compiler context.
src/coreclr/tools/aot/ILCompiler.Wasm.Lowering/WasmTypeSystemContext.cs New minimal wasm-configured type system context used by the resolver tool.
src/coreclr/tools/aot/ILCompiler.Wasm.Lowering/WasmMetadataFieldLayoutAlgorithm.cs New wasm field-layout algorithm mirroring crossgen2 instance layout logic.
src/coreclr/tools/aot/ILCompiler.Wasm.Lowering/WasmAbiTypeResolver.cs Resolver API implementation: per-type token and per-method signature queries.
src/coreclr/tools/aot/ILCompiler.Wasm.Lowering/Program.cs Implements the stdin/stdout query server protocol (“ready”, t ..., m ...).
src/coreclr/tools/aot/ILCompiler.Wasm.Lowering/ILCompiler.Wasm.Lowering.csproj New tool project, links shared lowering/type sources and pins output path.
src/coreclr/tools/aot/ILCompiler.TypeSystem/ILCompiler.TypeSystem.csproj Grants internals visibility to the resolver tool.
src/coreclr/tools/aot/ILCompiler.RyuJit/ILCompiler.RyuJit.csproj Includes the new WasmLowering.MethodDesc.cs split file.
src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj Includes split wasm encoding + cache interface + MethodDesc lowering file.
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/VectorOfTFieldLayoutAlgorithm.cs Extracted Vector<T> layout algorithm into a standalone file.
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilerContext.cs Removes the now-extracted nested VectorOfTFieldLayoutAlgorithm type.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/WasmLoweringParityTests.cs New parity tests comparing crossgen2 vs resolver lowering across CoreLib.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/ILCompiler.ReadyToRun.Tests.csproj Adds aliased reference to the resolver tool for side-by-side parity testing.
src/coreclr/tools/aot/ILCompiler.Compiler/ILCompiler.Compiler.csproj Includes split wasm encoding + cache interface file.
Directory.Build.props Adds WasmSignatureResolverDir for pinned resolver output placement.

Comment thread src/tasks/WasmAppBuilder/coreclr/SignatureMapper.cs Outdated
Comment thread src/tasks/WasmAppBuilder/WasmAppBuilder.csproj Outdated
@jkotas

jkotas commented Aug 5, 2026

Copy link
Copy Markdown
Member

Would it be better to expose this as crossgen command line option instead of repackaging large part of crossgen in a new tool?

@radekdoulik

Copy link
Copy Markdown
Member Author

Would it be better to expose this as crossgen command line option instead of repackaging large part of crossgen in a new tool?

That could work too. We don't include crossgen in the packages yet IIRC, I can add it in this PR though. I will try it.

For the record, the new tool is 632K, crossgen pack is 36M (measured on mac). We will ship the crossgen anyway, so it indeed makes sense to add the new option instead of the new tool.

radekdoulik and others added 2 commits August 5, 2026 23:04
The WasmAppBuilder generator needs struct sizes to build the signature
strings that describe P/Invokes to the interpreter, and metadata alone
does not give them. The previous commits added a standalone
ILCompiler.Wasm.Lowering tool for that, which meant extracting parts of
crossgen2 into shareable sources so a second host could link them.

Jan Kotas pointed out that crossgen2 already exposes exactly this: it
computes wasm signatures during compilation and always has. The tool
added no capability, only a second host for an API that already existed.
So this replaces it with a --wasm-abi-query mode on crossgen2 and
reverts every extraction that existed to serve the tool.

What is left in src/coreclr/tools is the query mode itself plus its
wiring, and one word in WasmLowering.cs widening the encoding table from
private to internal. crossgen2 is built by the 'clr' subset already, so
it is present wherever the generator runs; the old tool was in no subset
at all, which is why three library-test legs could not find it.

Query mode configures a compilation group before answering, because the
ReadyToRun field layout algorithm asks the group whether a derived type
needs its base offset aligned and a struct holding a reference reaches
that path. All inputs go in one version bubble: the alignment exists to
keep offsets baked into precompiled code valid, and the interpreter
computes layout itself.

Regenerating the CoreCLR helpers through this mode reproduces the
committed output byte for byte, using the published, trimmed,
single-file crossgen2 apphost.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f6d6e5a4-5b25-4198-b42d-d5b2dc781f47
Copilot AI review requested due to automatic review settings August 6, 2026 08:48

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.

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/WasmAbiQuery.cs:49

  • This type doesn’t appear to need to be public. ILCompiler.ReadyToRun already declares InternalsVisibleTo for ILCompiler.ReadyToRun.Tests, so making this internal would reduce accidental surface area without impacting the crossgen2 entrypoint or tests.
    public static class WasmAbiQuery
    {

Comment thread src/tasks/WasmAppBuilder/coreclr/WasmAbiTypeResolver.cs Outdated
radekdoulik and others added 2 commits August 6, 2026 12:36
The wasm P/Invoke generator asks crossgen2 for the ABI signature of each
P/Invoke it finds. In the repo crossgen2 comes from the build output, but
out of repo -- relinking from a restored SDK -- nothing resolved it, so
$(Crossgen2Path) reached the task empty and the build failed.

The SDK does resolve a crossgen2 pack, but only when PublishReadyToRun is
set, which a wasm CoreCLR app never sets. So declare the existing
Microsoft.NETCore.App.Crossgen2.<host-rid> pack in the wasm-tools workload
manifest instead, and ship an Sdk/Sdk.props inside that pack so the import
defines $(Crossgen2ToolPath).

Query mode never loads the JIT, so the host-targeting pack answers wasm
questions correctly; regenerating the browser helpers through the
NativeAOT-built pack binary reproduces the committed output byte for byte.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f6d6e5a4-5b25-4198-b42d-d5b2dc781f47
Installing wasm-tools for testing pulls every pack the manifest declares,
from a feed populated by the wasm build legs. None of them produce a
crossgen2 pack: a pack is named for the machine that *runs* the tool, so
building the regular pack project for a wasm target would yield
Microsoft.NETCore.App.Crossgen2.browser-wasm -- a crossgen2 that runs in
the browser. Subsets.props excludes it for that reason, correctly.

The Host variant pins the RID to the build host instead, which is exactly
the pack the workload resolves. Build it from the CoreCLR browser-wasm leg,
which already has the CoreCLR artifacts it needs, and stage its nupkg
alongside the runtime pack.

Opt-in via BuildCrossgen2HostPackForWorkloadTesting so the official build
is untouched -- it already publishes this pack from the host platform legs,
and a second copy would collide on package id.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f6d6e5a4-5b25-4198-b42d-d5b2dc781f47
Comment thread src/coreclr/tools/aot/crossgen2/Properties/Resources.resx Outdated
Copilot AI review requested due to automatic review settings August 6, 2026 16:10

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.

🟡 Changes recommended

There are merge-blocking nullable/treat-warnings-as-errors issues in newly added managed code that will fail the build as-is.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/tasks/WasmAppBuilder/FilterManagedAssemblies.cs:25

  • Assemblies is declared nullable (ITaskItem[]?) but is dereferenced unconditionally (Assemblies.Length / foreach). With nullable enabled and TreatWarningsAsErrors=true, this will produce CS8602 (and can NRE if MSBuild ever invokes the task without setting it). Make the property non-nullable and initialize with null! so [Required] validation remains meaningful.
  • Files reviewed: 52/52 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/WasmArgumentLayoutTests.cs Outdated
A test that looks for a substring in the generated C proves nothing runs,
and has to be kept in step with the generator to stay green. Remove the
three that did that, including the one covering how a floating point
callback argument reaches the interpreter, which end-to-end interop tests
are the right place to catch.

The tests left compare what the interop encoder returns against what the
compiler's own lowering returns, so they cannot drift from the generator
without one of the two changing.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f6d6e5a4-5b25-4198-b42d-d5b2dc781f47
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f6d6e5a4-5b25-4198-b42d-d5b2dc781f47

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.

🔵 Needs a closer look

The test corerun relink target wires crossgen2 discovery through a private property rather than the shared PortableCallHelpersGeneratorPath, making tool-path overrides inconsistent with the rest of the PR.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/tests/Common/CLRTest.WasmCorerun.targets:334

  • The generator path is wired through the private property $(_WasmCorerunGeneratorPath), which is inconsistent with the app-side targets (BrowserWasmApp.CoreCLR.targets / WasiApp.CoreCLR.targets) that use $(PortableCallHelpersGeneratorPath) and $(Crossgen2ToolPath). This makes it harder to override the tool path consistently across build + tests and prevents reuse of the pack-provided Crossgen2ToolPath when available.

This issue also appears on line 348 of the same file.

src/tests/Common/CLRTest.WasmCorerun.targets:348

  • This Exec still invokes $(_WasmCorerunGeneratorPath). If the tool-path wiring is unified on $(PortableCallHelpersGeneratorPath), this should use that property so the generator actually runs from the resolved/overridden location.
  • Files reviewed: 52/52 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Comment thread src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/WasmArgumentLayoutTests.cs Outdated
Comment thread src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/WasmArgumentLayoutTests.cs Outdated

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.

🔵 Needs a closer look

It changes critical wasm build/generation plumbing across crossgen2, MSBuild targets, and runtime symbol contracts, and needs a human to validate cross-platform build behavior end-to-end.

Review details

Suppressed comments (2)

src/coreclr/tools/aot/ILCompiler.ReadyToRun/PortableCallHelpers/PortableCallHelpersGenerator.cs:23

  • PortableCallHelpersGeneratorOptions leaves OutputDirectory and TargetOS as null unless the caller sets them, but the generator later assumes these are usable strings. Initializing them to non-null defaults avoids accidental NullReferenceExceptions and makes the required/optional contract clearer (the existing validation can still enforce non-empty).
    public sealed class PortableCallHelpersGeneratorOptions
    {
        public string OutputDirectory { get; init; }
        public IReadOnlyList<string> PInvokeModules { get; init; } = [];
        public string TargetOS { get; init; }
    }

src/coreclr/tools/aot/ILCompiler.ReadyToRun/PortableCallHelpers/PInvokeCollector.cs:101

  • PInvokeCallback.EntryPoint and EntrySymbol can remain null for non-exported callbacks / before EmitNativeToInterp assigns them, but they are typed as non-nullable string. Give them non-null defaults (or make them nullable) to prevent future accidental null dereferences and to better reflect the lifecycle of these values.
        public MethodSignature Parameters => Method.Signature;
        public string EntryPoint { get; }
        public EcmaMethod Method { get; }
        public string EntrySymbol { get; set; }
        public string AssemblyName { get; }
  • Files reviewed: 52/52 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

radekdoulik and others added 2 commits September 3, 2026 20:43
…LayoutTests.cs

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
…LayoutTests.cs

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

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.

🔵 Needs a closer look

It rewires a broad build+interop generation pipeline across crossgen2/MSBuild/runtime codepaths, and the remaining concern around generator-path override consistency warrants human validation before merge.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/tests/Common/CLRTest.WasmCorerun.targets:334

  • The crossgen2 path override here is wired to the private property $(_WasmCorerunGeneratorPath) and only defaults from $(Crossgen2InBuildDir). This is inconsistent with the rest of the build (which uses $(PortableCallHelpersGeneratorPath) / $(Crossgen2ToolPath)) and makes the error message point users at an internal underscore property.

Consider accepting $(PortableCallHelpersGeneratorPath) (and optionally $(Crossgen2ToolPath)) as the override/fallback source, while still allowing $(_WasmCorerunGeneratorPath) to win when explicitly set.

  • Files reviewed: 52/52 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

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

Please edit the PR description to only keep what's relevant for the final delta.

@jkotas

jkotas commented Sep 4, 2026

Copy link
Copy Markdown
Member

cc @dotnet/crossgen-contrib FYI

…ct-sizes-from-crossgen2

Conflict in src/coreclr/vm/wasm/helpers.cpp, between dotnet#132965 - which deleted
the hand-written R2R-to-interpreter thunks and made LookupManagedThunk read
the thunks crossgen2 emits - and this branch's rename of the call-helper
symbols from wasm to portable.

Resolved by taking main's file and reapplying the renames, since renaming is
this branch's only change to it: StringToWasmSigThunk[Hash] ->
StringToPortableSigThunk[Hash], and g_wasmThunks[Count] ->
g_portableCallHelperThunks[Count]. The g_wasmPortableEntryPointThunks table
that this branch also renamed is gone with the thunks it held, so that half of
the rename drops out. Main's new comments name g_wasmThunks in prose and were
renamed with it.

The resolved file differs from origin/main by those renames alone. Building
clr.runtime for browser-wasm recompiles helpers.cpp and the three generated
callhelpers translation units, and succeeds with no warnings.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f6d6e5a4-5b25-4198-b42d-d5b2dc781f47

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.

🔵 Needs a closer look

It makes broad build/toolchain/runtime-contract changes across crossgen2, VM wasm helpers, workloads, and CI that need careful human validation and full CI signal.

Review details

Suppressed comments (1)

src/coreclr/tools/aot/ILCompiler.ReadyToRun/PortableCallHelpers/PInvokeCollector.cs:101

  • PInvokeCallback.EntryPoint and EntrySymbol can be left uninitialized for non-export callbacks, but are declared as non-nullable strings. Initializing them avoids null values leaking into later formatting/logging if code changes, and makes the type’s invariants explicit.
        public MethodSignature Parameters => Method.Signature;
        public string EntryPoint { get; }
        public EcmaMethod Method { get; }
        public string EntrySymbol { get; set; }
        public string AssemblyName { get; }
  • Files reviewed: 52/52 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread src/tests/Common/CLRTest.WasmCorerun.targets

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

@maraf it would be good if you can have look later. We can address your feedback in followup

@radekdoulik

Copy link
Copy Markdown
Member Author

/ba-g the runtime-diagnostics (SOSTests cDAC_windows_x64_release) is unrelated and is failing on main and other PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arch-wasm WebAssembly architecture area-ReadyToRun

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants