[wasm] R2R bring-up prerequisites for browser - #133000
Conversation
|
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. |
There was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extracts a set of build/test plumbing prerequisites needed for ReadyToRun (R2R) bring-up on browser/wasi targets, plus a small refactor in SignatureMapper to allow token-mapping logic to be compiled independently of MSBuild-dependent code.
Changes:
- Disable embedded PGO/mibc usage for R2R when
TargetOSisbrowserorwasito avoid crossgen2 failures in wasm cross-module scenarios. - Adjust platform/build scripts for
wasi(CMake detection and Windows.cmdcross-target handling) and relax browser test trimming defaults outside CI. - Extend the Crossgen2 MSBuild task with a composite-only extra-args property and split
SignatureMapperinto MSBuild-free token mapping + reflection-based logic.
File summaries
| File | Description |
|---|---|
| eng/codeOptimization.targets | Disables IncludeMibcFilesInReadyToRun for browser/wasi to avoid wasm crossgen2 embedded-PGO crashes. |
| eng/native/configureplatform.cmake | Detects wasi via CLR_CMAKE_HOST_OS to match other “host OS” platform detection blocks. |
| eng/testing/tests.browser.targets | Forces PublishTrimmed=false only under CI, allowing local runs to follow normal trimming defaults. |
| src/coreclr/build-runtime.cmd | Treats wasi as a cross-target in the Windows CoreCLR build script. |
| src/native/libs/build-native.cmd | Treats wasi as a cross-target in the Windows native-libs build script. |
| src/mono/wasi/build/WasiApp.CoreCLR.targets | Updates a comment related to unresolved P/Invoke module warnings during generator runs. |
| src/tasks/Crossgen2Tasks/RunReadyToRunCompiler.cs | Adds Crossgen2CompositeExtraCommandLineArgs and appends it to the composite crossgen2 response file. |
| src/tasks/WasmAppBuilder/coreclr/SignatureMapper.cs | Converts SignatureMapper to partial and removes token-mapping members moved to a new file. |
| src/tasks/WasmAppBuilder/coreclr/SignatureMapper.Tokens.cs | New MSBuild-free token parsing/mapping half of SignatureMapper for direct compilation by tests. |
| src/tests/Common/CLRTest.WasmCorerun.targets | Updates a comment related to unresolved P/Invoke module warnings during test call-helper generation. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
It introduces at least one unintended behavior change (commented-out MSBuild logic) and a security-hardening gap in Node env-var merging that should be addressed before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/tasks/Crossgen2Tasks/RunReadyToRunCompiler.cs:26
- The new
Crossgen2CompositeExtraCommandLineArgsproperty is appended into the composite response file, but there are currently no MSBuild.targetsor callers setting it (a repo-wide search only finds it in this task implementation). As-is, this looks like dead plumbing unless it's also wired from the build targets that invokeRunReadyToRunCompiler.
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
A comment block in BrowserWasmApp.CoreCLR.targets currently comments out an active WasmBuildNative default, unintentionally changing behavior for WasmPerformanceInstrumentation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/mono/browser/build/BrowserWasmApp.CoreCLR.targets:212
- The new TODO comment block unintentionally comments out the existing
WasmBuildNativedefault forWasmPerformanceInstrumentation, so projects that setWasmPerformanceInstrumentationmay no longer trigger a relink whenWasmBuildNativeis unset.
<!-- TODO-WASM: At the moment we are skipping dotnet.native.wasm that always contain diagnostic-server.
Related https://github.com/dotnet/runtime/issues/132772
<WasmBuildNative Condition="'$(WasmBuildNative)' == '' and '$(EnableDiagnostics)' == 'true'">true</WasmBuildNative>
<WasmBuildNative Condition="'$(WasmBuildNative)' == '' and '$(WasmPerformanceInstrumentation)' != ''">true</WasmBuildNative>
-->
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
dotnet-policy-service
Bot
added
the
linkable-framework
There was a problem hiding this comment.
🔵 Needs a closer look
A block comment in BrowserWasmApp.CoreCLR.targets unintentionally comments out a live WasmBuildNative default, changing behavior for WasmPerformanceInstrumentation.
Review details
Suppressed comments (1)
src/mono/browser/build/BrowserWasmApp.CoreCLR.targets:212
- The TODO comment block accidentally comments out the
WasmBuildNativedefault forWasmPerformanceInstrumentation. As-is, settingWasmPerformanceInstrumentationwon’t trigger relinking, which is a behavioral change unrelated to the diagnostics-server TODO and likely breaks instrumentation scenarios.
<!-- TODO-WASM: At the moment we are skipping dotnet.native.wasm that always contain diagnostic-server.
Related https://github.com/dotnet/runtime/issues/132772
<WasmBuildNative Condition="'$(WasmBuildNative)' == '' and '$(EnableDiagnostics)' == 'true'">true</WasmBuildNative>
<WasmBuildNative Condition="'$(WasmBuildNative)' == '' and '$(WasmPerformanceInstrumentation)' != ''">true</WasmBuildNative>
-->
- Files reviewed: 18/18 changed files
- Comments generated: 0 new
- Review effort level: Lite
…wasi tests Trimmed xunit test apps launch through WasmTestRunner.dll (the app entry point), copied in as a reference and not statically referenced, so ILLink dropped it and the app failed at startup resolving its entry point (MONO_WASM: The handle is invalid). Mobile's ConfigureTrimming rooted the runner + xunit trimmer descriptors, but only under EnableAggressiveTrimming, which is not set for the default browser/wasi PublishTrimmed path. Add a target covering that path.
…s under browser trimming CoreCLRWasmNativeDefaultsTests: WasmPerformanceInstrumentation no longer forces a relink (that defaulting is commented out in BrowserWasmApp.CoreCLR.targets pending dotnet#132772), so the theory row now expects no relink. EnableDiagnostics still relinks via the property registry, so it is unchanged. System.Composition (MEF) composes parts via reflection and is not trim-compatible; enabling browser trimming (EAT) surfaces 14 failures. Mark them [ActiveIssue(TestPlatforms.Browser)], matching the existing LibraryTests_EAT precedent.
1c53d12 to
bd36598
Compare
|
/azp run runtime-wasm |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
# Conflicts: # src/mono/wasi/build/WasiApp.CoreCLR.targets # src/tests/Common/CLRTest.WasmCorerun.targets
…a/runtime into browsehost_load_r2r_5
pavelsavara
changed the title
This can be now removed from the description. There might be other parts in the description, which need update |
It's gone from the change already |
radekdoulik
left a comment
radekdoulik
left a comment
There was a problem hiding this comment.
Besides the 2 comments it LGTM and can be resolved in follow up if needed.
Extracts easy-to-review prerequisites from the R2R bring-up branch (
browsehost_load_r2r) so they can be reviewed and merged independently. The main behavioral change is enabling IL trimming for browser test apps (a prerequisite for R2R); the rest is build plumbing plus the fallout fixes that enabling trimming surfaced.R2R / crossgen2 plumbing
browser/wasi. crossgen2 crashes emitting embedded PGO data with cross-module R2R for wasm, and there is no wasm PGO profile to gain from. Re-enable once [wasm][R2R] crossgen2 NotImplementedException in ModuleTokenResolver with --embed-pgo-data + --opt-cross-module #132825 is fixed.Crossgen2CompositeExtraCommandLineArgs, appended to the composite crossgen2 response file.*.a) in the runtime pack native dir.wasi as a cross-target
CLR_CMAKE_HOST_OS(matching the emscripten host block) instead ofCLR_CMAKE_TARGET_OS.wasias a cross-target.Library tests
WasmWarnOnUnresolvedPInvokeModules=false) for wasm tests — trimmed test assemblies P/Invoke into non-browser native modules (WASM0066). Suppressed in a separate commit; TODO remove after [wasm] Compute CoreCLR P/Invoke struct sizes with crossgen2's type system #131877.WasmBuildNative defaults
WasmBuildNative=trueforEnableDiagnostics/perf-instrumentation yet ([browser][coreCLR] Multiple flavors of dotnet.native.wasm #132772).WasmBuildNative=truein Release when the in-tree defaults are not disabled (_WasmInTreeDefaults != 'false').Misc
SignatureMapper.Tokens.csso tests can compile it directly (the reflection-based half needs aLogAdapterand cannot be linked on its own).Note
This PR description was generated with the assistance of GitHub Copilot.