Allow acquiring the crossgen2 pack without PublishReadyToRun - #56119
Allow acquiring the crossgen2 pack without PublishReadyToRun#56119radekdoulik wants to merge 2 commits into
Conversation
crossgen2 carries the type system that knows a target's ABI, which a build can need for reasons other than compiling ReadyToRun images. WebAssembly is one: the CoreCLR wasm interop helpers encode struct sizes and argument lowering that cannot be derived from metadata alone, so a native relink runs crossgen2 whether or not the app is R2R. Both halves of the existing path are tied to PublishReadyToRun. The pack is requested only under ReadyToRunEnabled, so it is never restored, and ResolveReadyToRunCompilers runs only as part of publish and reports the tool as an item scoped to that target, so a build-time consumer has nothing to read even when the pack is present. Add RequiresCrossgen2Pack to opt into the download on its own, and ResolveCrossgen2ToolPath to report the executable in $(Crossgen2ToolPath). Neither changes what a ReadyToRun build does. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f6d6e5a4-5b25-4198-b42d-d5b2dc781f47
|
Azure Pipelines: Successfully started running 1 pipeline(s). 2 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
📋 SDK Diagnostic Documentation ReminderThis PR introduces 2 new SDK diagnostic codes:
Action RequiredPlease ensure that documentation for these diagnostics is added or updated in the dotnet/docs repository at:
Each diagnostic should have:
Thank you for helping keep our documentation up to date! 🙏 |
@akoeplinger pointed out that a consumer does not need a new target to locate crossgen2: ResolveReadyToRunCompilers already reports the executable in @(Crossgen2Tool), and nothing about it is specific to compiling images. Confirmed against the stock SDK. It reads @(ResolvedCrossgen2Pack) and @(ResolvedRuntimePack), both outputs of ResolveFrameworkReferences, which is a build-time target - so once that has run the tool resolves during an ordinary build, no publish involved: Crossgen2Tool = .../microsoft.netcore.app.crossgen2.osx-arm64/10.0.5/tools/crossgen2 TargetOS = osx TargetArch = arm64 The item also carries the target OS and architecture, which the target removed here did not compute. A consumer does have to guard the call. ResolveReadyToRunCompilers errors rather than reporting nothing when no pack was resolved, and it derives the path as <pack>/tools/crossgen2, which a repo-local crossgen2 build does not match. Both are the consumer's business to condition around, not the SDK's. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f6d6e5a4-5b25-4198-b42d-d5b2dc781f47
|
Azure Pipelines: Successfully started running 1 pipeline(s). 2 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
The new NETSDK1245 diagnostic can report the wrong host RID (portable vs non-portable) and the added unit test currently won’t catch that mismatch.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces a new MSBuild property (RequiresCrossgen2Pack) that lets ProcessFrameworkReferences restore/acquire the Crossgen2 tool pack even when PublishReadyToRun is disabled, enabling build-time Crossgen2 usage for non-R2R scenarios (e.g., portable call helper generation).
Changes:
- Add
RequiresCrossgen2Packparameter plumbing fromMicrosoft.NET.Sdk.FrameworkReferenceResolution.targetsintoProcessFrameworkReferences. - Extend
ProcessFrameworkReferencesto acquire the Crossgen2 tool pack when either R2R is enabled (existing behavior) orRequiresCrossgen2Packis set, and introduce new diagnostics (NETSDK1245/1246) for non-R2R acquisition failures. - Add unit tests covering acquisition gating and the new diagnostics, plus new localized resource entries.
File summaries
| File | Description |
|---|---|
| test/Microsoft.NET.Build.Tasks.Tests/ProcessFrameworkReferencesTests.cs | Adds tests for RequiresCrossgen2Pack acquisition and NETSDK1245/1246 diagnostics. |
| src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets | Forwards $(RequiresCrossgen2Pack) into the ProcessFrameworkReferences task invocation. |
| src/Tasks/Microsoft.NET.Build.Tasks/ProcessFrameworkReferences.cs | Implements Crossgen2 pack acquisition without R2R and adds new diagnostics for unsupported host/TFM. |
| src/Tasks/Common/Resources/Strings.resx | Adds NETSDK1245/NETSDK1246 resource strings. |
| src/Tasks/Common/Resources/xlf/Strings.cs.xlf | Adds localized entries for NETSDK1245/NETSDK1246. |
| src/Tasks/Common/Resources/xlf/Strings.de.xlf | Adds localized entries for NETSDK1245/NETSDK1246. |
| src/Tasks/Common/Resources/xlf/Strings.es.xlf | Adds localized entries for NETSDK1245/NETSDK1246. |
| src/Tasks/Common/Resources/xlf/Strings.fr.xlf | Adds localized entries for NETSDK1245/NETSDK1246. |
| src/Tasks/Common/Resources/xlf/Strings.it.xlf | Adds localized entries for NETSDK1245/NETSDK1246. |
| src/Tasks/Common/Resources/xlf/Strings.ja.xlf | Adds localized entries for NETSDK1245/NETSDK1246. |
| src/Tasks/Common/Resources/xlf/Strings.ko.xlf | Adds localized entries for NETSDK1245/NETSDK1246. |
| src/Tasks/Common/Resources/xlf/Strings.pl.xlf | Adds localized entries for NETSDK1245/NETSDK1246. |
| src/Tasks/Common/Resources/xlf/Strings.pt-BR.xlf | Adds localized entries for NETSDK1245/NETSDK1246. |
| src/Tasks/Common/Resources/xlf/Strings.ru.xlf | Adds localized entries for NETSDK1245/NETSDK1246. |
| src/Tasks/Common/Resources/xlf/Strings.tr.xlf | Adds localized entries for NETSDK1245/NETSDK1246. |
| src/Tasks/Common/Resources/xlf/Strings.zh-Hans.xlf | Adds localized entries for NETSDK1245/NETSDK1246. |
| src/Tasks/Common/Resources/xlf/Strings.zh-Hant.xlf | Adds localized entries for NETSDK1245/NETSDK1246. |
Review details
Suppressed comments (2)
test/Microsoft.NET.Build.Tasks.Tests/ProcessFrameworkReferencesTests.cs:321
- The NETSDK1245 test sets NETCoreSdkRuntimeIdentifier and NETCoreSdkPortableRuntimeIdentifier to the same value, which means it won’t catch cases where the task uses the portable RID for host pack lookup (and the diagnostic should name the portable RID). This can let regressions slip through for non-portable SDK scenarios where these values differ.
var config = new TaskConfiguration
{
TargetFrameworkVersion = "11.0",
EnableRuntimePackDownload = true,
NETCoreSdkRuntimeIdentifier = "win-x64",
NETCoreSdkPortableRuntimeIdentifier = "win-x64",
RuntimeGraphPath = CreateRuntimeGraphFile(MultiPlatformRuntimeGraph),
test/Microsoft.NET.Build.Tasks.Tests/ProcessFrameworkReferencesTests.cs:338
- After making NETCoreSdkRuntimeIdentifier differ from NETCoreSdkPortableRuntimeIdentifier, this test should also assert that the NETSDK1245 message names the portable RID (the value used for the host lookup in portable mode), not the non-portable host RID. Without a negative assertion, it’s easy for the message to accidentally include the wrong RID and still satisfy the current Contains check.
var engine = (MockNeverCacheBuildEngine4)task.BuildEngine;
var error = engine.Errors.Should().ContainSingle().Subject;
error.Code.Should().Be("NETSDK1245");
error.Message.Should().Contain("win-x64", "the message names the host it could not find a pack for");
}
- Files reviewed: 17/17 changed files
- Comments generated: 2
- Review effort level: Lite
| if (ReadyToRunEnabled) | ||
| { | ||
| Log.LogError(Strings.ReadyToRunNoValidRuntimePackageError); | ||
| } | ||
| else if (crossgen2PackSupport is ToolPackSupport.UnsupportedForHostRuntimeIdentifier) | ||
| { | ||
| Log.LogError(Strings.Crossgen2UnsupportedHostRuntimeIdentifier, NETCoreSdkRuntimeIdentifier); | ||
| } |
| var config = new TaskConfiguration | ||
| { | ||
| TargetFrameworkVersion = "11.0", | ||
| EnableRuntimePackDownload = true, | ||
| NETCoreSdkRuntimeIdentifier = "win-x64", |
…stem (#131877) 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](#131877) 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 - **wasi has no out-of-repo acquisition path.** `wasi-experimental` extends `microsoft-net-runtime-mono-tooling`, not `wasm-tools`, so it picks up no crossgen2 pack; the targets error explicitly there. Browser is the shipping wasm/CoreCLR target. - **Reverse thunks allocate one `int64_t` slot per managed parameter**, while a by-value struct argument occupies `ceil(size/8)` interpreter slots. No `[UnmanagedCallersOnly]` callback in CoreLib or the libraries takes a by-value struct, so nothing exercises this. The old generator rejected such callbacks with `WASM0067`; this one accepts them, so user code would get a bad thunk rather than a diagnostic. - **`'V'` (v128) has no case in the C++ emission helpers.** Pre-existing; fails loudly. - **Multi-slot types (`Int128`, `Vector256`, …) are rejected at the thunk emitter** rather than at the interop boundary, so the diagnostic differs from the old `WASM0068`. Still a clean `crossgen2 : error :` with exit 1. No such P/Invoke exists today. - Does not re-enable the tests disabled in #131811 (#133187), and does not address gaps #3–#7 there. ## 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. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jan Kotas <jkotas@microsoft.com> Copilot-Session: f6d6e5a4-5b25-4198-b42d-d5b2dc781f47
Summary
dotnet/runtime#131877 (open) proposes a second job for crossgen2: alongside compiling ReadyToRun images, a
--generate-portable-callhelpersmode that generates interop call helpers. A build would then need to run crossgen2 with ReadyToRun not involved at all.Acquisition is what blocks that today.
ProcessFrameworkReferencesrequests the pack only underReadyToRunEnabled && ReadyToRunUseCrossgen2, so with R2R off it is never restored. This addsRequiresCrossgen2Packto request it on its own, without changing what a ReadyToRun build does.WebAssemblySdkis acquired the same way a few lines below, so "a build needs a tool pack for its own reasons" is an established shape in this method.Discovery needs nothing. An earlier revision of this PR also added a
ResolveCrossgen2ToolPathtarget, on the belief thatResolveReadyToRunCompilerswas reachable only throughCreateReadyToRunImagesat publish. @akoeplinger pointed out that a consumer can just depend on the existing target — and that is right. It reads@(ResolvedCrossgen2Pack)and@(ResolvedRuntimePack), both outputs ofResolveFrameworkReferences, which is a build-time target. Measured against a stock 10.0.105 SDK during a plaindotnet build, no publish involved:The item also carries the target OS and architecture, which the removed target did not compute. So that half is gone, and what is left here is acquisition alone.
Consuming it
A consumer depends on
ResolveReadyToRunCompilersand reads@(Crossgen2Tool). Two things it has to condition around, both its own business rather than the SDK's:NETSDK1094, orNETSDK1095if the pack is present but unusable<pack>/tools/crossgen2, which a repo-local crossgen2 build does not matchSo the call belongs behind a condition that skips it whenever the consumer already knows where the tool is.
Motivation
The CoreCLR WebAssembly interop helpers encode struct sizes and argument lowering that cannot be derived from metadata alone, so a native relink has to run crossgen2 whether or not the app is ReadyToRun. dotnet/runtime#131877 currently arranges that by declaring the crossgen2 pack in the
wasm-toolsworkload manifest, which @akoeplinger questioned in dotnet/runtime#131877 (comment):This is that change, opened for discussion. Nothing sets
RequiresCrossgen2Packyet, and the workload entry would stay until an SDK carrying this flows.Note that if ReadyToRun becomes the default for CoreCLR browser (dotnet/runtime#132466), the existing gate already downloads the pack at restore, since
ProcessFrameworkReferencesruns beforeCollectPackageDownloadsfor build as well as publish.RequiresCrossgen2Packthen covers the narrower case of a user who setsPublishReadyToRun=falseand still needs their relink to work.Diagnostics
The ReadyToRun path keeps
NETSDK1094, which tells the user to turnPublishReadyToRunoff — no help to a build that asked for the tool itself. Following thePublishAotprecedent in the same method, the new path reports whichever lookup failed:NETSDK1245— The crossgen2 pack is not available for the build host platform '{0}'.NETSDK1246— The crossgen2 pack is not available for the target framework.These are the only two results
AddToolPackcan return forCrossgen2; the others belong to theILLink/ILCompilerbranches.Testing
Added to
ProcessFrameworkReferencesTests:RequiresCrossgen2Packalone,PublishReadyToRunalone, and neither — verified to fail when the gate change is revertedMicrosoft.NET.Build.Tasks.Tests --filter ProcessFrameworkReferencespasses 44/44 locally. The end-to-end wasm scenario is not exercised, since nothing sets the new property yet.Note
This description was drafted with GitHub Copilot.