[mono][wasm] Fix pinvoke with 64-bit enum argument#131021
Conversation
A P/Invoke whose argument is an enum with a 64-bit underlying type (e.g. 'enum : ulong') crashed on wasm with 'RuntimeError: null function or function signature mismatch'. interp_type_as_ptr() (used by interp_get_icall_sig to decide whether a native call can take the fast MINT_CALLI_NAT_FAST / do_icall path, which passes every argument as a pointer-sized gpointer) treated every enum as pointer-compatible without looking at its underlying type. Raw long/ulong are already correctly excluded on 32-bit targets via the SIZEOF_VOID_P == 8 guard, but a 64-bit enum slipped through and do_icall then called the native 'void(int64_t)' as 'void(int32_t)' on wasm32, causing a call_indirect signature mismatch. Resolve the enum's underlying type so it inherits the same (correctly guarded) decision as the raw integer type. Behavior only changes for enums with a 64-bit underlying type on 32-bit targets such as wasm32. Adds enum:ulong and enum:long coverage to the WASM ABI pinvoke test. Fixes #112262
|
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. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a WASM Mono interpreter native-call fast-path issue where P/Invokes with 64-bit underlying enum arguments could be treated as pointer-sized and lead to a call_indirect signature mismatch. It also extends the existing WASM ABI test coverage to validate 64-bit enum round-tripping.
Changes:
- Update
interp_type_as_ptrto consider an enum’s underlying type when deciding pointer-compatibility for the fast icall/native-call path. - Extend the WASM ABI native library and managed test entrypoint with
enum:ulongandenum:longP/Invoke coverage. - Add test assertions validating expected 64-bit enum values are observed in the test output.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/mono/mono/mini/interp/transform.c | Changes enum pointer-compatibility logic used for selecting fast native-call/icall signatures. |
| src/mono/wasm/testassets/EntryPoints/PInvoke/AbiRules.cs | Adds managed enums and P/Invoke calls that exercise 64-bit enum argument passing. |
| src/mono/wasm/testassets/native-libs/wasm-abi.c | Adds native functions that accept/return 64-bit integers for the new enum P/Invokes. |
| src/mono/wasm/Wasm.Build.Tests/PInvokeTableGeneratorTests.cs | Adds assertions to validate the new enum-based ABI outputs are present. |
Only enums with a 64-bit underlying type can be misclassified as pointer-sized on a 32-bit target, so defer to the (width-guarded) underlying type only for those and leave the classification of smaller enums unchanged. Verified that a pinvoke with a byte/sbyte/short/ushort/int/uint/long/ulong enum argument all round-trip correctly on wasm. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 62dcd5b3-7227-426b-a072-8812e9c4382d
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
Guard against mono_class_enum_basetype_internal returning NULL (SRE/broken types) before dereferencing, and read m_type_data_get_klass_unchecked once into a local instead of evaluating it twice. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 62dcd5b3-7227-426b-a072-8812e9c4382d
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "d885c28b812da24dfe636706a34d2982b770fafa",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "13c03a4a9a854291f77636b1e15038e65919e68b",
"last_reviewed_commit": "d885c28b812da24dfe636706a34d2982b770fafa",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "13c03a4a9a854291f77636b1e15038e65919e68b",
"last_recorded_worker_run_id": "29689452087",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "d885c28b812da24dfe636706a34d2982b770fafa",
"review_id": 4730869949
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: A P/Invoke whose argument is an enum with a 64-bit underlying type (e.g. enum : ulong) traps on wasm32 with RuntimeError: null function or function signature mismatch. The interpreter's interp_type_as_ptr() classified every enum as pointer-compatible, routing the call through the fast do_icall path that passes each argument as a pointer-sized gpointer. On a 32-bit target that turns a native void(int64_t) into void(int32_t), producing a call_indirect signature mismatch. This is a real, reproducible correctness bug (issue #112262).
Approach: The fix narrows the enum branch: for enums whose underlying type is I8/U8, it defers to interp_type_as_ptr(base_type), which is already width-guarded by #if SIZEOF_VOID_P == 8. All other enums keep their previous classification. This is minimal and well-targeted: behavior only changes for 64-bit enums on 32-bit hosts; smaller enums and all enums on 64-bit hosts are unchanged. The recursion terminates because the underlying type is a primitive. The base_type && guard correctly handles a potential NULL from mono_class_enum_basetype_internal. The claim that AOT (type_to_c) and jiterpreter paths already reduce enums to their underlying type is consistent with those paths not being implicated in the reported crash. The regression test is a sensible extension of the existing WASM ABI test: it adds enum:ulong/enum:long direct pinvokes exercised by the interpreter path where the bug lives, and asserts the full 64-bit round-trip values (18374966859414961921 = 0xFF00FF00FF00FF00 + 1; -2 = -3 + 1), both of which check out.
Summary: LGTM. The root cause analysis is accurate, the fix is minimal and correctly guarded, and the regression coverage exercises exactly the changed cases through the affected (interpreter) path. No actionable findings. As with any Mono interp change, final confidence rests on the WASM CI legs (EnsureWasmAbiRulesAreFollowedInInterpreter) passing.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 64 AIC · ⌖ 10.4 AIC · ⊞ 10K
|
The milestone is set to 10.0.x, but the PR targets main. |
|
/backport to release/10.0 |
|
Started backporting to |
Summary
Fixes #112262. A P/Invoke whose argument is an enum with a 64-bit underlying type (e.g.
[Flags] enum Flags : ulong) crashes on wasm at the call site withRuntimeError: null function or function signature mismatch.Root cause
interp_type_as_ptr()(interp/transform.c) decides whether a native call can take the fastMINT_CALLI_NAT_FAST→do_icallpath, which passes every argument as a pointer-sizedgpointer. It treated every enum as pointer-compatible without checking its underlying type:Raw
long/ulongare already correctly excluded on 32-bit targets via the#if SIZEOF_VOID_P == 8guard (which is why the documented workaround — casting the enum toulong— works), but a 64-bit enum slipped through.do_icallthen called the nativevoid(int64_t)asvoid(int32_t)on wasm32 →call_indirectsignature mismatch.Fix
Only a 64-bit enum can be misclassified as pointer-sized on a 32-bit target, so for enums with an
I8/U8underlying type defer to the underlying type (which isSIZEOF_VOID_P-guarded); all other enums keep their existing classification. Behavior only changes for enums with a 64-bit underlying type on 32-bit targets such as wasm32;enum:int/enum:uint/smaller enums and all enums on 64-bit hosts are unchanged.interp_type_as_ptris interp-transform-only; the AOT (type_to_c) and jiterpreter (mono_type_to_ldind) signature paths already reduce enums to their underlying type, so no parallel change is needed.Test
Extended the existing WASM ABI pinvoke test (
AbiRules.cs+wasm-abi.c) withenum:ulongandenum:longdirect pinvokes — the two cases the fix changes.EnsureWasmAbiRulesAreFollowedInInterpreter(the interpreter path where the bug lives) exercises the regression.Verification
Built the browser Mono runtime with the fix and ran the WASM ABI interpreter test end-to-end:
void(enum:ulong)pinvoke →function signature mismatch, app aborts before running.eu (eu)=18374966859414961921,ei (ei)=-2).Also verified out-of-band that a pinvoke argument of every enum width —
byte/sbyte/short/ushort/int/uint/long/ulong— round-trips correctly on wasm with this change.Note
This pull request was authored with GitHub Copilot.