Fix missing transition adapters for shared Wasm R2R unboxing stubs - #133516
Conversation
Restore managed U and UG unboxing thunks where interface and implementation ABIs differ, retain shared UM stubs, and publish precompiled unboxing code through portable entrypoints. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
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. |
|
This is obviously not ideal and the value over reverting the original pr is questionable. I'll try to fix the sharing model to fit the abi in the meantime |
|
Tagging subscribers to this area: @dotnet/crossgen-contrib |
Root the target R2R-to-interpreter adapter per managed signature. Keep shared U, UG, and UM stubs, but use the existing interpreted fallback when an exact incoming cookie or callable target adapter is unavailable. Remove the superseded managed-thunk fallback and cover both adapter dependencies. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
particular objection is no longer true |
Resolve GetUnboxingStub overlap with dotnet#133461 by retaining its STANDARD_VM_CONTRACT and void-pointer lookup while returning the target entrypoint checked by the adapter guard. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It changes CoreCLR VM behavior and R2R dependency rooting in a Wasm-specific path where subtle signature/entrypoint invariants can regress without a full human review and broader validation.
Review tier: Lite
Findings: None
What changed in this PR
This PR addresses browser-Wasm CoreCLR ReadyToRun failures involving shared unboxing stubs by ensuring the runtime only uses a shared unboxing stub when the necessary interpreter↔R2R transition thunks (and a callable portable entrypoint for the target) are available, otherwise falling back to the existing interpreted IL-stub path. It also updates Crossgen2 dependency rooting so those transition thunks are retained per target method, and extends Wasm R2R tests to cover a larger struct-return case.
Changes:
- Add runtime gating in
GetUnboxingStubto require a matching interpreter-to-R2R thunk for the unboxing signature and callable code in the target portable entrypoint before returning a shared stub. - Introduce a per-target dependency node (
WasmUnboxingStubTargetNode) so Crossgen2 roots the shared stub, the compiled target, and both transition thunk kinds (M*andI*) per managed target. - Extend Wasm generic-dispatch test coverage with a 56-byte struct return and validate the expected thunk keys are present in the produced R2R image.
| File | Description |
|---|---|
| src/coreclr/vm/wasm/helpers.cpp | Adds runtime checks to avoid publishing shared unboxing stub usage when required transition thunks / callable target entrypoints are missing. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs | Switches Wasm unboxing-stub rooting to a per-target node cache that pulls in both transition thunk dependencies. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/WasmUnboxingStubNode.cs | Adds WasmUnboxingStubTargetNode to express per-target dependency rooting for stub + target + transition thunks. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCases/VirtualMethodGenerics/NonGVM.cs | Adds a new 56-byte struct-return interface dispatch case to exercise distinct adapter rooting with shared stubs. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCases/R2RTestSuites.cs | Extends Wasm validation to assert presence of the expected M* and I* thunk keys for 16-byte and 56-byte struct return scenarios. |
|
This is approved it just needed a post approval conflict resolution so it needs another approval |
Fixes browser-Wasm CoreCLR ReadyToRun failures introduced after #133218 while preserving shared
U,UG, andUMunboxing stubs and almost all of their binary-size benefit.Corrected root-cause analysis
The initial version of this PR replaced
U/UGwith per-method managed thunks based on an ABI-mismatch hypothesis. That approach and its portable-prestub changes have been removed. The revised patch does not change the shared-stub calling conventions or lookup keys.NESM inspection stopped immediately before the failing Test23
call_indirect: the call expected fivei32parameters, but the selected table slot was zero (a null function reference). The expected parameter count alone did not establish aUversusUGmismatch. The observed delay-load fixup resolved to a non-unboxing shared target requiring a generic context, whose portable entrypoint lacked an R2R-to-interpreter adapter such asIvTiS1p.Replacing a compiled per-method unboxing thunk with a structural assembly stub lost dependencies previously introduced by compiling the thunk's managed body. There are two distinct transitions:
Madapter for the full managed unboxing signature, for exampleMS56Tp.Iadapter for the target signature, including its generic context where applicable, for exampleIS56TiporIvTiS1p, so its portable entrypoint is callable before the target's native body is published.Structural sharing also allows the runtime to find a shared stub for a generic instantiation Crossgen2 never compiled. A matching Wasm function shape does not imply that the exact managed transition cookie exists: different struct sizes can share the same structural stub but require different interpreter adapters. The actual
GitHub_19361execution exposed this case with a missingMS56Tpcookie.Changes
Iadapters through the existing call-site recording machinery.Evidence and validation
Browser-Wasm Release CoreCLR under Node.js 26.4.0, with explicitly regenerated and Wasm-validated CoreLib, LINQ, and test images:
LoaderClassloaderGenerics,DisplayName~genrecur.dllTest23 OK, matchingPassed testlineRegressions,DisplayName~genrecur.dllTest23 OK, matchingPassed testlineRegression_NoOptimize_r_1,Repro.Program.TestEntryPointStarting stress loop,Result: Completed Normally, matchingPassed testlineThe earlier
GitHub_19361name filter matched no test; earlier success claims using that filter were invalid. The results above use the actual fully qualified method name and confirm that the stress loop executed. Earlier stale CoreLib images were also replaced explicitly rather than relying on layout generation to refresh them.Additional validation:
ILCompiler.ReadyToRun.Tests: 75 passed, 37 target-inapplicable skips.I(target)fails on missingIS16Tip; removing onlyM(unboxing)fails on missingMS56Tp. Restoring both passes.The full CI outer-loop matrix and a browser-hosted run have not been rerun locally.
Binary size
Optimized browser
System.Private.CoreLibimages using--optimize --generate-unboxing-stubsin the local comparison:U/UGfallbackThe revised image adds 1,594 bytes over the earlier fully shared prototype and saves 394,333 bytes compared with the superseded fallback, retaining approximately 99.6% of the size reduction in that comparison. This supersedes the initial description's claim that the fix gives back nearly the entire saving.
Fixes #133491
Note
This pull request description and investigation summary were generated with GitHub Copilot.