Skip to content

Fix pointer array copying and various array pinning regressions in CoreCLR - #134284

Open
jkoritzinsky wants to merge 13 commits into
dotnet:mainfrom
jkoritzinsky:jkoritzinsky-pointer-array-marshalling
Open

jkoritzinsky wants to merge 13 commits into
dotnet:mainfrom
jkoritzinsky:jkoritzinsky-pointer-array-marshalling

Conversation

@jkoritzinsky

@jkoritzinsky jkoritzinsky commented Sep 19, 2026 •

Copy link
Copy Markdown
Member

Summary

Fix copying and pinning regressions in CoreCLR's built-in array marshalling:

  • Incorrect copying: the array metadata retained the pointee type while using a pointer-sized VARTYPE. The generic marshaler consequently used the pointee's size: a four-element byte*[] allocated and copied four bytes instead of 32 on x64.
  • Lost pinning: the pinning predicate required a blittable value-type method table, excluding pointer and function-pointer type descriptors.
  • Enum pinning: evaluate pinning against the enum's underlying primitive instead of the enum's own layout flags.
  • Unicode character pinning: recognize char[] when its selected native representation is UTF-16, including explicit I2/U2 element types. ANSI and Boolean conversions continue to use copying.

Preserve the actual element TypeHandle in the marshalling operands, and select nint only as the carrier for generic copying. Managed allocation retains the declared pointer-array type and preserves null native arrays. Eligible forward P/Invokes use the existing pinning path; ref/out and reverse calls continue to copy.

Treat pointer elements as opaque native-sized values, including function pointers, struct pointers, and pointer-to-pointer elements. Update the managed compatibility assertions and native layout consumers accordingly. SAFEARRAY pointer support and non-pointer conversion rules are not broadened.

Regression coverage

Added 324 cases in the existing interop suites: 250 pointer/native-integer cases and 74 enum/character/conversion-control cases. Coverage includes forward P/Invoke and unmanaged-function-pointer delegates, required copying, exact copy-back array types, ref/out parameters, reverse callbacks, fixed-array fields, null/empty and odd-length arrays, and pinning across compacting GC. Pointer coverage includes float*, struct pointers, pointer-to-pointer elements, and managed/unmanaged function pointers.

The enum cases cover all eight standard underlying integral types. Character cases cover UTF-16, ANSI, and explicit element-type overrides; Boolean controls ensure conversion still occurs. Assertions distinguish passing managed contents directly from copying. Native exports and callback types explicitly use Cdecl for x86.

Validation

The product fixes and full 324-case regression set were validated locally:

Runtime configuration New cases passed Total related array cases passed
Windows x64 Checked 324 344
Windows x64 Release 324 344
Windows x86 Checked 324 343

The x86 difference is a 64-bit-only SAFEARRAY element-size mismatch control. Result XML was checked for failures, rather than relying only on the merged runner's exit code.

After the final test-only renames and runtime-specific data splits, the x64 Checked managed interop tests were rebuilt and rerun: 344 passed, 0 failed, including all 324 added cases. The Release and x86 results above precede those test-only changes.

Baseline copying, pinning, and function-pointer failures were reproduced. With only the copying repair applied, 80 copy cases passed while all four pinning-direction checks still failed, verifying that pinning does not merely hide a broken copy path.

The enum/character additions were also run before their fix: 32 pinning cases failed while all ANSI and Boolean conversion controls passed. All 74 added enum/character/control cases pass with the updated predicate.

IntPtr[] and UIntPtr[] already behaved correctly on this checkout; their product behavior is unchanged and is covered by the new tests.

Runtime-specific test exclusions

CI exposed separate NativeAOT and Mono issues; this PR does not change either runtime's implementation. Split theory data into passing and failing partitions and use narrowly scoped ActiveIssue attributes:

The newly excluded row sets match the recorded CI failures exactly, and the generated merged runner's predicates and skip reporting were checked. NativeAOT and Mono behavior was investigated from CI evidence and source inspection; neither runtime was rerun locally. Linux and ARM64 were not run locally.

Generated-code comparison

Release x64 P/Invoke stubs, with tiering and ReadyToRun disabled:

Direction Baseline bytes Fixed bytes Baseline instructions Fixed instructions
Default 294 185 86 58
In 294 185 86 58
Out 303 185 89 58
In/Out 332 185 97 58

The fixed pinning stubs contain no unmanaged-allocation helper, Memmove, or CoTaskMemFree calls. These are code-generation measurements, not benchmark timings.

For the new enum and UTF-16 declarations, Release x64 stubs shrink from 281-285 bytes (82-83 instructions) to 170 bytes (54 instructions), likewise eliminating allocation/copy/free calls. The ANSI and Boolean control stubs retain their previous code sizes and call paths.

Resolves #134174
Resolves #134576
Resolves #134577
Resolves #134575
Resolves #134573

Note

This PR was created with assistance from GitHub Copilot.

Preserve pointer element TypeHandles while using native-sized carriers for generic marshalling. Restore pinning for eligible forward calls and cover data/function pointers, copy-back, callbacks, and fixed-array fields.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@azure-pipelines

Copy link
Copy Markdown
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.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

CoreCLR IL stub and interop behavior changed substantially, with only Windows x86/x64 validation reported and Linux/ARM64 unverified.

