Uh oh!
There was an error while loading. Please reload this page.
Implement TentativeMethodNode for WASM - #132814
Conversation
|
Azure Pipelines: 16 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Co-authored-by: jtschuster <36744439+jtschuster@users.noreply.github.com>
Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib |
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. |
There was a problem hiding this comment.
Pull request overview
This PR implements WebAssembly (Wasm) codegen support for TentativeMethodNode in the NativeAOT compiler pipeline, enabling emission of tentative method stubs and ensuring the Wasm object writer can register the correct function signatures and relocations.
Changes:
- Add Wasm instruction support for
unreachable,call, andreturn_call, plus helpers to emit them with function-index relocations. - Enable Wasm emission in
WasmEmitteroutside#if READYTORUN, and implement Wasm stub emission forTentativeMethodNode. - Update
TentativeMethodNodeto provide a type signature to the Wasm object writer, and add a unit test that validates emitted bytes + relocation behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/tools/Common/Compiler/ObjectWriter/WasmInstructions.cs | Adds Wasm expr kinds and helpers to emit unreachable, call, and return_call with function-index relocations. |
| src/coreclr/tools/Common/Compiler/DependencyAnalysis/Target_Wasm/WasmEmitter.cs | Removes #if READYTORUN gating so Wasm function bodies/relocs can be emitted in this compilation path. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TentativeMethodNode.cs | Makes TentativeMethodNode provide a type signature (via IMethodCodeNodeWithTypeSignature) for Wasm signature registration. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_Wasm/WasmTentativeMethodNode.cs | Implements Wasm stub emission: forward args to real body (return_call) or call throw-helper then unreachable. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs | Implements WasmTypeNode(MethodDesc) using WasmLowering.GetSignature(...).FuncType so Wasm signatures can be registered. |
| src/coreclr/tools/aot/ILCompiler.Compiler.Tests/DependencyGraphTests.cs | Adds a focused unit test asserting Wasm encoding + relocation for tentative stubs. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: jtschuster <36744439+jtschuster@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Jackson Schuster <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. |
There was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/coreclr/tools/Common/Compiler/DependencyAnalysis/Target_Wasm/WasmEmitter.cs:22
WasmEmitter.EncodedereferencesFunctionBodywithout validating thatEmitCodeinitialized it. SinceFunctionBodyis assigned vianull!and the struct is created before callingEmitCode, a missing assignment will currently surface as a NullReferenceException with little diagnostic value. Make the field nullable and throw a clear exception if it wasn't initialized, and use a local to avoid repeated field dereferences.
public WasmFunctionBody FunctionBody = null!;
public bool Is64Bit => factory.Target.PointerSize == 8;
public bool RelocsOnly => relocsOnly;
public ObjectNode.ObjectData Encode(ISymbolDefinitionNode symbolDefinitionNode)
{
byte[] encodedThunk = new byte[FunctionBody.EncodeSize()];
FunctionBody.Encode(encodedThunk);
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
Uh oh!
There was an error while loading. Please reload this page.
…/implement-tentativemethodnode-wasm
There was a problem hiding this comment.
🔵 Needs a closer look
The new Wasm tentative-method stub emission currently generates an invalid call sequence / trapping behavior for common signatures, which would break Wasm validation or runtime execution.
Review details
Suppressed comments (1)
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_Wasm/WasmTentativeMethodNode.cs:24
- The Wasm tentative-method stub currently pushes only
local.get 0and then doescall+unreachableunconditionally. For most managed Wasm signatures,WasmLowering.GetSignature(Method).FuncTypehas multiple parameters (e.g., shadow stack pointer plus other lowered args, and typically a trailing hidden parameter), so this will produce an invalid Wasm call (wrong operand stack / signature mismatch). It also traps after the call even whenGetTargetreturns the real body (helper not present), instead of returning normally.
Update the stub to (1) forward all parameters when targeting the real body, and (2) when targeting ThrowBodyRemoved, pass only the parameters that helper’s lowered signature expects, then unreachable.
WasmFuncType signature = WasmLowering.GetSignature(Method).FuncType;
ISymbolNode target = GetTarget(factory);
encoder.FunctionBody = new WasmFunctionBody(
signature,
[
Local.Get(0),
ControlFlow.Call(target),
ControlFlow.Unreachable,
]);
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
NativeAOT’s WASM backend could not emit tentative method stubs.
callplusunreachablefor throw helpers.return_callwhen forwarding to the real body.