Skip to content

Implement ReadyToRunHelperNode for WASM - #133739

Merged
jtschuster merged 21 commits into
mainfrom
copilot/implement-readytorunhelpernode-for-wasm
Sep 16, 2026
Merged

jtschuster merged 21 commits into
mainfrom
copilot/implement-readytorunhelpernode-for-wasm

Conversation

Copilot AI commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Adds WASM code emission for ReadyToRun static-base, delegate-construction, and virtual-function helpers.

  • Registers WASM function signatures with dependency analysis.
  • Emits architecture-equivalent WASM helper bodies.
  • Preserves shadow-stack and portable-entrypoint arguments across managed tail calls.

Note

This description was generated by GitHub Copilot.

@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.

Copilot AI linked an issue Sep 11, 2026 that may be closed by this pull request
@github-actions github-actions Bot added needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners area-ReadyToRun and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Sep 11, 2026
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

Co-authored-by: jtschuster <36744439+jtschuster@users.noreply.github.com>
Copilot AI changed the title [WIP] Implement ReadyToRunHelperNode for WASM Implement ReadyToRunHelperNode for WASM Sep 11, 2026
Copilot AI requested a review from jtschuster September 11, 2026 21:03
@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.

Copilot AI and others added 2 commits September 12, 2026 00:18
Co-authored-by: jtschuster <36744439+jtschuster@users.noreply.github.com>
Co-authored-by: jtschuster <36744439+jtschuster@users.noreply.github.com>
Co-authored-by: jtschuster <36744439+jtschuster@users.noreply.github.com>

Copilot AI 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

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_Wasm/WasmReadyToRunHelperNode.cs:71

  • These ReturnCall targets are managed entry points (HelperEntrypoint and target.Constructor). In ReadyToRun, WasmLowering gives every managed call the layout $sp, visible args, pep_ptr, but these sequences forward only $sp and the visible arguments, so the generated calls have the wrong arity and cannot preserve the incoming portable entrypoint. The NativeAOT reply does not address the ReadyToRun build of this shared file; append the final incoming local under #if READYTORUN to each managed tail call in this switch.
                            expressions.Add(ControlFlow.ReturnCall(helper));
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Lite

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 15, 2026 17:05

Copilot AI 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

Unresolved critical and moderate issues include a compile-time regression and incorrect WASM relocation handling.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/coreclr/tools/Common/JitInterface/WasmLowering.cs:563

  • Removing the MethodDesc overloads here breaks existing READYTORUN callers that still invoke WasmLowering.GetSignature(MethodDesc) and GetLoweringFlags(MethodDesc) (for example ReadyToRunCodegenNodeFactory.cs:1517, CorInfoImpl.ReadyToRun.cs, and WasmImportThunkPortableEntrypoint.cs). This is a compile-time regression; keep those overloads or update every caller in the same change.
        public static WasmSignature GetSignature(INodeWithTypeSignature node)
        {
            return GetSignature(node.Signature, GetLoweringFlags(node));
        }

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_Wasm/WasmReadyToRunHelperNode.cs:160

  • I32.ConstRVA encodes a linear-memory address, but target.GetTargetNode(factory) is a method entry/function pointer for the CanonicalEntrypoint and ExactCallableAddress delegate cases. The WASM ABI uses WASM_TABLE_INDEX_SLEB for function pointers (see CorInfoTypes.cs:516-519 and emitwasm.cpp:1031-1035), so these delegates receive an image/data address rather than a callable table index; target.Thunk on line 165 has the same problem. Keep DispatchCell as a memory address, but emit method/thunk symbols with a table-index relocation.

[!NOTE] This review comment was created by GitHub Copilot.

                            expressions.Add(I32.ConstRVA(target.GetTargetNode(factory)));
                        }
  • Files reviewed: 8/8 changed files
  • Comments generated: 2
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 15, 2026 17:35

Copilot AI 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

Critical unresolved overload and duplicate-declaration issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 9/9 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread src/coreclr/tools/Common/JitInterface/WasmLowering.cs
Copilot AI review requested due to automatic review settings September 15, 2026 18:22
Comment thread src/coreclr/tools/Common/Compiler/DependencyAnalysis/ObjectNode.cs Outdated

Copilot AI 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.

🔵 Needs a closer look

One or more issues must be addressed before approval.

Review details

Suppressed comments (2)

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_Wasm/WasmReadyToRunHelperNode.cs:159

  • target.GetTargetNode(factory) is a method entrypoint, but I32.ConstRVA emits WASM_MEMORY_ADDR_REL_SLEB. Function pointers in the Wasm ABI require a table-index relocation (WASM_TABLE_INDEX_SLEB); the JIT uses that relocation for IF_FUNCPTR, and the current value will not identify the delegate target in the function table. Emit a table-index constant for this method pointer (and use the same fix for the thunk below).
                            expressions.Add(I32.ConstRVA(target.GetTargetNode(factory)));

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_Wasm/WasmReadyToRunHelperNode.cs:50

  • The new hand-authored helper bodies have no focused regression coverage for the static-base cctor paths or virtual-function resolution. The existing WASM tests cover signature/argument-layout calculations and interpreter-transition thunks, so a wrong local/stack sequence or helper relocation here could still pass those tests; add a WASM NativeAOT test that exercises these helper paths.
        protected override void EmitCode(NodeFactory factory, ref WasmEmitter encoder, bool relocsOnly)
        {
            Debug.Assert(!encoder.Is64Bit);

            List<WasmExpr> expressions = new List<WasmExpr>();
  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 15, 2026 19:11

Copilot AI 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

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_Wasm/WasmReadyToRunHelperNode.cs:203

  • This adds several independent WASM helper paths, but no test exercises the generated static-base, delegate-constructor, or virtual/interface helper bodies. The existing browser WASM test covers R2R/interpreter transition signatures and does not reach these helpers, so malformed function signatures, tail-call argument lists, and helper relocations can regress without detection. Add focused WASM coverage for at least lazy statics, delegate construction, and interface/non-interface dispatch.
            encoder.FunctionBody = new WasmFunctionBody(WasmLowering.GetSignature(this).FuncType, expressions.ToArray());
  • Files reviewed: 6/6 changed files
  • Comments generated: 1
  • Review effort level: Lite

@jtschuster
jtschuster merged commit bad7509 into main Sep 16, 2026
108 of 111 checks passed
@jtschuster
jtschuster deleted the copilot/implement-readytorunhelpernode-for-wasm branch September 16, 2026 17:12
@github-project-automation github-project-automation Bot moved this to Done in AppModel Sep 16, 2026
@dotnet-milestone-bot dotnet-milestone-bot Bot added this to the 12.0-preview1 milestone Sep 17, 2026
jtschuster added a commit to jtschuster/runtime that referenced this pull request Sep 18, 2026
Adds WASM code emission for ReadyToRun static-base,
delegate-construction, and virtual-function helpers.

- Registers WASM function signatures with dependency analysis.
- Emits architecture-equivalent WASM helper bodies.
- Preserves shadow-stack and portable-entrypoint arguments across
managed tail calls.

- Fixes dotnet#133738

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: jtschuster <36744439+jtschuster@users.noreply.github.com>
Co-authored-by: Michal Strehovský <MichalStrehovsky@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Implement ReadyToRunHelperNode for WASM

5 participants