Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Marshalled arrays should include SizeParamIndex and direction #372

Closed
AArnott opened this issue Aug 20, 2021 · 3 comments · Fixed by #552
Closed

Marshalled arrays should include SizeParamIndex and direction #372

AArnott opened this issue Aug 20, 2021 · 3 comments · Fixed by #552
Assignees
Labels
bug Something isn't working

Comments

@AArnott
Copy link
Member

AArnott commented Aug 20, 2021

For #269 we fixed a bug by emitting [MarshalAs(UnmanagedType.LPArray)] on parameters that carry arrays of COM objects.

We might further improve perf and function by adding the [Out] or [In] attributes from the metadata to these same parameters so the interop marshaler can skip some work.
We might also be useful to set the SizeParamIndex argument when the metadata includes it.

@skippy10110 mentioned in #333 (comment)_ a code snippet that included these elements that seem to still be missing.

@ghost
Copy link

ghost commented Aug 20, 2021

In my case the [Out] is required for the call to work correctly. I don't segfault without it, but the array won't be populated if it's not included. (i.e. pceltFetched gets set to 1, but array[0] still equals null)

SizeParamIndex = 0 I'm able to remove without affecting anything.

I didn't look at putting together a minimal repro project for you after you linked the #269 issue. I can still do that if it would be useful.

@AArnott
Copy link
Member Author

AArnott commented Aug 20, 2021

I don't think we need the repro any more now that we understand the fix. And thank you for helping me see the known runtime impact of omitting the [Out] attribute, as that raises this priority from "nice to have" to "must have".

@AArnott AArnott added the bug Something isn't working label Aug 20, 2021
@pziezio
Copy link
Contributor

pziezio commented Aug 23, 2021

SizeParamIndex = 0 I'm able to remove without affecting anything.

In the [In] direction MarshalAsAttribute.SizeParamIndex documentation says that It does not have any effect on managed code that calls COM objects. That makes sense for me since we still need to pass pointer to array and size separately as specified in the COM interface.

In the [Out] direction Default Marshaling for Arrays says When marshaling arrays from unmanaged code to managed code, the marshaler checks the MarshalAsAttribute associated with the parameter to determine the array size. If the array size is not specified, only one element is marshaled. So if the new array is created by the unmanaged code the SizeParamIndex or SizeConst attributes are obligatory. That also makes sense since Marhaller has to know how many elements needs to allocate on the managed side.

@AArnott AArnott self-assigned this May 16, 2022
AArnott added a commit that referenced this issue May 17, 2022
The `MarshalAsAttribute` doesn't let us discern between 0 and unset. The C# compiler can tell and sets metadata based on the syntax. We have to lug around `NativeArrayInfo` in order to track a 0 vs. an unset value though till we're ready to emit syntax.

Fixes #372
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants