Conversation
|
Azure Pipelines: Successfully started running 6 pipeline(s). 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
|
Tagging subscribers to this area: @dotnet/area-infrastructure-libraries |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved configuration, trimmed-closure assertion, and MVID image-validation findings remain.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
src/tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks/ConvertDllsToWebCil.cs — Reject unreadable prebuilt images before staging |
|
src/mono/nuget/Microsoft.NET.Sdk.WebAssembly.Pack/build/Microsoft.NET.Sdk.WebAssembly.Browser.props — Gate the CoreCLR crossgen override away from Mono builds |
What changed in this PR
Enables trimmed CoreCLR browser library-test CI with ReadyToRun while preserving separate interpreter coverage.
Changes:
- Adds scoped trimming/R2R properties and dedicated CI lanes.
- Adds CoreCLR browser publishing, Crossgen2 resolution, WebCIL staging, and MVID validation.
- Adds R2R test coverage, quarantines, Helix propagation, and workflow documentation.
| File | Summary |
|---|---|
src/tasks/Microsoft.NET.WebAssembly.Webcil/WebcilReader.cs |
Corrects metadata-reader disposal. |
src/tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks/ConvertDllsToWebCil.cs |
Validates and stages R2R WebCIL images. |
src/mono/wasm/Wasm.Build.Tests/WebcilInWasmSizesTests.cs |
Covers WebCIL/R2R staging behavior. |
src/mono/wasm/Wasm.Build.Tests/Templates/WasmTemplateTestsBase.cs |
Pins Crossgen2 pack versions. |
src/mono/wasm/Wasm.Build.Tests/ReadyToRunTests.cs |
Tests R2R publishing and trimmed closure behavior. |
src/mono/wasm/Wasm.Build.Tests/Common/EnvironmentVariables.cs |
Adds test environment configuration. |
src/mono/wasm/testassets/BlazorBasicTestApp/App/Pages/Weather.razor |
Supports browser interaction testing. |
src/mono/sample/wasm/Directory.Build.props |
Updates sample build properties. |
src/mono/nuget/Microsoft.NET.Sdk.WebAssembly.Pack/build/Microsoft.NET.Sdk.WebAssembly.Browser.props |
Activates CoreCLR R2R wiring. |
src/mono/nuget/Microsoft.NET.Sdk.WebAssembly.Pack/build/Microsoft.NET.Sdk.WebAssembly.Browser.CoreCLR.targets |
Implements CoreCLR browser R2R publishing. |
src/mono/nuget/Microsoft.NET.Sdk.WebAssembly.Pack/build/Microsoft.NET.Sdk.WebAssembly.Browser.CoreCLR.ReadyToRun.targets |
Resolves browser-compatible Crossgen2 tooling. |
src/mono/nuget/Microsoft.NET.Sdk.WebAssembly.Pack/build/Microsoft.NET.Sdk.WebAssembly.Browser.CoreCLR.props |
Imports CoreCLR R2R configuration. |
src/mono/browser/build/WasmApp.ReadyToRun.targets |
Updates browser R2R target wiring. |
src/mono/browser/build/WasmApp.InTree.props |
Updates in-tree browser build properties. |
src/libraries/System.Runtime/tests/System.Threading.Tasks.Tests/System.Runtime.CompilerServices/AsyncProfilerV2Tests.cs |
Adds targeted R2R quarantines. |
src/libraries/System.Runtime/tests/System.Threading.Tasks.Tests/System.Runtime.CompilerServices/AsyncProfilerV1Tests.cs |
Adds targeted R2R quarantines. |
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Runtime/CompilerServices/MethodImplAttributeTests.cs |
Adds R2R-specific test handling. |
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Reflection/MethodBaseTests.cs |
Adds R2R-specific test handling. |
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Reflection/CustomAttributeDataTests.cs |
Adds R2R-specific test handling. |
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ExceptionTests.cs |
Adds R2R-specific test handling. |
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/DelegateTests.cs |
Adds R2R-specific test handling. |
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ArrayTests.cs |
Adds R2R-specific test handling. |
src/libraries/System.Runtime/tests/System.Runtime.Tests/System.Runtime.Tests.csproj |
Updates trimming and assembly staging metadata. |
src/libraries/System.Runtime.InteropServices.JavaScript/tests/System.Runtime.InteropServices.JavaScript.UnitTests/System.Runtime.InteropServices.JavaScript.Tests.csproj |
Configures JavaScript interop test publishing. |
src/libraries/sendtohelix-browser.targets |
Propagates browser test settings. |
src/libraries/Directory.Build.props |
Adds test-scoped R2R enablement. |
src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs |
Adds WASM R2R detection. |
src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj |
Updates CoreLib trimming configuration. |
eng/testing/tests.browser.targets |
Configures browser trimming and R2R settings. |
eng/pipelines/runtime.yml |
Adds standard browser R2R coverage. |
eng/pipelines/extra-platforms/runtime-extra-platforms-wasm.yml |
Adds extra-platform R2R coverage. |
eng/pipelines/common/templates/wasm-coreclr-library-tests.yml |
Propagates R2R and trimming parameters. |
eng/liveILLink.targets |
Enables browser test trimming configuration. |
docs/workflow/testing/libraries/testing-wasm.md |
Documents local trimmed R2R testing. |
|
@AndyAyersMS are the AsyncProfilerTests expected to work on wasm, the session disabled them individually but I can make it blanket if preferred? |
I'm not sure what surface these cover. If they rely on the ResumeInfo.DiagnosticIP then they're not going to work now and possibly for quite some time. |
|
#133707 has a fix for the MethodBase.GetCurrentMethod failures currently marked ActiveIssue here |
) Fixes #133617. ## Problem The Wasm portable-entrypoint slow path retains a `PrestubMethodFrame` while invoking a resolved R2R body. Function-only stack walks can report both the real managed activation and that transition frame. `MethodBase.GetCurrentMethod` can therefore accept its own prestub frame as the caller. A Checked-runtime capture confirmed this: the stack marker was `0x4fe71c`; the real `GetCurrentMethod` body at SP `0x4fe710` did not pass the caller-marker comparison, but its explicit `PrestubMethodFrame` at SP `0x4fe720` did. The result was `GetCurrentMethod` instead of the calling method. This also reproduces without trimming. #133610 addressed the duplicate in `GetStackFramesCallback`, but caller-sensitive reflection uses a different callback and did not benefit from that filter. ## Change - Mark completed non-FCall Wasm prestub transitions as not representing an additional managed activation. - Filter those frames centrally for `FUNCTIONSONLY` walks, covering both ordinary and skipped explicit frames. - Retain the frame, MethodDesc, argument-root reporting, and physical unwind/exception lifetime. Popping early or clearing the MethodDesc would lose GC protection and the unwind bridge across interpreter-to-R2R transitions. - Replace #133610's StackTrace-specific adjacency filter. Filtering by lifecycle rather than matching method identities preserves genuine recursion. - Extend the existing `github60486` regression with direct/inlineable/generic caller identity, assembly lookup, recursive frame counts, and compacting-GC object/byref/struct argument coverage. Its original DIM frame-sequence and exception-stack checks remain intact. Native FCalls retain their existing representation. Non-Wasm filtering behavior is unchanged. ## Validation ### CoreCLR runtime regression Built browser-Wasm and macOS arm64 Checked runtimes with Release libraries. The expanded `Loader/classloader/DefaultInterfaceMethods/regressions/github60486` passes with fixed Wasm R2R, the interpreter, and native arm64. The identical final regression assembly fails against the saved original Wasm runtime: expected `ValidateCurrentMethod`, actual `GetCurrentMethod`. These fixed runs include the original regression checks from #133610, with its `debugdebugger.cpp` filter removed. ### Original browser library lane Validated the exact patch in an isolated checkout of #133656 at `282d12dfe220a1f34d0bb81a85239cab60c779ce`, which supplies the browser R2R publishing/test prerequisites. Built matched original/fixed CoreCLR Release products locally and ran Chromium 153. Temporarily enabled only the nine #133617-quarantined methods, preserving every original assertion and unrelated exclusion; all annotations were restored afterward. This PR contains no library-test quarantine changes. | Coverage | Original | Fixed | |---|---:|---:| | Cold trimmed direct GetCurrentMethod | 1 failed | 1 passed | | Cold trimmed inlineable GetCurrentMethod | 1 failed | 1 passed | | Trimmed targeted batch | 7 passed, 1 failed | 8 passed | | Untrimmed R2R targeted batch | 8 passed, 1 failed | 9 passed | | Full trimmed System.Runtime | 77,396 passed, 1 failed, 175 skipped | 77,397 passed, 0 failed, 175 skipped | The affected-class fixed run also passed: 23 passed, 3 existing skips. `CrossAssembly2` remains excluded when trimmed by dotnet/linker#2078 and passes untrimmed; it is not counted as a trimmed pass. Main test IL and main R2R images are byte-identical across each original/fixed pair; skip sets are identical. Served assets were verified against their respective rebuilt packs. This is a matched-product A/B, not a native-module-only swap: CoreLib R2R image bytes also differ. ### Existing reflection compatibility controls `System.Reflection.Tests` caller-assembly theory, real cross-assembly delegate invocation, static-constructor caller lookup, executing-assembly lookup, and reflected first/second invocation all pass: six cases on each original/fixed, trimmed/untrimmed browser R2R configuration. Fresh-process delegate and reflective-invocation probes also pass in all four configurations. All existing annotations remained intact. These are compatibility controls, not failing-before reproductions. Full browser reflection suites have identical original/fixed results: - Untrimmed: 1,750 passed, 1 failed, 23 skipped. - Trimmed: 1,747 passed, 2 failed, 23 skipped. Existing failures are `GetEntryAssembly` (browser host expectation versus R2R expectation) and, trimmed only, `AssemblyGetForwardedTypesLoadFailure` (underlying trimming cause not isolated). Failure signatures and skip sets match before/after; neither was suppressed. Native arm64 Checked reflection control: six focused cases passed; full suite 1,776 passed, 2 platform skips, zero failures. ## Remaining scope Draft for review of the frame-lifecycle distinction and FCall exception. Dedicated FCall cold/warm stack-identity coverage, a broader CoreCLR GC-stress campaign, and other browsers have not been run. No performance claim is made. > [!NOTE] > This change and pull request description were developed with GitHub Copilot assistance. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…d pipeline Implements the two modes: a dev-loop build stages the prebuilt framework R2R images from the runtime pack and ships the app as IL, while publish crossgens the whole closure per app, trimmed or untrimmed. The main correctness problems addressed: - Per-app R2R images are named <name>.wasm, but ComputeWasmPublishAssets classifies managed assemblies by the .dll extension, so the images were treated as native and leaked to the publish root, leaving the boot config with no coreAssembly. Restore the IL .dll in the publish list so ConvertDllsToWebcil stages the image from PrebuiltR2RDirectory. This has to happen in both the outer and nested passes: a native relink crossgens inside WasmNestedPublishApp, where ProcessPublishFilesForWasm is never scheduled, and _GatherWasmFilesToPublish filters to .dll, dropping every compiled assembly while exiting 0. - ILLink stamps PostprocessAssembly on its own collection rather than ResolvedFileToPublish on the Blazor/static-web-assets route, so the mainline compile list was empty and crossgen2 never ran. - A trimmed publish flow served the full copy-local set from the runtime pack mixed with the trimmed closure, which mixes version bubbles and lets an untrimmed assembly call a member ILLink removed from the trimmed framework. Restrict the served set to the linker output and repoint it there. - Flag flips left derived outputs behind. Static web assets are content-fingerprinted, so a re-stage adds a new name beside the old file instead of replacing it, leaving two copies of an assembly from two different version bubbles. Record the mode and drop the derived outputs when it changes. - Per-app crossgen inputs are deliberately conservative: cross-module inlining means any change must recompile every image, and stale images in obj/R2R are pruned. Composite and non-wasm container formats are rejected with a comprehensible error instead of producing images that fail at startup, and a missing crossgen2 is reported at the point of use. PublishReadyToRun defaults to false; flipping it belongs to the codegen-quality work stream.
The four relink triggers keyed solely on IsBrowserWasmProject, which a Blazor app leaves unset because it resolves the wasm RID late, so WasmBuildNative=true was a silent no-op there and the app shipped the prebuilt dotnet.native.wasm from the runtime pack. OR in WasmBuildNative, which is unambiguous: this file is imported only for CoreCLR browser-wasm apps. Kept as an OR so IsBrowserWasmProject, which also steers ICU and tzdata skipping, is never forced on. Fixes dotnet#133185
- ResolveReadyToRunCompilers override: drop the last-resort Crossgen2Path that named a possibly non-existent executable; when the in-build dir has no crossgen2 the tool is left to the SDK-pack fallback (and ultimately the actionable _WasmCoreClrValidateReadyToRun error) instead of failing with "file not found". - Use Update="@(...)" on the CopyToOutputDirectory metadata item groups so the metadata is stamped on the existing items rather than relying on bare metadata-only elements. - Revert the WasmBuildNative relink trigger OR: the relink is driven by IsBrowserWasmProject only, as before. dotnet#133185 does not reproduce on current SDKs and the OR was defensive. - Narrow the stale-mode served-assets cleanup to the fingerprinted *.wasm/*.dll we stage, so the SDK never recursively removes app-authored content under the served directory. - Quote the crossgen2 override path arguments passed by the Wasm.Build.Tests ReadyToRun opt-in.
_WasmCoreClrSuppressNestedPublishAssetCopy and _WasmCoreClrRestoreCopyToOutputDirectory only do anything when _WasmFrameworkCopyToOutputDirectory=PreserveNewest, which is set exclusively by eng/testing/tests.browser.targets so the xunit runner gets framework assets in bin/. For a real app the default is Never, making both a no-op, so they move out of the shipped WebAssembly SDK into the test infra, gated on RuntimeFlavor=CoreCLR. Also drop _WasmCoreClrInvalidateStaleModeOutputs, which deleted stale derived outputs on a mode flip. Incremental mode-flip cleanup will be reimplemented later without deleting files; every test/CI matrix cleans obj per case, so nothing exercised it.
Honor browser aggressive trimming and add a test-scoped ReadyToRun switch with matching build and Helix CI lanes. Preserve existing browser descriptors instead of importing Apple-only roots. Keep TestUtilities interpreted for the tracked platform-probe issue, preserve original conformance assertions, and quarantine known WebAssembly R2R failures with a shared browser/WASI predicate. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Remove the nine dotnet#133617 ActiveIssue annotations now that dotnet#133707 has landed. Preserve every original GetCurrentMethod call and assertion and retain unrelated platform and trimming exclusions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Decode StoreMulti callable addresses using the function-table-relative index used by resumption stubs. Add focused coverage, remove exclusions resolved by landed fixes, and retain only the three separately tracked V1 identity cases. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Deeper review showed that the VM-only translation could hide the function-table ordering defect through fallback instead of preserving native resume execution. Keep the independently validated exclusion updates while the compiler ordering fix is revised. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Place virtual-dispatch thunks after compiled methods and transition thunks so function-table-relative fixups remain aligned with RuntimeFunctions ordinals. Add coverage for async resumption targets interacting with virtual dispatch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The async resume ordering fix resolves the broad signature mismatch and EH failures, but RuntimeAsync_WhenAny_TracksAllBranches still reproduces the separate memory-OOB transition issue tracked by dotnet#133627. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
0eadf63 to
954ccfd
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The regression test does not assert that virtual-dispatch and StoreMulti fixups are present, so it may pass without covering the ordering interaction.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Require the interaction test to observe both StoreMulti fixups and a virtual-dispatch thunk before accepting the RuntimeFunctions ordering checks. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@davidwrighton @jkotas I can split the fix out of this if required but this would have to stack on top of it because so many things fail without the fix. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new R2R assertion computes the resume target as the parent’s final runtime-function index instead of the following generated resume stub.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
|
I think this need SDK flow ? @maraf @akoeplinger |




