Repository navigation
[wasm][R2R] Fix closed static delegate struct returns - #134108
Conversation
Add a portable-entrypoint adapter that reorders the captured target and hidden return buffer before dispatching to a closed static delegate target. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd5418a9-09c6-4017-a1e1-edb2cf396b39
|
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 |
Centralize string-discoverable Wasm thunk key generation and document the closed-static return-buffer signature predicates. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd5418a9-09c6-4017-a1e1-edb2cf396b39
Fall back to an interpreted IL adapter when no pregenerated closed-static return-buffer thunk exists, and apply the ABI predicate independently of ReadyToRun. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd5418a9-09c6-4017-a1e1-edb2cf396b39
|
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. |
When no pregenerated closed-static return-buffer thunk is registered, keep the target entrypoint and rely on INTOP_CALLDELEGATE's inline closed-static dispatch instead of generating an IL stub. 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
🔵 Needs a closer look
The unresolved critical fallback-stub preparation concern and moderate test-coverage gap make automated approval unsafe.
Review effort: Lite
Findings: None
What changed in this PR
This PR fixes CoreCLR WebAssembly ReadyToRun closed static delegates returning aggregate structs by adding return-buffer argument shuffling.
Changes:
- Adds runtime IL fallback adapters and portable entrypoint support.
- Emits and roots signature-shared Crossgen2 WebAssembly thunks.
- Expands R2R, interpreted, generic, and dynamic delegate regression coverage.
| File | Summary |
|---|---|
src/tests/readytorun/wasm/WasmInterpreterTransitions/WasmInterpreterTransitions.cs |
Adds delegate regression coverage. Moderate: fallback tests should validate captured object references in returned structs. |
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/DelegateTests.cs |
Verifies delegate metadata, identity, equality, and object-reference returns. |
src/coreclr/vm/wasm/helpers.hpp |
Declares Wasm return-buffer classification and thunk lookup helpers. |
src/coreclr/vm/wasm/helpers.cpp |
Implements Wasm signature classification and thunk lookup. |
src/coreclr/vm/precode_portable.hpp |
Adds portable closed-static thunk entrypoint storage. |
src/coreclr/vm/method.hpp |
Adds the new IL stub type. |
src/coreclr/vm/ilstubcache.cpp |
Registers and names the new stub type. |
src/coreclr/vm/fptrstubs.h |
Adds adapter cache declarations. |
src/coreclr/vm/fptrstubs.cpp |
Implements adapter caching. |
src/coreclr/vm/dllimport.h |
Adds related stub flags and classification support. |
src/coreclr/vm/comdelegate.cpp |
Selects, creates, and caches adapters. Critical: prepare dynamic IL stubs before caching their addresses; nit: remove the dead null branch. |
src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs |
Roots thunk dependencies for relevant signatures. |
src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj |
Includes the new thunk node. Nit: preserve alphabetical item ordering. |
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs |
Caches and roots generated thunks. |
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/WasmVirtualDispatchThunkNode.cs |
Uses shared lookup-key generation. |
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/WasmUnboxingStubNode.cs |
Uses shared lookup-key generation. |
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/WasmClosedStaticRetBufThunkNode.cs |
Emits the closed-static return-buffer thunk. |
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/StringDiscoverableAssemblyStubNode.cs |
Provides shared Wasm lookup-key generation. |
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DelegateCtorSignature.cs |
Roots thunks for delegate constructor signatures. |
|
More cleanup coming - Done |
Root closed-static return-buffer thunks only from compiled call sites, remove creation-only tests that no longer discriminate correctness, and leave interpreter fallbacks uncached for later R2R module registration. 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
Critical issues remain in Wasm thunk ordering and unmanaged-signature adapter classification.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
Resolved since last review (1)
Remove the unused pending-resolution wrapper, preserve Wasm function-table ordering for the D thunk, and use standard contracts for the resolution-only call chain. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd5418a9-09c6-4017-a1e1-edb2cf396b39
|
this code should be updated to use |
|
It would we also good to add the shapes that this fixes into src/tests/readytorun/wasm/WasmInterpreterTransitions/WasmInterpreterTransitions.cs And maybe also into src/tests/readytorun/wasm/WasmInterpreterTransitions/echo.c after #134355 lands. Does this PR impact/enable native C ABI too ? |
|
Thanks. The adapter can precede a managed-callable P/Invoke stub. I verified that locally with a native-linked closed P/Invoke delegate returning a 16-byte blittable struct: the D adapter returned the expected fields, while temporarily bypassing it returned corrupted fields. That's a useful separate end-to-end case we could put alongside Note This reply was generated with GitHub Copilot assistance. |
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
done |
…4611) ## Summary Browser-Wasm CoreCLR ReadyToRun skipped delegate-constructor lowering. Wasm can't use the dynamically composed `READYTORUN_DELEGATE_CTOR` helpers, and because ReadyToRun is `IsAot()`, it also skipped the `GetDelegateCtor` path the JIT uses. As a result, every `new D(target, ftn)` called the original runtime-implemented `.ctor(object, native int)` through the interpreter. With this change, Wasm ReadyToRun uses the same `GetDelegateCtor` transformation as the JIT. Crossgen2 selects the CoreLib constructor: | Selected constructor | Used for | |---|---| | `Delegate.CtorClosed` | Closed instance targets on reference types, with the same explicit arity as `Invoke` (including virtual and generic targets) | | `Delegate.DelegateConstruct` | Every other shape | - `DelegateConstruct` is the managed method the runtime already maps every delegate constructor to (`ecall.cpp`), so the fallback matches today's behavior for all shapes. - The `CtorClosed` shapes match those for which `COMDelegate::GetDelegateCtor` picks `CtorClosed`. The caller has already resolved the method pointer (`ldftn`/`ldvirtftn`, including instantiating stubs), so `CtorClosed` only stores the target and the pointer. - Generated code references the two CoreLib methods by name, the same way runtime async references `AsyncHelpers` methods such as `RestoreInlinedFrameContexts`. The JIT synthesizes these calls, so the caller's IL has no token for them. Crossgen2 therefore pre-seeds their manifest tokens, mirroring `AddNecessaryAsyncReferences`. ## Changes - **`flowgraph.cpp`:** - On `TARGET_WASM`, the ReadyToRun branch of `fgOptimizeDelegateConstructor` is taken only for NativeAOT, so Wasm ReadyToRun falls through to `GetDelegateCtor`. - After retargeting the call, the AOT entry point is recomputed with `getFunctionEntryPoint`, because the importer cached the original constructor's entry point. - Non-Wasm targets and NativeAOT are unchanged. - **Crossgen2:** - `GetDelegateCtor` is implemented in the ReadyToRun partial; the RyuJit partial throws `NotImplementedException`, as before. - Manifest tokens for `CtorClosed` and `DelegateConstruct` are seeded lazily on Wasm. - No R2R format, JIT-EE interface, or VM changes. The image version stays 30.0. ## Validation Local runs on macOS arm64, branch merged with current `main`: - **Builds:** Crossgen2, the cross-targeting Wasm JIT, the osx JIT and SuperPMI; browser-Wasm `clr+libs+host+packs` Release. - **Image tests:** `ILCompiler.ReadyToRun.Tests` with the Wasm target: 123 passed, 0 failed. `WasmDelegateConstructors` asserts that no `DelegateCtor` fixups remain and that both CoreLib constructors are imported. - **Runtime test:** `readytorun/wasm/WasmInterpreterTransitions` passes in these configurations: | Configuration | Result | |---|---| | Composite (harness default; CoreLib outside the bubble) | Pass; also passes with `DOTNET_GCStress=0xC` | | Per-assembly, no cross-module inlining | Pass; also passes with `DOTNET_GCStress=0xC` | | Per-assembly with `--opt-cross-module:*` | Pass; `CtorClosed` is inlined under a `CHECK_IL_BODY`/`VERIFY_IL_BODY` fixup, and the inlined null-receiver throw calls the private CoreLib helper through a manifest reference | | Composite with `--inputbubble` | Pass | It covers the open static, open instance (via an IL helper), closed instance, closed static, virtual (including a derived override), generic owner, shared and unshared generic method, and null-receiver shapes, plus open- and closed-static retbuf shapes. Each is value-checked, with `Target`/`Method` identity and repeated construction. - **Image dumps:** - The composite and per-assembly images call both constructors through `METHOD_ENTRY_REF_TOKEN` manifest references. - CoreLib still contains compiled bodies for both constructors without any extra rooting. - **Other:** `jitformat` and `git diff --check`; independent code review with no findings. ## Performance 100,000 constructions per sample; median of 3 runs × 4 samples, from the same build: | Shape | Interpreter only | R2R | R2R + `--opt-cross-module:*` | |---|---:|---:|---:| | Open static | 9.83 ms | **6.61 ms** | 6.55 ms | | Closed instance | 6.15 ms | **2.50 ms** | 2.22 ms | Before this change, R2R construction took ~44–46 ms for both shapes in an earlier build (directional only). Open static might get faster later from a dedicated `CtorOpen` path, which would need the shuffle thunk and collectible data; that would be a follow-up. ## Related work - #134108 (merged): the closed-static struct-return invocation adapter, now exercised by the runtime test. - #134497 (merged): complementary coverage of closed-static target interpreter thunks. Resolves #134564 > [!NOTE] > This pull request description was generated with GitHub Copilot. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4dd9dfb0-13d5-4a8c-9419-3f808cbd9b8b


