[wasm] Generate thunks for unmanaged calli signatures and allow thiscall - #134826
Conversation
The portable callhelpers generator only collected signatures from P/Invokes, UnmanagedCallersOnly methods and UnmanagedFunctionPointer delegates, so a calli through an unmanaged function pointer whose shape appeared nowhere else had no interp-to-native thunk and asserted in GetCookieForCalliSig. Scan method bodies for unmanaged calli and add their signatures. ComputeCalliSigThunk also rejected IMAGE_CEE_CS_CALLCONV_THISCALL before building a key; on wasm thiscall lowers the same as cdecl. Co-authored-by: Copilot App <223556219+Copilot@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. |
|
Tagging subscribers to this area: @agocke |
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
Reverts the test disables added in #133571 now that unmanaged calli thunks are generated and thiscall is accepted. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The cross-language ABI changes appear coherent, but the native helper was not compiled and the re-enabled tests were not run end to end.
Review effort: Balanced
Findings: None
What changed in this PR
Adds WebAssembly CoreCLR thunk generation for unmanaged calli signatures and permits thiscall, re-enabling affected calling-convention tests.
Changes:
- Scans IL for unmanaged
callisignatures and emits portable call-helper thunks. - Accepts
thiscallduring runtime thunk lookup. - Adds generator coverage and restores four browser tests.
| File | Description |
|---|---|
src/tests/JIT/Directed/callconv/ThisCall/ThisCallTest.csproj |
Re-enables the browser test. |
src/tests/JIT/Directed/callconv/StdCallMemberFunction/StdCallMemberFunctionTest.csproj |
Restores test-specific browser corerun. |
src/tests/JIT/Directed/callconv/StdCallMemberFunction/StdCallMemberFunctionTest.cs |
Removes the browser skip. |
src/tests/JIT/Directed/callconv/PlatformDefaultMemberFunction/PlatformDefaultMemberFunctionTest.csproj |
Restores test-specific browser corerun. |
src/tests/JIT/Directed/callconv/PlatformDefaultMemberFunction/PlatformDefaultMemberFunctionTest.cs |
Removes the browser skip. |
src/tests/JIT/Directed/callconv/CdeclMemberFunction/CdeclMemberFunctionTest.csproj |
Restores test-specific browser corerun. |
src/tests/JIT/Directed/callconv/CdeclMemberFunction/CdeclMemberFunctionTest.cs |
Removes the browser skip. |
src/coreclr/vm/wasm/helpers.cpp |
Allows thiscall thunk lookup. |
src/coreclr/tools/aot/ILCompiler.ReadyToRun/PortableCallHelpers/PInvokeCollector.cs |
Collects unmanaged calli signatures from IL. |
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/WasmArgumentLayoutTests.cs |
Tests thunk generation and excluded signatures. |
…e thunks The wasm C ABI returns a struct that isn't a single scalar through a hidden leading pointer and no return value. The reverse thunk generator declared such returns as 'void *', so native code calling an [UnmanagedCallersOnly] method returning e.g. a two-float struct trapped with a function signature mismatch (and the interpreter would have written the struct into a 4-byte local). Take the sret pointer as the first parameter and pass it to the interpreter as the return buffer, matching the P/Invoke declaration path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Marshal.GetFunctionPointerForDelegate needs a stub created at run time, which CoreCLR on wasm cannot allocate. Keep the forward and UnmanagedCallersOnly cases running there. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
WasmLowering.GetSignature used HasFlag(UnmanagedCallingConvention) to detect unmanaged signatures, but the calling convention is an enum value in the low nibble, not a bitmask, so cdecl/stdcall/thiscall signatures were lowered as managed. Compare the masked value instead and drop the workaround in the calli collector. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…-calli-thunks Resolves conflicts with #134355 in PInvokeTableGenerator.EmitNativeToInterp and WasmArgumentLayoutTests. The R2R dispatch for exported callbacks now forwards the hidden sret pointer, and the rejection of callbacks returning through a hidden buffer is removed because the wrapper now models that ABI. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…-calli-thunks Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
pavelsavara
left a comment
There was a problem hiding this comment.
Do we need to re-generate committed files?
I don't think so (and double checked browser locally to verify). We do still need to stand up a test that does verify the stubs don't need to be regenerated though. |
On browser CoreCLR,
JIT/Directed/callconv/{Cdecl,StdCall,PlatformDefault}MemberFunctionandThisCallfail with aGetCookieForCalliSig: unknown thunk signatureassert. The MemberFunction tests printWASM calli missing for key: MS8ii, which comes fromdelegate* unmanaged[Cdecl, MemberFunction]<C*, int, SizeF>. That signature only shows up as acalli, so the thunk generator never sees it. ThisCall gets rejected by the callconv switch before a key is even built.#133571 disabled these tests on browser. This PR fixes the cause and re-enables all four tests.
Changes
PortableCallHelpers/PInvokeCollector.cs: the newCollectUnmanagedCalliSignaturesscans IL forcallithrough unmanaged function pointers. It adds their signatures to the interp-to-native thunk table. Managed and varargs calli, generic-shaped signatures, and signatures that can't be lowered are skipped, with a Verbose log.JitInterface/WasmLowering.cs:GetSignaturetreatedUnmanagedCallingConvention(0x9) as a bit flag, so Cdecl, StdCall and ThisCall signatures (0x1–0x3) were lowered as managed. It now checks the masked calling convention, and the collector no longer has to passIsUnmanagedCallersOnlyto compensate.vm/wasm/helpers.cpp:ComputeCalliSigThunknow acceptsIMAGE_CEE_CS_CALLCONV_THISCALL.PortableCallHelpers/PInvokeTableGenerator.cs: when a reverse thunk returns a struct that the wasm C ABI returns by reference, the thunk now takes the hidden leadingsretpointer and hands it to the interpreter as the return buffer. This applies to[UnmanagedCallersOnly]wrappers and exports. Before, it was declared as returningvoid *. Native code callingGetSizethrough the vtable (SizeFreturn) then trapped withfunction signature mismatch, and the interpreter would have written 8 bytes into a 4-byte local. The P/Invoke declaration path already handled this. This also removes the rejection of callbacks that return through a hidden buffer, which [wasm][coreclr] Dispatch R2R-compiled UnmanagedCallersOnly callbacks to their native entrypoint #134355 added because the wrapper didn't model that ABI yet (cc @pavelsavara). The export-only R2R dispatch from [wasm][coreclr] Dispatch R2R-compiled UnmanagedCallersOnly callbacks to their native entrypoint #134355 forwardssretthe same way.WasmArgumentLayoutTests.cs: new testsPortableCallHelpersGeneratorEmitsThunksForUnmanagedCalliSites,PortableCallHelpersGeneratorReturnsStructsThroughHiddenPointerInReverseThunksandSignatureCallingConventionSelectsLowering.src/tests/JIT/Directed/callconv/: reverts theActiveIssue,WasmBuildTestCorerun=false, andCLRTestTargetUnsupporteddisables that Fix combined ActiveIssue restrictions in runtime test wrappers #133571 added. The files now match their state before Fix combined ActiveIssue restrictions in runtime test wrappers #133571. The exception isThisCallTest.cs: itsMarshal.GetFunctionPointerForDelegatereverse cases are now skipped on wasm. CoreCLR wasm can't allocate the stub those need at run time, and it isn't reliable on Mono either ([Mono/WASM]Marshal.GetFunctionPointerForDelegatecrashes the runtime #104391). Its forward andUnmanagedCallersOnlycases still run there.Validation
--generate-portable-callhelpersover the Helix payloads of the three MemberFunction tests. Each one now emitsS8iifromTest8ByteHFA.S8iithrough anUnmanagedFunctionPointerdelegate, so only the runtime switch change matters for it.WasmArgumentLayoutTestspass. They ran against a browser CoreLib/libs layout staged from the Helix correlation payload.function signature mismatchinTest8ByteHFAUnmanagedCallersOnly. I reproduced that locally with the Helix payload. The nativecall_indirectexpects(i32, i32, i32) -> void. After the reverse-thunk fix, the regeneratedGetSizethunk isvoid(void* sret, void*, int32_t). The new unit test covers this, and all 69WasmArgumentLayoutTestspass../build.sh -os browser -c Debug -subset clr+libs, which compiles thehelpers.cppchange) and builtJIT/Directed/callconvwithsrc/tests/build.sh -browser Debug. All four tests exit 100 under node. I reran this after merging main, which brought in [wasm][coreclr] Dispatch R2R-compiled UnmanagedCallersOnly callbacks to their native entrypoint #134355, [wasm][R2R] Fix generic context / async continuation order in Wasm interpreter thunks #134676 and Disable unused ReadyToRun metadata on WebAssembly #134690. All 112ILCompiler.ReadyToRun.TestsinWasmArgumentLayoutTestspass, and so do the four callconv tests.WasmLoweringfix, all 88WasmArgumentLayoutTestspass. The rebuilt crossgen2 regenerates call helpers for the four callconv tests that are byte-identical to the ones linked in the end-to-end run.Note on checked-in tables
The checked-in
src/coreclr/vm/wasm/{browser,wasi}/callhelpers-*.cpptables may now be missing framework calli signatures until they are regenerated withgenerate-coreclr-helpers.sh. A missing entry is not a broken one. They were not regenerated in this PR.Note
This PR description was generated with AI assistance (GitHub Copilot).