Review effort: Lite
Findings: None

What changed in this PR

Fixes CoreCLR pointer-array marshalling by preserving pointer element types, using nint as the copy carrier, and enabling safe pinning.

Changes:

  • Corrects native-size copying and declared array-type preservation.
  • Enables pinning for pointer and function-pointer arrays.
  • Adds extensive P/Invoke, reverse-callback, and fixed-array tests.
File Description
mlinfo.h Preserves the element TypeHandle.
mlinfo.cpp Treats pointer elements as native-sized values.
ilmarshalers.cpp Updates carrier selection, allocation, pinning, and layout handling.
fieldmarshaler.cpp Uses the preserved element type for native layout.
StubHelpers.cs Supports pointer-array compatibility assertions.
MarshalArrayLPArrayNative.cpp Adds native pointer-array test helpers.
AsLPArrayTest.cs Adds pointer-array marshalling and pinning coverage.
AsByValArrayTest.cs Adds fixed pointer-array field coverage.

Comment thread src/tests/Interop/PInvoke/Array/MarshalArrayAsParam/AsLPArray/AsLPArrayTest.cs Outdated
Comment thread src/tests/Interop/PInvoke/Array/MarshalArrayAsParam/AsLPArray/AsLPArrayTest.cs Outdated
Add five- and twenty-one-element cases with a fifth pointer value of 21, and cover float pointers in parameter and fixed-field marshalling tests.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 22, 2026 17:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Broad, low-level CoreCLR marshalling changes require final human review.

Review effort: Lite
Findings: None

Replace manual GCHandle cleanup with a using-scoped GCHandle<Array> and use matching typed context conversions in the collection callback.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 22, 2026 23:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The marshalling and runtime changes require final human review.

Review effort: Lite
Findings: None

Skip the new fixed-array cases on wasm until test-specific reverse thunks are available. Keep function-address preparation in a non-inlined helper behind the platform guard and document the tracking issue.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 23, 2026 22:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 1 Medium severity

Open (2)

Comment thread src/tests/Interop/PInvoke/Array/MarshalArrayAsParam/AsLPArray/AsLPArrayTest.cs Outdated
Apply the missing reverse-thunk issue to the new LPArray suite and fixed-array theory so wasm reports skipped tests. Remove the manual platform guard and helper split.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 23, 2026 23:24
Comment thread src/tests/Interop/PInvoke/Array/MarshalArrayAsParam/AsLPArray/AsLPArrayTest.cs Outdated
@jkoritzinsky

Copy link
Copy Markdown
Member Author

Fixed the NativeAOT coverage and addressed the two new issues.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The runtime marshalling changes are broad and require final human review.

Review effort: Lite
Findings: None

@jkoritzinsky jkoritzinsky changed the title Fix pointer array copying and pinning in CoreCLR Fix pointer array copying and various array pinning regressions in CoreCLR Sep 24, 2026
Rename direct-contents assertions and split failing NativeAOT and compiled Mono cases into narrowly excluded tests. Preserve passing theory rows and track the exclusions with ActiveIssue attributes.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 24, 2026 23:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Broad runtime marshalling changes and incomplete cross-platform validation warrant final human review.

Review effort: Lite
Findings: None

Comment thread src/tests/Interop/PInvoke/Array/MarshalArrayAsParam/AsLPArray/AsLPArrayTest.cs Outdated
jkoritzinsky and others added 4 commits September 26, 2026 09:06
Replace the array tests IsSupported wrappers with class-level ActiveIssue attributes for dotnet#91388, preserving the existing platform and runtime filtering.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3a58d7c5-bde9-4e44-a0bb-320dd6b721df
Prevent VT_CY arrays from bypassing element conversion through pinning. Cover default, In, Out, and InOut array directions through P/Invokes and delegates, with default decimal arrays retaining direct-content behavior.

Fixes dotnet#134575

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3a58d7c5-bde9-4e44-a0bb-320dd6b721df
Expand runtime-owned LPArray and AsAny buffers using checked maximum multibyte sizes. Preserve fixed-field and caller-owned reverse-PInvoke byte bounds with an explicit marshaler option, and keep native-to-managed byte counts unchanged. Cover conversion, copy-back, explicit subtypes, AsAny, and bounded-buffer canaries.

Fixes dotnet#134573

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3a58d7c5-bde9-4e44-a0bb-320dd6b721df
Track the unsupported NativeAOT decimal marshalling cases with an ActiveIssue for dotnet/runtimelab#175 while preserving coverage on other runtimes.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3a58d7c5-bde9-4e44-a0bb-320dd6b721df

@MichalStrehovsky MichalStrehovsky left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not the one ultimately in charge, but this is starting to conflate fixes for things unrelated to pinning that will make backporting more difficult if we need to backport some of this.

Comment thread src/tests/Interop/PInvoke/Array/MarshalArrayAsParam/AsLPArray/AsLPArrayTest.cs Outdated
@jkoritzinsky

Copy link
Copy Markdown
Member Author

All of this PR needs to be back ported. I figured it was easier to backport 1 or 2 PRs that fix a bunch of regressions than 7 or 8 PRs with conflicting changes.

Comment thread src/tests/Interop/PInvoke/Array/MarshalArrayAsParam/AsLPArray/AsLPArrayTest.cs Outdated
Fix issue link

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

4 participants