SafeHandle marshalling #133
Conversation
Extend the MarshallingInfo discriminated union to not only represent attribute info but to also represent any other marshalling info that requires a Compilation object to determine. This avoids having to plumb passing down a compilation object through 4 API layers and makes the marshaller selection switch statement a little bit simpler.
… using xunit asserts that currently we just call directly in the Demo.
…mentation to explain the overall logic of SafeHandle marshaller. Signed-off-by: Jeremy Koritzinsky <[email protected]>
jkoritzinsky
added
the
area-DllImportGenerator
| /// where native values will leak if we | ||
| /// fail to unmarshal, but do run cleanup. | ||
| /// </summary> | ||
| LeakSafeUnmarshal |
There was a problem hiding this comment.
It may be useful to have a comment that describes the overall structure of the method to make it clear how the stages fit together. Something like:
... Setup ...
try
{
... Marshal ...
fixed (... Pin ...)
{
... Invoke ...
}
... Unmarshal ...
}
finally
{
... Cleanup...
}
... KeepAlive ...
There was a problem hiding this comment.
This is something we are attempting to push into the design documents: https://github.com/dotnet/runtimelab/blob/DllImportGenerator/DllImportGenerator/designs/Pipeline.md
Which also need to be updated with these new stages.
DllImportGenerator/DllImportGenerator/Marshalling/SafeHandleMarshaller.cs
Outdated
Show resolved
Hide resolved
| /// where native values will leak if we | ||
| /// fail to unmarshal, but do run cleanup. | ||
| /// </summary> | ||
| LeakSafeUnmarshal |
There was a problem hiding this comment.
This is something we are attempting to push into the design documents: https://github.com/dotnet/runtimelab/blob/DllImportGenerator/DllImportGenerator/designs/Pipeline.md
Which also need to be updated with these new stages.
DllImportGenerator/DllImportGenerator/Marshalling/SafeHandleMarshaller.cs
Outdated
Show resolved
Hide resolved
…rshaller.cs Co-authored-by: Jan Kotas <[email protected]>
I think exception thrown during cleanup should be a non-scenario. |
|
Any more feedback? |
| InvocationExpression( | ||
| MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression, | ||
| ParseTypeName(TypeNames.System_Runtime_InteropServices_MarshalEx), | ||
| IdentifierName("SetHandle")), |
There was a problem hiding this comment.
Being a guest here, but happened to see this is the only that doesn't currently use nameof?
There was a problem hiding this comment.
The MarshalEx type isn't a type in the .NET Standard 2.0 contract, so we can't use nameof for any of its methods. It's currently in Ancillary.Interop. In the future its methods will move into dotnet/runtime in some fashion.
Co-authored-by: Jan Kotas <[email protected]> Commit migrated from dotnet/runtimelab@2cde5aa
Implement default marshalling for SafeHandles using both CoreCLR and CoreRT's implementations as a baseline.
Introduce a new phase : "LeakSafeUnmarshal" to represent unmarshalling steps that cause leaks if skipped in exceptional cases.
Since we don't have access to a
Compilationobject inMarshallingGenerators.TryCreateand plumbing one down there requires passing it through 3 or 4 layers of the API, I've renamed theMarshallingAttributeInfodistriminated union toMarshallingInfoand extended it to also represent marshalling info that requires aCompilationobject to calculate it.I've added some xunit-like execution tests in the Demo project that the Main method just calls directly until we decide how we want to do our execution testing.