Summary
Addresses #133193 by enabling aggressively trimmed browser CoreCLR library tests and adding separate
LibraryTestsCoreCLR_R2RChrome CI lanes. Uses the existing browser trimming configuration and descriptors; no Apple-mobile descriptor imports are added.TestWasmReadyToRun, avoiding globalPublishReadyToRun=truereaching host-side build tools.TEST_READY_TO_RUN_MODE=1and usePlatformDetection.IsWasmReadyToRunfor browser/WASI-scoped quarantines, preserving interpreter coverage.TestUtilities.dllinterpreted for the open platform-probe issue; CoreLib, libraries, and test assemblies remain R2R..ilprojLink metadata from the TestILAssembly reference so its DLL is staged with the correct VFS name.WasmBuildNativeImplicitInReleaseConfiguration=false.Integrated prerequisites and fixes
The original browser R2R publishing prerequisite #133378 has landed on main and is incorporated by the main merges in this branch. Other landed fixes incorporated here include:
IsDynamicCodeCompiledtrimming correction.Function-table ordering fix
#133146 introduced virtual-dispatch thunks into the Wasm function table before compiled methods and transition thunks. Existing resumption fixups intentionally encode relative function-table indices and correlate them with RuntimeFunctions ordinals. The inserted table-only thunk shifted those spaces apart.
Nesm decoded the live failure before the fix:
The correct adjacent generated
<Resume>stub had the expected four-argument signature. The fix movesWasmVirtualDispatchThunkNodeafter compiled method and transition-thunk nodes, restoring the invariant without changing dispatch codegen or adding steady-state overhead. New compiler regression coverage validates both ResumptionStubEntryPoint and StoreMultiCallableAddrOfCode targets in the presence of virtual dispatch.Remaining quarantines
TestUtilities.dllstays interpreted; product/test assemblies remain R2R. Fix PR #133773 is still open.RuntimeAsync_WhenAny_TracksAllBranchesremains excluded for its independent memory-OOB transition failure.Local validation
macOS arm64 host, browser-wasm CoreCLR Release, Chromium 154.
Rebuilt Release tasks and matched browser CoreCLR, crossgen2, CoreLib, libraries, host, and packs after merging main and applying the ordering fix.
Focused fix validation supplied and independently integrated:
RuntimeAsync_CallstackEmittedOnResume: 1/1 passed.RuntimeAsync_YieldAtEachLevel_CallstackShrinks: 1/1 passed.WasmVirtualDispatch+RuntimeAsyncWasmDiagnosticIPFixups: 2/2 passed.System.Threading.Tests: 549 passed, 87 skipped, zero failed.System.Threading.Tasks.Testswith only [wasm][R2R] StateMachineAsync profiler callstack identities do not resolve #134145 and [browser][CoreCLR][R2R] RuntimeAsync_WhenAny_TracksAllBranches traps in an R2R-to-interpreter transition #133627 retained: 375 passed, 430 skipped, zero failed.<Resume>method with(i32,i32,i32,i32)->i32, proving native R2R resume execution rather than interpreter fallback.Current full smoke after the revised fix has completed successfully for System.Runtime, HTTP, WebSockets, JavaScript interop, and Threading; the initial Tasks run reached only the known #133627 failure. After restoring that one exclusion, the standalone full Tasks project passed as reported above.
The full supported R2R library selection is still running at the time of this update. Results will be added before the draft is marked ready.
Earlier descriptor-ablation comparisons ran full UnmanagedMemoryStream, FileSystem, Cryptography, Data.Common, Globalization, and LINQ suites in both actual EAT and trimmed R2R with the original browser roots: 136,138 passed, 1,141 skipped, zero failed per configuration, with identical counts. No Apple-only roots or replacement descriptors were required.
ActiveIssue-filtered cases are omitted from totals rather than counted as skipped. No performance claim is made; the ordering fix changes deterministic table placement only and has no steady-state dispatch cost.
Note
This PR description and changes were prepared with GitHub Copilot assistance.