[wasm][R2R] Fix generic context / async continuation order in Wasm interpreter thunks

[lewing]

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:

• Async2TaskAdapters.TestValueTaskOfTaskValueVersusAsync fails with SanityCheck() in Wasm R2R test #133953: SanityCheck() in interpreted AsyncHelpers.Await<__Canon>(ValueTask<T>) (R2R→interpreter).
• Wasm R2R test failure: GenericVirtualMethod.Program.TestGenericVirtualMethod #134660: numGenericArgs > 0 in ProcessDynamicDictionaryLookup for a generic virtual async method.
• [browser][CoreCLR][R2R] RuntimeAsync_WhenAny_TracksAllBranches traps in an R2R-to-interpreter transition #133627: memory access out of bounds in WasmR2RToInterpreterThunk(iiaS8p) (AwaitAwaiter<TAwaiter>).
• Remove BypassReadyToRun attributes from AsyncHelpers #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().
• 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, 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.
• RaiseSignature's output is unchanged: the direct-forwarding thunks and INodeWithTypeSignature.Signature still get the context as the first entry of the parameter list.

Testing

• Reproduced the Async2TaskAdapters.TestValueTaskOfTaskValueVersusAsync fails with SanityCheck() in Wasm R2R test #133953 and Wasm R2R test failure: GenericVirtualMethod.Program.TestGenericVirtualMethod #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.
• Remove BypassReadyToRun attributes from AsyncHelpers #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. WasmArgumentLayoutTests passes 93/93 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 ([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-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; Fix scheduled WASM extra-platforms job selection #134613 restored that lane.

Notes

• Disable async tests on Wasm R2R #134643 adds ActiveIssues for Async2TaskAdapters.TestValueTaskOfTaskValueVersusAsync fails with SanityCheck() in Wasm R2R test #133953 and Wasm R2R test failure: GenericVirtualMethod.Program.TestGenericVirtualMethod #134660; those shouldn't be needed with this change.
• Introduce Wasm-level signature nodes #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.

cc @AndyAyersMS @davidwrighton

Resolves #133953
Resolves #134660
Resolves #133627

Note

This PR description was drafted with GitHub Copilot.

[azure-pipelines]

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.

[lewing]

/azp run runtime-extra-platforms

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/crossgen-contrib
See info in area-owners.md if you want to be subscribed.

[lewing]

@AndyAyersMS I hope this isn't duplicating anything, go ahead and reject this if you have a fix in progress.

[dotnet-policy-service]

Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara
See info in area-owners.md if you want to be subscribed.

[AndyAyersMS]

@AndyAyersMS I hope this isn't duplicating anything, go ahead and reject this if you have a fix in progress.

Nope, I hadn't looked at fixing this yet.

[davidwrighton]

@lewing, getting the wrong GC Ref map is a serious problem. Please dont' comment that it isn't understood, please fix instead.

[lewing]

Sorry, that was badly worded. The GC ref map mismatch is fixed in this PR, not left open. WasmImportThunk now builds its spill layout with the same ArgIterator flags GCRefMapBuilder.GetCallRefMap uses for the callee. The generic context is spilled at GetParamTypeArgOffset() and the continuation at GetAsyncContinuationArgOffset(), which are the slots the delay-load GC ref map reports as the instantiation parameter and as a GC reference. "Haven't reproduced" only meant that no test forces a GC during the fixup.

bd8aa3c moves the ArgIterator construction from GetCallRefMap into a shared helper and adds WasmThunkArgLayoutMatchesCallRefMapLayout. It asserts that the thunk and GC ref map layouts are identical for shared generic, async and shared generic async methods, and it fails against the old layout. I've also updated the description.

Note

This comment was drafted with GitHub Copilot.

[lewing]

/azp run runtime-extra-platforms

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[lewing]

/azp run runtime-wasm-libtests

[azure-pipelines]

No pipelines are associated with this pull request.

[jkotas]

Can this comment also explain why this special casing of async continuation + generic context is required (or link to where it is explained)? I am not able to figure it out, and it seems to be a bug farm. What would need to change to get rid of this special casing?

[lewing]

The Wasm signature string has no distinct token for the hidden generic context. WasmLowering.GetSignature emits it as the pointer char (i on wasm32), the same as an explicit pointer argument, and the thunks only have that string, since they're shared across methods by signature. So the layout can only recognize the context by position, when the a continuation follows it. Without a continuation it doesn't matter: GetParamTypeArgOffset() is the same slot as the first argument, so the layout is identical either way.

Removing the special case needs a dedicated token for the generic context, emitted by GetSignature and the VM's GetSignatureKey (vm/wasm/helpers.cpp), and understood by RaiseSignature, the portable call-helper tokenizer and readytorun-format.md. The layout would then use the hidden instantiation argument whenever the token is present. The cost is that signatures differing only in context versus an explicit pointer argument would no longer share a thunk. Since that changes the thunk key format on both the VM and crossgen2 sides, I'd prefer to do it as a follow-up: #134716. For now f119a1d expands this comment to say why the special case exists.

Note

This comment was drafted with GitHub Copilot.

[lewing]

@jkotas it is definitely a source of bugs as demonstrated in this pr which reconstructs the requirement from the rest of signature and fixes the existing thunks. If this is the time to make the signature string encode the generic context distinctly I'm happy to do it here even if the agent is overly focused on our repo instructions to separate concerns.

[lewing]

@jkotas here's what the dedicated token looks like: prototype branch. It isn't part of this PR yet.

• The generic context gets its own signature char, g, in both crossgen2 (WasmLowering.GetSignature) and the VM key (GetSignatureKey in vm/wasm/helpers.cpp). Wasm function types are unchanged.
• HasGenericContextBeforeAsync, the RaiseSignature roundtrip swap, and the async-only case in WasmThunkArgLayout are gone. The layout treats the context as the hidden instantiation argument whenever g is present. RaiseSignature reports it as a flag, like the continuation, instead of as parameter 0.
• 9 files, +82/−69. WasmArgumentLayoutTests passes 94/94. With a rebuilt browser runtime, the R2R async runtime tests pass 132/132, and a logging build confirmed the lookups hit the new g keys.
• Every thunk body is byte-identical to before; the cost is lost sharing. A method with a generic context and one with an explicit pointer argument used to share a thunk, so CoreLib gains 232 duplicate thunks (about 20 KB). Registering one body under both keys would recover most of that, but StringDiscoverableAssemblyStubNode would need to support more than one lookup string.

I'm happy to fold this into this PR or land it separately as #134716. Which do you prefer?

Note

This comment was drafted with GitHub Copilot.