Summary
Fix closed static delegates returning structs through a hidden return buffer on CoreCLR WebAssembly ReadyToRun.
A closed static delegate receives arguments using the instance
Delegate.Invokeshape:The static target follows the Wasm aggregate-return ABI and expects:
Both pointer arguments lower to
i32, so the previous direct dispatch passed Wasm type validation but silently exchanged the captured object and return-buffer address. Structs containing object references consequently returned null or invalid references.Approach
Add a portable equivalent of
ThisPtrRetBufPrecode, following the existing portable unboxing-stub design:ClosedStaticRetBufPortableEntryPointstores the target portable entrypoint immediately before its embedded PEP.D-prefixedWasmClosedStaticRetBufThunkNode, shared by physical Wasm signature. It swaps the captured target and return-buffer parameters, then dispatches through the target PEP.SArray<void*>scan/compact helper driven by resolver function pointers. Resolution runs in two global passes—target method PEPs first, then closed-static adapters—so dependencies owned by separate loader allocators can complete during the same module injection.DynamicMethodDescgenerations. Cached unresolved adapters also retry pending registration, including after a previous registration failure.GetSignatureKeyto select only indirect aggregate returns, independently of whether ReadyToRun is enabled. Async targets are excluded because their hidden continuation parameter requires a different adapter shape.Invokemethod, with collectible loader-allocator dependencies preserved.This keeps the normal closed-delegate representation and preserves
Target,Method, equality, and reverseMethodDescmapping. Interpreted calls continue to useINTOP_CALLDELEGATE; compiled calls use the stable adapter PEP once its prerequisites are available. No generated IL adapter is needed.Validation
After merging current
main, including #133656, this PR removes the single temporary #133618 quarantine fromSystem.Tests.DelegateTests.ClosedStaticDelegate. The test body and all assertions remain unchanged, so the trimmed browser CoreCLR ReadyToRun CI lane now exercises the original object-reference struct-return failure directly. Static verification confirmed that no other #133618 suppression exists in source. Live execution of the newly re-enabled library test is delegated to this PR's CI rather than repeated locally.Latest stabilization pass:
WasmInterpreterTransitions: expected 100, actual 100. Coverage includes interpreted/R2R creation and targets, runtime-generated targets, generic wrappers, object-reference returns, direct-return controls, 32 LCG recycling generations with required collection, and colliding physical signatures with different aggregate argument layouts.VerifyDynamicClosedStaticDelegate; restoring fallback passes.function signature mismatch. Publishing the flag after the append resolves the cached adapter on retry. Fault injection was removed before the final build.target-native=64190) but left the adapter unresolved (adapter-native=0). The two global passes resolved both (target-native=64190,adapter-native=64186) during the same module injection and the delegate invocation passed.Earlier revisions also passed desktop
System.Runtime.Tests(79,400 total, zero failures, 89 skipped), an adapter-disabled negative control, and the LCG cache-order negative control. The full desktop suite was not rerun for this stabilization commit.The separate loading, registration-failure, and cross-loader experiments remain local; this PR adds no new test harness or CI lane.
Resolves #133618
Note
This pull request was developed with GitHub Copilot assistance.