[wasm][R2R] Fix generic context / async continuation order in Wasm interpreter thunks - #134676
Conversation
…thunks WasmLowering.RaiseSignature models the hidden generic context as explicit parameter 0, so ArgIterator placed it after the async continuation slot. The interpreter (and the VM ArgIterator) expect the generic context before the async continuation, so the R2R->interpreter and interpreter->R2R thunks swapped the two arguments for shared generic async methods. This caused the interpreter to see a MethodDesc in the GC-reported continuation slot (SanityCheck() failures in AsyncHelpers.Await) and a null/garbage generic context (numGenericArgs > 0 assert in dictionary lookups). Fixes #133953 Fixes #134660 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Replace the per-thunk context/continuation offset swap with a shared GCRefMapBuilder.BuildWasmThunkArgIterator helper. When a generic context precedes the async continuation, it drops the context from the raised layout signature and builds the ArgIterator with methodRequiresInstArg, so the context is stored and loaded through GetParamTypeArgOffset() and precedes the continuation, matching the interpreter and the callee's GC ref map. This also fixes WasmImportThunk, which spilled the two arguments in the swapped order during delay-load fixups, so the GC ref map would report the generic context slot as an object reference and miss the continuation. HasGenericContextBeforeAsync moves to WasmLowering instead of being duplicated in each thunk. Add WasmArgumentLayoutTests coverage for the thunk layout, and re-enable RuntimeAsync_WhenAny_TracksAllBranches, which was disabled for the same root cause. Fixes #133627 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
RaiseSignature re-implemented the check for a generic context that precedes the async continuation. Call WasmLowering.HasGenericContextBeforeAsync instead so the signature encoding is parsed in one place. 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. |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Tagging subscribers to this area: @dotnet/crossgen-contrib |
|
@AndyAyersMS I hope this isn't duplicating anything, go ahead and reject this if you have a fix in progress. |
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
Nope, I hadn't looked at fixing this yet. |
|
@lewing, getting the wrong GC Ref map is a serious problem. Please dont' comment that it isn't understood, please fix instead. |
The delay-load GC ref map for a call is computed from the callee's MethodDesc, while WasmImportThunk spills the arguments using only the callee's Wasm signature. Factor GetCallRefMap's ArgIterator construction into BuildCallRefMapArgIterator and add a test asserting both produce the same generic context, async continuation and argument offsets for shared generic, async and shared generic async CoreLib methods. Against the previous thunk layout, the shared generic async cases fail with the generic context one slot away from where the GC ref map reports it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Sorry, that was badly worded. The GC ref map mismatch is fixed in this PR, not left open. bd8aa3c moves the Note This comment was drafted with GitHub Copilot. |
The hidden generic context is always passed immediately before the async continuation (clr-abi.md, "Passing Continuation argument"). Since it is encoded like any pointer-sized argument, detect it as the pointer char immediately preceding the 'a' continuation marker instead of re-parsing the return type and 'this'. Reword the comments that described the context as "explicit parameter 0": that referred to RaiseSignature's MethodSignature parameter list, not the ABI position. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Replace the multi-line remarks on BuildWasmThunkArgIterator and HasGenericContextBeforeAsync with one-line summaries and a short inline comment where the code isn't self-explanatory; the ABI ordering is already documented in clr-abi.md. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add WasmThunkArgLayout, which walks a managed Wasm signature string in Wasm parameter order and maps each element (this, retbuf, generic context, async continuation, explicit arguments) to its Wasm parameters and its ArgIterator offset. The R2R-to-interpreter, interpreter-to-R2R, and import thunks now each handle their arguments with a single loop over the layout instead of hand-coding the hidden argument sequence. Move BuildWasmThunkArgIterator from GCRefMapBuilder into the new type. The refactor produces byte-identical crossgen2 output. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/azp run runtime-extra-platforms |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
- Remove the unused WasmThunkArgLayout.Signature, WasmThunkArg.Type and RetBufParamIndex; make BuildArgIterator private. - Move the typed load/store helpers to Memory.Load/Store in WasmInstructions and use them for the thunk return values as well. - Drop the stale spill pseudocode in WasmImportThunk and note when a generic context is a hidden argument. - Fold the ArgIterator-level thunk layout tests into the layout theories. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/azp run runtime-wasm-libtests |
|
No pipelines are associated with this pull request. |
…async Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Encode the hidden generic context as 'g' in managed Wasm signature strings instead of the pointer-sized type code, so it no longer has to be inferred from its position before an async continuation. - WasmLowering.GetSignature emits 'g'; RaiseSignature drops it like the continuation and GetHiddenArgumentFlags reports both, so the roundtrip is exact without reordering. HasGenericContextBeforeAsync is removed. - WasmThunkArgLayout lays out the context as the hidden instantiation argument whenever the token is present, async or not. - Thunk nodes report the context through HasGenericContextArg so their Wasm function types are unchanged. - The VM's semantic GetSignatureKey emits 'g'; the Wasm calling-convention keys keep the pointer type code. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Match the other signature tokens ('T', 'a', 'p') and the VM key builder.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
With the dedicated 'g' token, RaiseSignature no longer returns the generic context as a parameter, so lowering the raised signature without hidden flags dropped it and tripped the FuncType assert. Delegate Invoke never takes a generic context, so treat 'g' like 'a' and don't create the adapter. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This reverts 8c611cc, 4ac464b and 56b87a7. Without an async continuation the generic context occupies the same slot as a leading pointer argument, and runtime code relies on that: InitHelpers.CallClassConstructor calls a shared generic cctor as delegate*<void*, void>. With 'g' the VM looked up the cctor's thunk under Ivgp while R2R code only registered Ivip, so the portable entry point got no interpreter thunk and browser R2R startup failed. Emitting 'g' only before 'a' would carry no more information than the pointer char in that position, so go back to the previous encoding. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ument InitHelpers.CallClassConstructor calls a shared generic class constructor as delegate*<void*, void>, so without an async continuation the context must lower to the same thunk key as a leading pointer argument. Document the invariant in readytorun-format.md. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Is this ready to go and just need a re-review? |
|
Yes, it's ready for a re-review. CI on be631a8 is green: Build Analysis passes, the only failure is a known MsQuic timeout, and the browser R2R smoke lane passes. Since the last review I folded in a dedicated generic-context token and then reverted it, because it broke shared generic cctors called through @jkotas @davidwrighton, could you take another look? Note This comment was drafted with GitHub Copilot. |
…all (#134826) On browser CoreCLR, `JIT/Directed/callconv/{Cdecl,StdCall,PlatformDefault}MemberFunction` and `ThisCall` fail with a `GetCookieForCalliSig: unknown thunk signature` assert. The MemberFunction tests print `WASM calli missing for key: MS8ii`, which comes from `delegate* unmanaged[Cdecl, MemberFunction]<C*, int, SizeF>`. That signature only shows up as a `calli`, so the thunk generator never sees it. ThisCall gets rejected by the callconv switch before a key is even built. #133571 disabled these tests on browser. This PR fixes the cause and re-enables all four tests. ## Changes - `PortableCallHelpers/PInvokeCollector.cs`: the new `CollectUnmanagedCalliSignatures` scans IL for `calli` through unmanaged function pointers. It adds their signatures to the interp-to-native thunk table. Managed and varargs calli, generic-shaped signatures, and signatures that can't be lowered are skipped, with a Verbose log. - `JitInterface/WasmLowering.cs`: `GetSignature` treated `UnmanagedCallingConvention` (`0x9`) as a bit flag, so Cdecl, StdCall and ThisCall signatures (`0x1`–`0x3`) were lowered as managed. It now checks the masked calling convention, and the collector no longer has to pass `IsUnmanagedCallersOnly` to compensate. - `vm/wasm/helpers.cpp`: `ComputeCalliSigThunk` now accepts `IMAGE_CEE_CS_CALLCONV_THISCALL`. - `PortableCallHelpers/PInvokeTableGenerator.cs`: when a reverse thunk returns a struct that the wasm C ABI returns by reference, the thunk now takes the hidden leading `sret` pointer and hands it to the interpreter as the return buffer. This applies to `[UnmanagedCallersOnly]` wrappers and exports. Before, it was declared as returning `void *`. Native code calling `GetSize` through the vtable (`SizeF` return) then trapped with `function signature mismatch`, and the interpreter would have written 8 bytes into a 4-byte local. The P/Invoke declaration path already handled this. This also removes the rejection of callbacks that return through a hidden buffer, which #134355 added because the wrapper didn't model that ABI yet (cc @pavelsavara). The export-only R2R dispatch from #134355 forwards `sret` the same way. - `WasmArgumentLayoutTests.cs`: new tests `PortableCallHelpersGeneratorEmitsThunksForUnmanagedCalliSites`, `PortableCallHelpersGeneratorReturnsStructsThroughHiddenPointerInReverseThunks` and `SignatureCallingConventionSelectsLowering`. - `src/tests/JIT/Directed/callconv/`: reverts the `ActiveIssue`, `WasmBuildTestCorerun=false`, and `CLRTestTargetUnsupported` disables that #133571 added. The files now match their state before #133571. The exception is `ThisCallTest.cs`: its `Marshal.GetFunctionPointerForDelegate` reverse cases are now skipped on wasm. CoreCLR wasm can't allocate the stub those need at run time, and it isn't reliable on Mono either (#104391). Its forward and `UnmanagedCallersOnly` cases still run there. ## Validation - I ran the patched crossgen2 with `--generate-portable-callhelpers` over the Helix payloads of the three MemberFunction tests. Each one now emits `S8ii` from `Test8ByteHFA`. - ThisCall already had `S8ii` through an `UnmanagedFunctionPointer` delegate, so only the runtime switch change matters for it. - The new unit test fails without the collector change and passes with it. All 68 `WasmArgumentLayoutTests` pass. They ran against a browser CoreLib/libs layout staged from the Helix correlation payload. - After the first CI run, all three MemberFunction tests trapped with `function signature mismatch` in `Test8ByteHFAUnmanagedCallersOnly`. I reproduced that locally with the Helix payload. The native `call_indirect` expects `(i32, i32, i32) -> void`. After the reverse-thunk fix, the regenerated `GetSize` thunk is `void(void* sret, void*, int32_t)`. The new unit test covers this, and all 69 `WasmArgumentLayoutTests` pass. - End to end: I did a local browser build (`./build.sh -os browser -c Debug -subset clr+libs`, which compiles the `helpers.cpp` change) and built `JIT/Directed/callconv` with `src/tests/build.sh -browser Debug`. All four tests exit 100 under node. I reran this after merging main, which brought in #134355, #134676 and #134690. All 112 `ILCompiler.ReadyToRun.Tests` in `WasmArgumentLayoutTests` pass, and so do the four callconv tests. - After the `WasmLowering` fix, all 88 `WasmArgumentLayoutTests` pass. The rebuilt crossgen2 regenerates call helpers for the four callconv tests that are byte-identical to the ones linked in the end-to-end run. ## Note on checked-in tables The checked-in `src/coreclr/vm/wasm/{browser,wasi}/callhelpers-*.cpp` tables may now be missing framework calli signatures until they are regenerated with `generate-coreclr-helpers.sh`. A missing entry is not a broken one. They were not regenerated in this PR. > [!NOTE] > This PR description was generated with AI assistance (GitHub Copilot). --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Radek Doulik <radek.doulik@gmail.com> Co-authored-by: Jan Kotas <jkotas@microsoft.com>
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.RaiseSignaturehas no way to mark a parameter as the hidden generic context, so it returns it as the first entry of theMethodSignatureparameter list, withthisand the return buffer implied by the signature flags and return type. The thunks built theirArgIteratorfrom that signature withoutmethodRequiresInstArg, soArgIteratorlaid the frame out as[this][continuation][ctx][args]. The interpreter, the VMArgIteratorand 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:
SanityCheck()in interpretedAsyncHelpers.Await<__Canon>(ValueTask<T>)(R2R→interpreter).numGenericArgs > 0inProcessDynamicDictionaryLookupfor a generic virtual async method.WasmR2RToInterpreterThunk(iiaS8p)(AwaitAwaiter<TAwaiter>).[BypassReadyToRun]fromAsyncHelpers) hits the interpreter→R2R direction of the same bug on both the R2R and non-R2R browser legs.Fix
ArgIteratorwithmethodRequiresInstArg: true. The context is then stored and loaded throughGetParamTypeArgOffset(), and the continuation throughGetAsyncContinuationArgOffset(). Without an async continuation the context occupies the same slot as a leading pointer argument, so it keeps the pointer encoding (i/l).InitHelpers.CallClassConstructorrelies on that when it calls a shared generic class constructor asdelegate*<void*, void>.WasmR2RToInterpreterThunkNode,WasmInterpreterToR2RThunkNodeandWasmImportThunk. Each used to hand-code the hidden-argument sequence, which is how the slots got swapped.WasmThunkArgLayoutwalks the Wasm signature string in Wasm parameter order,[this] [retbuf] [generic context] [continuation] [args]per clr-abi.md, takes each offset from thatArgIterator, and asserts that the two agree. Each thunk keeps its own emission and loops over the entries.WasmImportThunk, the swapped spill disagreed with the delay-load GC ref map, whichGCRefMapNodebuilds from the callee'sMethodDescviaGCRefMapBuilder.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, andWasmThunkArgLayoutMatchesCallRefMapLayoutasserts the two layouts are identical.HasGenericContextBeforeAsyncintoWasmLowering, and use it fromRaiseSignatureso the encoding is parsed in one place.Testing
run_test_p0_coreclr_R2R_CG2_browser_wasm_checked(theasyncwork item), with an osx-arm64 crossgen2 and wasm JIT.asyncwork item passes 132/132 in both R2R (RunCrossGen2=1, no tiered compilation) and non-R2R modes. Without it, both failures reproduce.WasmArgumentLayoutTestscases assert the kind, offset and Wasm parameter index of every thunk argument foriiaip,iTiaip,S16iaipandS16Tiaip, the no-context cases, and multi-slot and by-reference arguments.GenericContextEncodesAsLeadingPointerArgumentpins theCallClassConstructorinvariant: without an async continuation, a context and a leading pointer argument lower to the same key.WasmArgumentLayoutTestspasses 98/98 locally with a browser-wasm target.WasmThunkArgLayoutMatchesCallRefMapLayoutcompares every slot of the thunk layout with theArgIteratorthatGetCallRefMapuses, for shared generic, async and shared generic async CoreLib methods.asyncwork item (--parallelism:1), and theasyncwork item passes 132/132 in R2R mode.AsyncProfilerTests.RuntimeAsync_WhenAny_TracksAllBranches([browser][CoreCLR][R2R] RuntimeAsync_WhenAny_TracksAllBranches traps in an R2R-to-interpreter transition #133627). It runs only in the full browser CoreCLR R2R library lane, which PRs can't reach today:runtime-extra-platformsadds Wasm jobs only for scheduled builds, andruntime-wasm-libtestsis disabled in AzDO. It hasn't run in CI for this PR. The first scheduledruntime-extra-platformsbuild after merge will run it; Fix scheduled WASM extra-platforms job selection #134613 restored that lane.Notes
HasGenericContextBeforeAsyncproperties, so whichever lands second will have a small textual conflict.WasmImportThunkis 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.gtoken for the context ([wasm][R2R] Give the hidden generic context its own token in Wasm signature strings #134716) was folded in and then reverted. The VM computedIvgpfor a shared generic class constructor while R2R code registered onlyIvipfromCallClassConstructor, so the portable entry point got no interpreter thunk and every app in the browser R2R smoke lane failed at startup. Emittinggonly beforeawould 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.