bugfix: fix incorrect dynamic array marshalling#70
bugfix: fix incorrect dynamic array marshalling#70ODtian wants to merge 2 commits intoMikhailGorobets:mainfrom
Conversation
|
@ODtian Unit tests are failing |
There was a problem hiding this comment.
Pull request overview
Fixes incorrect codegen for dynamic array marshalling when an array field is remapped via override-native-type="true" (issue #766), ensuring the correct marshaller is selected and preventing pointer/length field corruption during managed→native copies.
Changes:
- Prevent
RemappedTypeMarshallerfrom intercepting array types; allowValueTypeArrayMarshallerto handle value-type arrays even when remapped. - Refactor
ValueTypeArrayMarshaller.GenerateCopyMemoryto optionally avoid taking the address of the native destination for dynamic arrays. - Add missing dynamic-array length assignment to the related native length field during managed→native marshalling.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| SharpGen/Generator/Marshallers/ValueTypeArrayMarshaller.cs | Routes remapped value-type arrays to this marshaller and adds dynamic-array length assignment; uses takeAddress:false for managed→native dynamic copies. |
| SharpGen/Generator/Marshallers/ValueTypeArrayMarshaller.Copy.cs | Refactors copy helper to optionally skip address-of on the native side. |
| SharpGen/Generator/Marshallers/RemappedTypeMarshaller.cs | Stops remapped-type marshaller from claiming array marshalling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression, IdentifierName(MarshalParameterRefName), IdentifierName(csElement.Name)), | ||
| ParseExpression($"({csElement.PublicType.QualifiedName}*)System.Runtime.InteropServices.NativeMemory.Alloc((nuint) (System.Runtime.CompilerServices.Unsafe.SizeOf<{csElement.PublicType.QualifiedName}>() * {csElement.Name}.Length))"))), | ||
| GenerateCopyMemory(csElement, ArrayCopyDirection.ManagedToNative) | ||
| GenerateCopyMemory(csElement, ArrayCopyDirection.ManagedToNative, takeAddress: false), | ||
| ExpressionStatement(AssignmentExpression(SyntaxKind.SimpleAssignmentExpression, MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression, IdentifierName(MarshalParameterRefName), IdentifierName(csElement.ArraySpecification?.SizeIdentifier)), ParseExpression($"({csElement.ArraySpecification?.TypeSizeIdentifier}){csElement.Name}.Length"))) |
There was a problem hiding this comment.
GenerateCopyMemory(..., takeAddress: false) is applied for the dynamic-array Managed→Native path, but the dynamic-array Native→Managed path still calls GenerateCopyMemory(csElement, ArrayCopyDirection.NativeToManaged) with the default takeAddress=true. Since GetMarshalStorageLocation(csElement) is a pointer field for dynamic arrays, taking its address yields a T** and will copy from/to the pointer variable itself rather than the pointed-to array. Pass takeAddress: false for the dynamic Native→Managed case as well (and consider restricting takeAddress based on ArraySpecificationType.Dynamic inside GenerateCopyMemory to avoid future call-site mistakes).
| CsField { ArraySpecification.Type: ArraySpecificationType.Dynamic } => | ||
| IfStatement(BinaryExpression(SyntaxKind.GreaterThanExpression, GeneratorHelpers.NullableLengthExpression(IdentifierName(csElement.Name)), ZeroLiteral), | ||
| Block( | ||
| ExpressionStatement( | ||
| AssignmentExpression(SyntaxKind.SimpleAssignmentExpression, | ||
| MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression, IdentifierName(MarshalParameterRefName), IdentifierName(csElement.Name)), | ||
| ParseExpression($"({csElement.PublicType.QualifiedName}*)System.Runtime.InteropServices.NativeMemory.Alloc((nuint) (System.Runtime.CompilerServices.Unsafe.SizeOf<{csElement.PublicType.QualifiedName}>() * {csElement.Name}.Length))"))), | ||
| GenerateCopyMemory(csElement, ArrayCopyDirection.ManagedToNative) | ||
| GenerateCopyMemory(csElement, ArrayCopyDirection.ManagedToNative, takeAddress: false), | ||
| ExpressionStatement(AssignmentExpression(SyntaxKind.SimpleAssignmentExpression, MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression, IdentifierName(MarshalParameterRefName), IdentifierName(csElement.ArraySpecification?.SizeIdentifier)), ParseExpression($"({csElement.ArraySpecification?.TypeSizeIdentifier}){csElement.Name}.Length"))) |
There was a problem hiding this comment.
This change introduces new dynamic-array behavior for value-type arrays when the element type is remapped (MappedToDifferentPublicType) and adds length-field assignment during marshalling. There doesn't appear to be an integration test covering a struct field dynamic array with override-native-type="true" + length(...) (e.g., PointerSizeMemberExtended::byteCode in SdkTests/Struct/Mapping.xml). Adding an SdkTests/Struct test that exercises round-tripping (pointer + length) would help prevent regressions like the reported memory corruption and missing length assignment.
The tests pass successfully on my local machine. I suspect this is caused by a misconfiguration in the GitHub Actions workflow, as the output seems to indicate that the .NET 6.0 environment is missing. 🥲 |
Try adding a .NET 6.0 setup step to the CI (it’s also possible to update the configuration files to the latest .NET version supported by GitHub Actions, but I think that would take a lot of time). |
|
There’s also another version of these bindings, but I haven’t tried it |
Thanks for mentioning it, but the project doesn't seem to be officially supported or actively maintained. 😭 BTW, can you approve the awaiting action request? Thanks! |
Could you also add a unit test for the case where the error occurred? (Also, please make the commit titles consistent with this repository’s commit history.) |
Related: DiligentGraphics/DiligentCore#766
Root Cause & Context:
When a user configures
<map override-native-type="true" />for an array field (e.g., mappingbyte[]to a different native type), two cascading issues occur:RemappedTypeMarshallerincorrectly claimed responsibility for the field. Since it lacks array handling capabilities, it generated code that completely missed the array marshalling logic.ValueTypeArrayMarshallerwas correctly forced to handle the field, its implementation for Dynamic Arrays was flawed:GenerateCopyMemory(designed for Constant Arrays), which unconditionally took the address of the target field (&@ref.Ptr). For dynamic arrays (which are pointers), this caused the marshaller to overwrite the pointer variable itself and adjacent struct fields (e.g., array length), leading to Access Violations.LengthRelation), leaving the native API with a zero or uninitialized length.Fix:
RemappedTypeMarshaller.CanMarshalto explicitly exclude array types, preventing incorrect interception.ValueTypeArrayMarshaller.CanMarshalto allow handling of fields that are mapped to different public types (MappedToDifferentPublicType).GenerateCopyMemoryto accept abool takeAddressparameter.takeAddress = falseto use the pointer directly as the destination, bypassing the incorrect address-taking.ArraySpecification) during dynamic array marshalling.