[release/10.0] [mono][wasm] Fix pinvoke with 64-bit enum argument#131040
[release/10.0] [mono][wasm] Fix pinvoke with 64-bit enum argument#131040github-actions[bot] wants to merge 3 commits into
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
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
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
|
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. |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "6bac59a1b0d00145dda639ccbe0d898072795c6c",
"last_dispatched_base_ref": "release/10.0",
"last_dispatched_base_sha": "018b2627823158a827a8222f0327f99f6a970565",
"last_reviewed_commit": "6bac59a1b0d00145dda639ccbe0d898072795c6c",
"last_reviewed_base_ref": "release/10.0",
"last_reviewed_base_sha": "018b2627823158a827a8222f0327f99f6a970565",
"last_recorded_worker_run_id": "29694125095",
"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": "6bac59a1b0d00145dda639ccbe0d898072795c6c",
"review_id": 4731077898
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: Justified. This backports merged PR #131021 to release/10.0, fixing a real, customer-reported crash (issue #112262): a P/Invoke taking a 64-bit-underlying enum (e.g. enum : ulong) traps on wasm with null function or function signature mismatch.
Approach: Correct and minimal. interp_type_as_ptr previously classified every enum as pointer-compatible, so a 64-bit enum on a 32-bit target (wasm32) was routed through do_icall's gpointer fast path, producing a call_indirect signature mismatch. The fix defers I8/U8-underlying enums to their underlying type, which is already SIZEOF_VOID_P-guarded; smaller enums and all enums on 64-bit hosts are unchanged. The recursion into the width-guarded scalar check is sound, and the null-guard on mono_class_enum_basetype_internal is appropriate defensive coding.
Summary: ✅ LGTM. Small, well-scoped fix matching the already-merged source PR, with a targeted regression test extending the existing WASM ABI interpreter test to cover both enum:ulong and enum:long round-trips. No concerns.
Detailed Findings
✅ Correctness — Fix targets the exact misclassification
The behavior change is confined to enums with a 64-bit underlying type on 32-bit targets: for those, classification now follows the underlying I8/U8 type (which returns FALSE on wasm32 via the #if SIZEOF_VOID_P == 8 guard), so the call no longer takes the pointer-sized fast icall path. enum:int/enum:uint/smaller enums, and every enum on 64-bit hosts, keep their prior classification. The klass lookup is cached in a local and the basetype is null-guarded before dereference.
✅ Scope — Sibling signature paths verified unaffected
The fix is limited to interp_type_as_ptr, which is 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; consistent with the source PR's analysis.
✅ Test quality — Regression exercises the buggy path
AbiRules.cs and wasm-abi.c add enum:ulong/enum:long direct pinvokes, and the assertions in EnsureWasmAbiRulesAreFollowed verify full 64-bit round-trips (eu (eu)=18374966859414961921 = 0xFF00FF00FF00FF00 + 1, ei (ei)=-2 = -3 + 1). EnsureWasmAbiRulesAreFollowedInInterpreter covers the interpreter path where the bug lives. The native helpers accept_and_return_ulong/accept_and_return_long correctly declare unsigned long long/long long signatures.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 83.6 AIC · ⌖ 10.6 AIC · ⊞ 10K
Backport of #131021 to release/10.0
/cc @lewing
Customer Impact
[Select one or both of the boxes. Describe how this issue impacts customers, citing the expected and actual behaviors and scope of the issue. If customer-reported, provide the issue number.]
Regression
[If yes, specify when the regression was introduced. Provide the PR or commit if known.]
Testing
[How was the fix verified? How was the issue missed previously? What tests were added?]
Risk
[High/Medium/Low. Justify the indication by mentioning how risks were measured and addressed.]
IMPORTANT: If this backport is for a servicing release, please verify that:
release/X.0-staging, notrelease/X.0.release/X.0(no-stagingsuffix).Package authoring no longer needed in .NET 9
IMPORTANT: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older versions.