Introduce Wasm-level signature nodes - #134211
jtschuster with Copilot wants to merge 6 commits into
Conversation
Co-authored-by: jtschuster <36744439+jtschuster@users.noreply.github.com>
Co-authored-by: jtschuster <36744439+jtschuster@users.noreply.github.com>
Co-authored-by: jtschuster <36744439+jtschuster@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. |
|
Tagging subscribers to this area: @dotnet/crossgen-contrib |
Co-authored-by: jtschuster <36744439+jtschuster@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
One or more issues must be addressed before approval.
Review effort: Lite
Findings: None
What changed in this PR
Introduces a Wasm-specific signature abstraction across compiler dependency analysis and object emission, separating lowered signatures from managed method signatures.
Changes:
- Adds
INodeWithWasmSignaturewith compatibility lowering for existing nodes. - Updates Wasm writers, factories, dependencies, and thunk nodes.
- Removes unnecessary signature raise/lower conversions.
| File | Description |
|---|---|
| src/coreclr/tools/Common/Compiler/ObjectWriter/WasmObjectWriter.cs | Updated as part of this pull request. |
| src/coreclr/tools/Common/Compiler/ObjectWriter/ObjectWriter.cs | Updated as part of this pull request. |
| src/coreclr/tools/Common/Compiler/DependencyAnalysis/ObjectNode.cs | Updated as part of this pull request. |
| src/coreclr/tools/Common/Compiler/DependencyAnalysis/INodeWithTypeSignature.cs | Updated as part of this pull request. |
| src/coreclr/tools/Common/Compiler/DependencyAnalysis/AssemblyStubNode.cs | Updated as part of this pull request. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs | Updated as part of this pull request. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/WasmVirtualDispatchThunkNode.cs | Updated as part of this pull request. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/WasmUnboxingStubNode.cs | Updated as part of this pull request. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/WasmR2RToInterpreterThunkNode.cs | Updated as part of this pull request. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/WasmInterpreterToR2RThunkNode.cs | Updated as part of this pull request. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/WasmImportThunk.cs | Updated as part of this pull request. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_Wasm/WasmReadyToRunHelperNode.cs | Updated as part of this pull request. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_Wasm/WasmJumpStubNode.cs | Updated as part of this pull request. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs | Updated as part of this pull request. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/JumpStubNode.cs | Updated as part of this pull request. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/AddressTakenMethodNode.cs | Updated as part of this pull request. |
What would be the problems with doing that? It is what we do for the ReadyToRun helpers today and I don't see it as problematic. This is adding a new concept. The |
It's not a blocker for anything, I just thought it's unnecessary to lower a signature to wasm if we don't need the non-wasm signature. Since there's a potential use for the non-wasm MethodSignature, I'm fine closing this and constructing MethodSignatures for ExternFunctionSymbolNodes. |
It's unnecessary the same way how it's unnecessary in the ReadyToRun helpers. It feels like a premature optimization (especially if the motivation is ExternFunctionSymbolNode since we only have a handful of them). I would keep the number of concepts (interfaces) lower for now. |
…terpreter thunks (#134676) ## Problem For shared-generic runtime-async methods, crossgen2's Wasm thunks passed the hidden generic context and the async continuation in each other's slots. `WasmLowering.RaiseSignature` has no way to mark a parameter as the hidden generic context, so it returns it as the first entry of the `MethodSignature` parameter list, with `this` and the return buffer implied by the signature flags and return type. The thunks built their `ArgIterator` from that signature without `methodRequiresInstArg`, so `ArgIterator` laid the frame out as `[this][continuation][ctx][args]`. The interpreter, the VM `ArgIterator` and the callee's GC ref map (`GCRefMapBuilder.GetCallRefMap`) all expect `[this][ctx][continuation][args]`. Andy diagnosed this in #133627 and proposed modeling the context as the hidden instantiation argument; this PR follows that suggestion. Symptoms: - #133953: `SanityCheck()` in interpreted `AsyncHelpers.Await<__Canon>(ValueTask<T>)` (R2R→interpreter). - #134660: `numGenericArgs > 0` in `ProcessDynamicDictionaryLookup` for a generic virtual async method. - #133627: memory access out of bounds in `WasmR2RToInterpreterThunk(iiaS8p)` (`AwaitAwaiter<TAwaiter>`). - #128626 (removing `[BypassReadyToRun]` from `AsyncHelpers`) hits the interpreter→R2R direction of the same bug on both the R2R and non-R2R browser legs. ## Fix - When a generic context precedes the async continuation, drop it from the layout signature and build the `ArgIterator` with `methodRequiresInstArg: true`. The context is then stored and loaded through `GetParamTypeArgOffset()`, and the continuation through `GetAsyncContinuationArgOffset()`. Without an async continuation the context occupies the same slot as a leading pointer argument, so it keeps the pointer encoding (`i`/`l`). `InitHelpers.CallClassConstructor` relies on that when it calls a shared generic class constructor as `delegate*<void*, void>`. - Share one argument layout across the three thunks that spill arguments: `WasmR2RToInterpreterThunkNode`, `WasmInterpreterToR2RThunkNode` and `WasmImportThunk`. Each used to hand-code the hidden-argument sequence, which is how the slots got swapped. `WasmThunkArgLayout` walks the Wasm signature string in Wasm parameter order, `[this] [retbuf] [generic context] [continuation] [args]` per [clr-abi.md](https://github.com/dotnet/runtime/blob/main/docs/design/coreclr/botr/clr-abi.md#passing-continuation-argument), takes each offset from that `ArgIterator`, and asserts that the two agree. Each thunk keeps its own emission and loops over the entries. - For `WasmImportThunk`, the swapped spill disagreed with the delay-load GC ref map, which `GCRefMapNode` builds from the callee's `MethodDesc` via `GCRefMapBuilder.GetCallRefMap`. A GC during the fixup would have reported the generic context slot as an object reference and missed the continuation. The thunk now spills both to the offsets the GC ref map describes, and `WasmThunkArgLayoutMatchesCallRefMapLayout` asserts the two layouts are identical. - Move the three copies of `HasGenericContextBeforeAsync` into `WasmLowering`, and use it from `RaiseSignature` so the encoding is parsed in one place. ## Testing - Reproduced the #133953 and #134660 failures locally from the CI Helix payload of `run_test_p0_coreclr_R2R_CG2_browser_wasm_checked` (the `async` work item), with an osx-arm64 crossgen2 and wasm JIT. - With this change, the full `async` work item passes 132/132 in both R2R (`RunCrossGen2=1`, no tiered compilation) and non-R2R modes. Without it, both failures reproduce. - #128626's diff on top of this change also passes 132/132 in both modes. - New `WasmArgumentLayoutTests` cases assert the kind, offset and Wasm parameter index of every thunk argument for `iiaip`, `iTiaip`, `S16iaip` and `S16Tiaip`, the no-context cases, and multi-slot and by-reference arguments. `GenericContextEncodesAsLeadingPointerArgument` pins the `CallClassConstructor` invariant: without an async continuation, a context and a leading pointer argument lower to the same key. `WasmArgumentLayoutTests` passes 98/98 locally with a browser-wasm target. - `WasmThunkArgLayoutMatchesCallRefMapLayout` compares every slot of the thunk layout with the `ArgIterator` that `GetCallRefMap` uses, for shared generic, async and shared generic async CoreLib methods. - The shared layout produces byte-identical crossgen2 output to the previous head of this PR (4e66ef0) for System.Private.CoreLib and the 78 assemblies in the `async` work item (`--parallelism:1`), and the `async` work item passes 132/132 in R2R mode. - Re-enabled `AsyncProfilerTests.RuntimeAsync_WhenAny_TracksAllBranches` (#133627). It runs only in the full browser CoreCLR R2R library lane, which PRs can't reach today: `runtime-extra-platforms` adds Wasm jobs only for scheduled builds, and `runtime-wasm-libtests` is disabled in AzDO. It hasn't run in CI for this PR. The first scheduled `runtime-extra-platforms` build after merge will run it; #134613 restored that lane. ## Notes - #134643 adds ActiveIssues for #133953 and #134660; those shouldn't be needed with this change. - #134211 touches the members right above the removed `HasGenericContextBeforeAsync` properties, so whichever lands second will have a small textual conflict. - The two interpreter transition thunks are shared across images by signature string, first registration wins (`WasmImportThunk` is not; each image references its own). An image compiled by an older crossgen2 would still carry the old layout under the same key. That only matters for mixing crossgen2 versions, which Wasm R2R doesn't ship yet. - A dedicated `g` token for the context (#134716) was folded in and then reverted. The VM computed `Ivgp` for a shared generic class constructor while R2R code registered only `Ivip` from `CallClassConstructor`, so the portable entry point got no interpreter thunk and every app in the browser R2R smoke lane failed at startup. Emitting `g` only before `a` would carry no more information than the pointer char in that position. cc @AndyAyersMS @davidwrighton Resolves #133953 Resolves #134660 Resolves #133627 > [!NOTE] > This PR description was drafted with GitHub Copilot. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Decouple the concept of providing a WASM signature and having a managed signature and lowering flags. This enables ExternFunctionSymbolNodes to eventually only provide their WasmSignature without an intermediate MethodSignature.
Changes
INodeWithWasmSignaturefor final lowered signatures.WasmLowering.JumpStubNodehierarchy without broadening external symbols.