[wasm][R2R] Root interpreter thunks for closed static delegate targets - #134497
Conversation
|
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. |
|
Tagging subscribers to this area: @dotnet/crossgen-contrib |
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The closed-static signature construction drops non-Static flags, which can leave required thunks unrooted.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Fixes missing WebAssembly R2R interpreter thunks for closed static delegate targets and adds regression coverage.
Changes:
- Constructs lowered closed-static delegate signatures.
- Roots matching interpreter thunks during delegate invocation.
- Adds compiler layout and nullable-expression regression tests.
| File | Summary |
|---|---|
src/libraries/tests.proj |
Re-enables Microsoft.CSharp tests for Wasm R2R. |
src/libraries/Microsoft.CSharp/tests/DelegateInDynamicTests.cs |
Adds nullable closed-static delegate regression coverage. |
src/coreclr/tools/Common/JitInterface/WasmLowering.cs |
Builds closed-static delegate signatures; must preserve all original signature flags. |
src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs |
Roots the required interpreter thunk. |
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/WasmArgumentLayoutTests.cs |
Tests ABI layouts and hidden arguments. |
Build the target signature with the delegate Invoke flags, and cover default, varargs, and unmanaged calling conventions with a regression test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd5418a9-09c6-4017-a1e1-edb2cf396b39
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Fix hidden-argument flag handling and retain or narrow the test exclusion for the known dispatch failure.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Resolved since last review (1)
Source the interpreter-thunk lowering flags from the resolved call signature instead of the delegate Invoke MethodDesc, so a shared generic delegate's generic-context hidden argument is reflected, and skip unmanaged calls to match the other call-signature recording paths.
Delegate Invoke cannot be unmanaged, so assert the invariant and root the closed-static interpreter thunk unconditionally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/ba-g unrelated CI issues |
Consume GetClosedStaticDelegateTargetSignature from dotnet#134497 for the closed-static retbuf thunk dependency, removing the duplicate signature construction. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd5418a9-09c6-4017-a1e1-edb2cf396b39


Summary
Root the R2R-to-interpreter thunk needed when a Wasm delegate invocation reaches a closed static target, including a runtime-generated expression or dynamic call-site target.
The delegate call site has an instance signature, while the closed static target takes the bound object as an explicit first parameter. These can have identical physical Wasm parameter types but different interpreter thunk keys. For example, the caller can root
IiTiiS4pwhile the dynamic target requiresIiiiiS4p. When the latter is absent, entry-point preparation leaves the code field null and the R2R delegate invocation traps.Changes
MethodSignature, prepending the bound object and using normal Wasm lowering rather than rewriting signature strings.Invokecalls.No VM dispatch changes, diagnostic probes, or experimental switches are included.
Validation
On this branch, based directly on upstream
mainat206bf81aa71b157cb03ae7ed1a42d1ed7d3aa2dc:WasmArgumentLayoutTests: 77 passed, zero failures or skipped tests.Before transferring the same four-file patch from the investigation branch, using browser CoreCLR Release with trimming and R2R:
Microsoft.CSharp.Tests: 4,805 passed, zero failures or skipped tests, after removing diagnostics and rebuilding native/runtime-loader artifacts.DynamicMethodCreateDelegateTests: 14 passed.IiiS4pand trap; enabling it passes.The complete browser runtime suite has not been rerun against a freshly rebuilt main-based runtime; this remains a draft. The broader expression
LambdaTestsslice also exposed an interpreter memory-access failure inInvokeComputedLambda, reproduced with the new dependency disabled at the same native instruction, and is not addressed here.Scope
This is complementary to #134108, which supplies the closed static delegate return-buffer adapter. This PR supplies missing target thunk coverage, including scalar-returning targets; it does not implement that adapter.
Found while investigating #134200. That issue describes a broader startup failure, so this PR does not claim to resolve it in full.
Note
This pull request was prepared with GitHub Copilot assistance.