Wasm import thunk corrections#126973
Merged
davidwrighton merged 8 commits intodotnet:mainfrom Apr 20, 2026
Merged
Conversation
…rator to match the ArgIterator behavior in the runtime - Notably, the ArgIterator in the runtime follows interpreter semantics, so this logic adjusts the way we stash memory into the stack to match the interpreter behavior - And the WasmImportThunk code now utilizes the ArgIterator to do its layout. - We may need to reconcile behavior differences between the interpreter/RyuJIT abi, but they are fairly close, so this is probably reasonable to do for now. Alternatively, we can remove the whole GCRefMap infra in favor of a small bit of conservative reporting, and a completely different format.
- Move the save/restore adjustment to the actual helper logic in the runtime
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates WASM ReadyToRun import thunk plumbing to better match the interpreter calling convention and the VM’s TransitionBlock expectations, enabling DelayLoad_MethodCall to be implemented on WASM rather than stubbed out.
Changes:
- Implemented
DelayLoad_MethodCallfor WASM using a naked wrapper that temporarily adjusts__stack_pointerand delegates to a C++ implementation. - Reworked
WasmImportThunkto compute argument save/restore offsets viaArgIterator/TransitionBlock, and to pass module-fixup RVA into the helper. - Updated WASM32 calling convention modeling in the R2R compiler (
TransitionBlock,ArgIterator) and refactoredGCRefMapBuilderto share ArgIterator construction.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/wasm/helpers.cpp | Adds WASM implementation of DelayLoad_MethodCall and helper implementation function. |
| src/coreclr/vm/cgensys.h | Adjusts DelayLoad_MethodCall declaration for WASM to match the new C++ signature/return type. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/WasmImportThunk.cs | Rewrites thunk emission to use TransitionBlock/ArgIterator offsets and pass module-fixup RVA; updates helper dispatch load. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TransitionBlock.cs | Updates WASM32 transition block sizing/alignment assumptions for interpreter slot sizing. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/GCRefMapBuilder.cs | Factors ArgIterator construction into a reusable helper used by both GC map building and WASM thunk emission. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ArgIterator.cs | Updates WASM32 argument sizing/alignment and initial stack offset calculations toward interpreter slot conventions. |
Add copilot suggestions Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…init - Align total allocation (args + transition block) to 16 bytes instead of only aligning the args portion - Use interpreter stack slot size (8) instead of pointer size (4) for hidden argument offset calculations in ArgIterator - Initialize m_ReturnAddress to 0 and store caller frame pointer in the transition block header Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Replace magic constant 8 with StackElemSize(PointerSize) for Wasm32 initial stack offset - Use StackElemSize for byref arg stack size in SizeOfFrameArgumentArray - Align arg stack size to slot size instead of pointer size Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
adamperlin
reviewed
Apr 17, 2026
Contributor
|
Overall this looks good to me, just a few questions + suggestions for comment clarifications. |
jkotas
reviewed
Apr 17, 2026
jkotas
reviewed
Apr 17, 2026
Member
Author
|
@jkotas @adamperlin I believe all feedback has been addressed, and build analysis is happy. Could one of you signoff? |
adamperlin
approved these changes
Apr 20, 2026
Contributor
adamperlin
left a comment
adamperlin
left a comment
There was a problem hiding this comment.
I think this looks good to me!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DelayLoad_MethodCallfunctionNOTE: We have not yet handled all of the variation between the interpreter and R2R calling conventions, but this is close enough to make good progress.