Uh oh!
There was an error while loading. Please reload this page.
[wasm][R2R] testing ... - #132419
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the WebAssembly build/pack pipeline to support (and validate) staging ReadyToRun (R2R) “webcil-in-wasm” images for CoreCLR browser-wasm, adds a basic regression test for the staged framework R2R marker, and includes a small CoreCLR ETW robustness fix.
Changes:
- Add MSBuild/SDK plumbing to stage prebuilt R2R webcil-in-wasm images during build/publish for CoreCLR browser-wasm, and to generate framework R2R images into the runtime pack layout.
- Extend Crossgen2 task RID/arch handling for
browser/wasiandwasm, and adjust local test infra for CoreCLR wasm toolchain discovery. - Add/adjust wasm test coverage around staged framework R2R and improve EventPipe diagnostics test resiliency.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks/ConvertDllsToWebCil.cs | Adds probing/staging of prebuilt R2R webcil-in-wasm images via new PrebuiltR2RDirectories input. |
| src/tasks/Crossgen2Tasks/RunReadyToRunCompiler.cs | Adds a new task parameter for composite extra args (currently not wired into command generation). |
| src/tasks/Crossgen2Tasks/ResolveReadyToRunCompilers.cs | Adds browser/wasi target OS mapping and wasm architecture support. |
| src/mono/wasm/Wasm.Build.Tests/Wasm.Build.Tests.csproj | Adjusts local EMSDK path fallback to use the shared tool cache sentinel. |
| src/mono/wasm/Wasm.Build.Tests/ReadyToRunTests.cs | New test asserting framework webcil R2R marker is present/absent based on PublishReadyToRun. |
| src/mono/wasm/Wasm.Build.Tests/Blazor/EventPipeDiagnosticsTests.cs | Adds timeout + runtime-failure detection to avoid hanging on trapped runtimes. |
| src/mono/sample/wasm/console-node/Wasm.Console.Node.Sample.csproj | Enables PublishReadyToRun for CoreCLR (when UseMonoRuntime == false). |
| src/mono/sample/wasm/browser-advanced/Wasm.Advanced.Sample.csproj | Adds commented guidance for enabling PublishReadyToRun in the sample. |
| src/mono/nuget/Microsoft.NET.Sdk.WebAssembly.Pack/build/Microsoft.NET.Sdk.WebAssembly.Browser.targets | Plumbs PrebuiltR2RDirectories into ConvertDllsToWebcil and imports CoreCLR-specific targets for R2R. |
| src/mono/nuget/Microsoft.NET.Sdk.WebAssembly.Pack/build/Microsoft.NET.Sdk.WebAssembly.Browser.CoreCLR.targets | New CoreCLR browser-wasm targets enabling/staging R2R and trimming/publish ordering hooks. |
| src/mono/browser/build/WasmApp.ReadyToRun.targets | In-tree override of ResolveReadyToRunCompilers to use in-build crossgen2. |
| src/mono/browser/build/WasmApp.InTree.props | Hooks in-tree builds to import the override targets/props for CoreCLR browser R2R. |
| src/mono/browser/build/BrowserWasmApp.CoreCLR.targets | Remaps managed-assembly scanning to trimmed IL when publish outputs are R2R webcil-in-wasm. |
| src/installer/pkg/sfx/Microsoft.NETCore.App/Microsoft.NETCore.App.Runtime.CoreCLR.sfxproj | Adds framework R2R generation/packaging for CoreCLR browser runtime pack (native/r2r). |
| src/coreclr/vm/eventtrace.cpp | Avoids ETW method rundown work when EECodeInfo is invalid. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs | Adds wasm-only guard to reject methods that would create un-encodable TypeHandle fixups. |
| src/coreclr/jit/CMakeLists.txt | Installs the wasm universal JIT into the alljits component. |
| eng/testing/scenarios/BuildWasmAppsJobsListCoreCLR.txt | Adds Wasm.Build.Tests.ReadyToRunTests to the CoreCLR wasm job list. |
| /// .dll candidate with empty <c>R2RWebcilPath</c>, the first matching image is staged instead of | ||
| /// converting the IL. Empty for Mono. | ||
| /// </summary> | ||
| public string[] PrebuiltR2RDirectories { get; set; } |
| public bool ShowCompilerWarnings { get; set; } | ||
| public bool UseCrossgen2 { get; set; } | ||
| public string Crossgen2ExtraCommandLineArgs { get; set; } | ||
| public string Crossgen2CompositeExtraCommandLineArgs { get; set; } | ||
| public ITaskItem[] Crossgen2PgoFiles { get; set; } |
| <Exec Condition="Exists('$(_WasmCrossgen2Exe)')" | ||
| Command=""$(_WasmCrossgen2Exe)" -o:"%(_WasmFrameworkR2RInput.OutputWasm)" -r:"$(LibrariesSharedFrameworkBinArtifactsPath)*.dll" -r:"$(_WasmCoreLibIL)" --targetarch:$(TargetArchitecture) --targetos:$(TargetOS) --obj-format:wasm --codegenopt:JitWasmNyiToR2RUnsupported=1 --codegenopt:JitWasmSimdNyiToR2RUnsupported=1 -O "%(_WasmFrameworkR2RInput.FullPath)"" /> |
| namespace Wasm.Build.Tests | ||
| { | ||
| // CoreCLR browser-wasm ships framework ReadyToRun images as webcil-in-wasm; a non-zero R2R table | ||
| // size in the framework webcil is the marker that R2R was produced and staged. See dotnet/runtime#121257. | ||
| public class ReadyToRunTests : WasmTemplateTestsBase | ||
| { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/tasks/Crossgen2Tasks/RunReadyToRunCompiler.cs:27
- Crossgen2CompositeExtraCommandLineArgs is added as a public task parameter but is never used when building the crossgen2 response file, so any MSBuild plumbing that sets it will have no effect (composite builds will silently ignore these args). Either wire it into GenerateCrossgen2ResponseFile (likely only when _createCompositeImage is true) or remove the property to avoid a misleading knob.
public bool ShowCompilerWarnings { get; set; }
public bool UseCrossgen2 { get; set; }
public string Crossgen2ExtraCommandLineArgs { get; set; }
public string Crossgen2CompositeExtraCommandLineArgs { get; set; }
public ITaskItem[] Crossgen2PgoFiles { get; set; }
src/tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks/ConvertDllsToWebCil.cs:31
- PrebuiltR2RDirectories is optional (the code explicitly handles it being absent), but the property is declared as non-nullable. In projects with nullable enabled this is misleading and can produce warnings; it also obscures the MSBuild contract that the parameter may be omitted/empty.
/// <summary>
/// Directories holding prebuilt R2R webcil-in-wasm images (CoreCLR browser). For a managed non-culture
/// .dll candidate with empty <c>R2RWebcilPath</c>, the first matching image is staged instead of
/// converting the IL. Empty for Mono.
/// </summary>
public string[] PrebuiltR2RDirectories { get; set; }
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 44 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/tasks/Crossgen2Tasks/RunReadyToRunCompiler.cs:27
- The new property
Crossgen2CompositeExtraCommandLineArgsis declared but never referenced anywhere in this task (and there are no MSBuild uses in the repo). As-is, it adds surface area/confusion without changing behavior. Either wire it intoGenerateCrossgen2ResponseFile()(likely only for_createCompositeImage), or remove it until it’s needed.
src/libraries/System.Collections/tests/System.Collections.Tests.csproj:13 - The comment says this project always references the
NetCoreAppMinimumFormatters build for a functional BinaryFormatter, but the reference is now conditional and skipped for browser/wasi. Please update the comment to reflect the new conditional behavior (this same comment exists in multiple test csprojs touched by this PR).
<ItemGroup>
<!-- Reference the `NetCoreAppMinimum` build which has a functional BinaryFormatter and force a private copy to ensure it's not excluded -->
<ProjectReference Include="$(LibrariesProjectRoot)System.Runtime.Serialization.Formatters\src\System.Runtime.Serialization.Formatters.csproj" Private="true" SetTargetFramework="TargetFramework=$(NetCoreAppMinimum)" Condition="'$(TargetOS)' != 'browser' and '$(TargetOS)' != 'wasi'" />
</ItemGroup>
src/mono/nuget/Microsoft.NET.Sdk.WebAssembly.Pack/build/Microsoft.NET.Sdk.WebAssembly.Browser.CoreCLR.targets:8
- The PR description is "just testing", but this change introduces substantial new behavior across build tasks, MSBuild targets, CoreCLR tooling, and test infrastructure for WASM ReadyToRun. Please update the PR description to clearly state the motivation, intended scenarios, and any validation performed so reviewers can assess risk and correctness.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 44 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/tasks/Crossgen2Tasks/RunReadyToRunCompiler.cs:27
- Crossgen2CompositeExtraCommandLineArgs is introduced but not used anywhere in RunReadyToRunCompiler (and there are no call sites in the repo). This adds dead API surface and can confuse callers about whether composite builds support distinct extra args.
src/mono/nuget/Microsoft.NET.Sdk.WebAssembly.Pack/build/Microsoft.NET.Sdk.WebAssembly.Browser.CoreCLR.targets:8 - The PR description ('just testing') doesn’t match the scope of this change set (new CoreCLR browser-wasm R2R targets, task API surface, CoreCLR VM changes, etc.). Please update the PR description to clearly state the problem being solved, the intended scenarios (build vs publish, trimmed vs untrimmed), and how the changes are validated (build/test matrix).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 44 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/tasks/Crossgen2Tasks/RunReadyToRunCompiler.cs:27
- The new
Crossgen2CompositeExtraCommandLineArgsproperty is not used anywhere inRunReadyToRunCompiler(and it isn’t passed fromMicrosoft.NET.CrossGen.targetseither), so it currently adds public surface area without affecting behavior. Consider removing it until there’s a concrete use (or wire it intoGenerateCrossgen2ResponseFilein a follow-up).
src/mono/wasm/Wasm.Build.Tests/ReadyToRunTests.cs:15 - Tests in this repo generally avoid embedding GitHub issue/PR references in comments unless required. Please drop the
dotnet/runtime#121257reference here and keep the comment focused on the behavior being asserted.
9eae2a5 to
d65cae4CompareThere was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs:2728
- CanEncodeTypeInSignature returns true for non-Ecma type definitions, but SignatureBuilder.EmitTypeSignature casts named types to EcmaType. This can incorrectly treat non-metadata (e.g., NoMetadataType) types as encodable and later cause InvalidCastException or incorrect signature emission. Return false when the type definition is not an EcmaType.
if (typeDefinition is not EcmaType ecmaTypeDefinition)
return true;
src/tests/readytorun/tests/mainv3.csproj:12
- Typo in comment: "non-sanitzed" should be "non-sanitized".
<!-- This test launches crossgen2 with dotnet, so we would need a non-sanitzed jitinterface library. To simplify our infrastructure, we'll instead skip this test. -->
src/coreclr/inc/readytorun.h:23
- The PR description ("just testing") doesn’t match the scope here (format version bump and broader R2R/Wasm/CoreCLR plumbing). Please update the PR title/description to reflect the actual intent and rationale (and ideally link the driving issue/design), or split into focused PRs.
// If you update this, ensure you run `git grep MINIMUM_READYTORUN_MAJOR_VERSION`
// and handle pending work.
#define READYTORUN_MAJOR_VERSION 26
#define READYTORUN_MINOR_VERSION 0x0002
47369f0 to
7dafcd0Compare7dafcd0 to
e018b3eComparecf4b487 to
a687750Comparea687750 to
91fb17aCompare0cb2fbb to
5c15ffcCompare…l-in-wasm as-is PrepareForReadyToRunCompilation writes the per-app R2R output as <name>.wasm when the container format is wasm, so the published candidate hits the pass-through path instead of PE conversion. ConvertDllsToWebCil recognizes a webcil-in-wasm candidate (\0asm magic) and stages it as-is rather than failing PEReader with 'Unknown file format'.
Enable PublishReadyToRun by default for CoreCLR browser, always source build-phase webcil staging from the runtime-pack native/r2r images, and add the Browser.ReadyToRun.targets crossgen2 override (activated only when an in-build crossgen2 is provided) so in-tree/WBT publish uses the wasm-aware non-composite Crossgen2Tasks shim and bypasses the base SDK's NETSDK1095 gate. _GatherWasmFilesToPublish no longer clears the bundle list when publish assets are webcil (.wasm), and the ManagedToNativeGenerator scan set is remapped to IL for both trimmed (linker output) and untrimmed (pack IL) R2R.
Drop the now-redundant per-project PublishReadyToRun opt-in from the JS interop tests (the SDK pack defaults it on) and enable TestTrimming there. Suppress WasmWarnOnUnresolvedPInvokeModules for CoreCLR wasm tests, and clarify the sfxproj comment on why framework R2R uses the CoreLib model instead of the SDK's in-place R2R.
The per-app R2R crossgen build otherwise exceeds the 4h job timeout. Add build-time TestAssemblyShardCount/Index partitioning in tests.proj (stable hash of the project file name, XUnitLogChecker kept on every shard) and split the browser CoreCLR library-tests leg into 8 shards. Inert unless TestAssemblyShardCount > 1.
Add ReadyToRunTests covering build (pack framework R2R) and publish (per-app crossgen2, trimmed/untrimmed, with and without native relink), asserting a non-zero R2R table in the CoreLib webcil and driving all Blazor pages in a real browser with no exceptions. Adds a self-contained Weather page to BlazorBasicTestApp and a BASE_DIR env accessor to locate the in-tree crossgen2 and Crossgen2Tasks shim.
Browser.props already wires the crossgen2 override generically (gated on Crossgen2InBuildDir, which in-tree builds set) and is imported via the SDK's Sdk.props, so WasmApp.InTree.props re-importing Crossgen2SdkOverridePropsPath produced MSB4011. Drop the redundant in-tree block and delete the now-orphaned WasmApp.ReadyToRun.targets (Browser.ReadyToRun.targets is an equivalent superset); fix the stale mirror comments.
5c15ffc to
5e768faCompare
just testing