-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add new SafeHandleMarshaller type to provide out-of-generator marshalling support. #85419
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @dotnet/interop-contrib Issue DetailsFixes #74035 We can't remove the built-in marshalling support from the generator yet, but once the out-of-band packages we ship don't support .NET 6. we can remove the built-in support that emits the marshalling code in the stub. I believe the .NET 9 packages won't support .NET 6, so once we snap for .NET 9 and update how we ship the packages, we can clean this up. This PR also adds a requested feature to the SafeHandle marshaller: If the call to native code fails, we'll call
|
...ystem.Private.CoreLib/src/System/Runtime/InteropServices/Marshalling/SafeHandleMarshaller.cs
Show resolved
Hide resolved
…turn/out marshaller might still want to use it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need any additional testing here or is this simply making our existing semantics public? Please do a quick audit just to make sure we have good coverage here.
....InteropServices/gen/Microsoft.Interop.SourceGeneration/SafeHandleMarshallingInfoProvider.cs
Show resolved
Hide resolved
....InteropServices/gen/Microsoft.Interop.SourceGeneration/SafeHandleMarshallingInfoProvider.cs
Outdated
Show resolved
Hide resolved
...ystem.Private.CoreLib/src/System/Runtime/InteropServices/Marshalling/SafeHandleMarshaller.cs
Outdated
Show resolved
Hide resolved
It would be good to see if we have a test for this marshaller and arrays. Hopefully we are close to it just being apart of the system and that should be naturally covered, but that is a scenario that is troublesome and very bad when we get it wrong. |
Explicitly did not support marshalling arrays of SafeHandles beforehand and now we won't by construction of our custom type marshalling system. I'll add some tests in CompileFails to validate. |
…esign doesn't support that, so we won't use it.
Looks like adding this marshaller might have caused a significant size regression in NativeAOT, though I'm not sure why the regression would be as large as it is. I'll try to investigate that today. |
Found the problem, and it's not good. NativeAOT doesn't accelerate the |
Marking as blocked as we need to resolve how to avoid rooting the reflection stack before we merge this in. |
…rameterless constructors in the SafeHandleMarshaller.
I'm proposing a breaking change to avoid the NativeAOT size regression and that should be reviewed.
Added When you commit this breaking change:
Tagging @dotnet/compat for awareness of the breaking change. |
Based on a conversation with @jkotas in #85541, we have decided to take a breaking change to only allow |
…dle tests to reflect the breaking change.
How would an array be handled using this marshaller? For example, [LibraryImport("some_lib")]
public static partial void WaitForMultipleSignals([MarshalUsing(typeof(ArrayMarshaller<,>), CountElementName = nameof(numSignals))] SafeWaitHandle[] signals, int numSignals); Wouldn't it also require support for |
We don't support arrays of
Yes. We are not supporting this at present. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also update our compat doc about SafeHandle
scenarios. This is breaking change from DllImport
, which we've typically documented in the design doc.
Fixes #74035
We can't remove the built-in marshalling support from the generator yet, but once the out-of-band packages we ship don't support .NET 6. we can remove the built-in support that emits the marshalling code in the stub. I believe the .NET 9 packages won't support .NET 6, so once we snap for .NET 9 and update how we ship the packages, we can clean this up.
This PR also adds a requested feature to the SafeHandle marshaller: If the call to native code fails, we'll call
Dispose()
on the pre-allocated handle to avoid leaking it to the finalizer queue.