Skip to content

Implement wasm JumpStubNode with signature-matched argument forwarding - #133248

Open
jkoritzinsky with Copilot wants to merge 5 commits into
mainfrom
copilot/implement-jumpstubnode-for-wasm
Open

Implement wasm JumpStubNode with signature-matched argument forwarding#133248
jkoritzinsky with Copilot wants to merge 5 commits into
mainfrom
copilot/implement-jumpstubnode-for-wasm

Conversation

CopilotAI commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Implements JumpStubNode for Wasm so address-taken method stubs no longer throw NotImplementedException during emission. The stub now preserves the target method’s lowered Wasm signature, forwards all arguments, and transfers control to the target.

  • Wasm JumpStub emission

    • Replaces the Wasm JumpStubNode.EmitCode stub implementation with real body generation.
    • Computes the lowered Wasm function type from INodeWithTypeSignature.
    • Emits local.get for each parameter and calls the target symbol.
  • Signature propagation for address-taken methods

    • Updates AddressTakenMethodNode to implement IMethodCodeNodeWithTypeSignature.
    • Ensures Wasm method declaration/signature recording uses the target method signature for emitted jump stubs.
for(inti=0;i<signature.Params.Types.Length;i++){expressions.Add(Local.Get(i));}expressions.Add(ControlFlow.Call(_target));encoder.FunctionBody=newWasmFunctionBody(signature,expressions.ToArray());

@azure-pipelines

Copy link
Copy Markdown
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: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
CopilotAI changed the title [WIP] Implement JumpStubNode as a WASM stubImplement wasm JumpStubNode with signature-matched argument forwardingSep 4, 2026
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
@jkoritzinsky
jkoritzinsky marked this pull request as ready for review September 4, 2026 18:14
@jkoritzinsky
jkoritzinsky requested review from jtschuster and a lite review from CopilotSeptember 4, 2026 18:14
@azure-pipelines

Copy link
Copy Markdown
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.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new Wasm stub uses return_call (tail-call proposal opcode), which can break compatibility on runtimes that don’t enable tail calls.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR adds a real Wasm implementation of JumpStubNode so address-taken method stubs can be emitted with a signature that matches the target method, forwarding all lowered Wasm arguments and transferring control to the target.

Changes:

  • Add a new Wasm instruction kind and helper for return_call.
  • Implement JumpStubNode.EmitCode for Wasm by computing the lowered WasmFuncType, emitting local.get for each parameter, and invoking the target.
  • Update AddressTakenMethodNode to implement IMethodCodeNodeWithTypeSignature so signature metadata can flow through for Wasm emission.
File summaries
FileDescription
src/coreclr/tools/Common/Compiler/ObjectWriter/WasmInstructions.csAdds ReturnCall opcode support and a ControlFlow.ReturnCall helper.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_Wasm/WasmJumpStubNode.csImplements Wasm JumpStubNode emission with signature-based argument forwarding and target invocation.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/AddressTakenMethodNode.csSwitches to IMethodCodeNodeWithTypeSignature to propagate method signature info for Wasm stub emission.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment on lines +17 to +34
WasmLowering.LoweringFlags flags = WasmLowering.LoweringFlags.None;

if (signatureNode.HasGenericContextArg)
{
flags |= WasmLowering.LoweringFlags.HasGenericContextArg;
}

if (signatureNode.IsAsyncCall)
{
flags |= WasmLowering.LoweringFlags.IsAsyncCall;
}

if (signatureNode.IsUnmanagedCallersOnly)
{
flags |= WasmLowering.LoweringFlags.IsUnmanagedCallersOnly;
}

WasmFuncType signature = WasmLowering.GetSignature(signatureNode.Signature, flags).FuncType;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not required for this PR, but we have similar logic in CorInfoImpl.recordCallSite(), CorInfoImpl.recordWasmManagedCallSig and WasmObjectWriter.RecordMethodDeclaration(). Should we add an overloads of WasmLowering.GetSignature() that take INodeWithTypeSignature or CORINFO_SIG_INFO* and generates the flags?

protected override void EmitCode(NodeFactory factory, ref WasmEmitter encoder, bool relocsOnly)
{
throw new NotImplementedException();
INodeWithTypeSignature signatureNode = (INodeWithTypeSignature)this;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JumpStubNode doesn't implement INodeWithTypeSignature, though its only implementation, AddressTakenMethodNode, does. Should we add INodeWithTypeSignature to JumpStubNode with abstract implementations?

Comment on lines +36 to +43
List<WasmExpr> expressions = new List<WasmExpr>(signature.Params.Types.Length + 1);
for (int i = 0; i < signature.Params.Types.Length; i++)
{
expressions.Add(Local.Get(i));
}

expressions.Add(ControlFlow.ReturnCall(_target));
encoder.FunctionBody = new WasmFunctionBody(signature, expressions.ToArray());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could we preallocate the array instead of using a List and ToArray()?

@jtschusterjtschuster left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple nits, but not necessarily blocking.

@jkoritzinsky

Copy link
Copy Markdown
Member

@copilot please address all feedback from jtschuster.

Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
CopilotAI review requested due to automatic review settings September 5, 2026 18:51

CopilotAI commented Sep 5, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot please address all feedback from jtschuster.

Addressed in 492d5db4: added shared WasmLowering overloads/helpers for signature lowering flags, made JumpStubNode carry the INodeWithTypeSignature contract directly, and preallocated the JumpStub expression array.

Note

AI-generated via GitHub Copilot.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

WasmLowering.GetLoweringFlags(CORINFO_SIG_INFO*) currently treats any non-zero callConv flag bits (e.g., HASTHIS) as unmanaged, which can produce incorrect lowered signatures for normal managed calls.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment threadsrc/coreclr/tools/Common/JitInterface/WasmLowering.cs Outdated
…lags
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
CopilotAI review requested due to automatic review settings September 5, 2026 19:04

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The changes are cohesive and consistent with existing Wasm lowering/signature patterns, and no correctness issues were identified in the updated emission and flag/signature propagation.

Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arch-wasmWebAssembly architecturearea-NativeAOT-coreclr

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Implement JumpStubNode for WASM

4 participants

@jkoritzinsky@jtschuster