-
Notifications
You must be signed in to change notification settings - Fork 206
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
Implement the non-blittable type marshalling proposal #302
Implement the non-blittable type marshalling proposal #302
Conversation
…riction that the stack buffer passed in must be stack allocated. This enables us to conditionally stack alloc depending on used stack space in the future.
public bool b; | ||
} | ||
|
||
struct Native |
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.
It would be nice to try to establish naming convention for these and use it here. Native
is very generic and it does not scale to have more than one in the given namespace.
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 agree that we need to establish a naming convention for these types, but I don't think we need to do that immediately. We'll need to do it when we decide we want to support generating the native types for structs.
|
||
public S ToManaged() => new S { b = Value != 0 }; | ||
|
||
public int Value { get; set; } |
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.
Native type with Value property
To reduce number of locals, I may be nice to pass the Value in directly for the simple case, e.g.:
__retVal_gen_native = Method__PInvoke__(__p_gen_native__marshaler.Value, ...);
|
||
if (scenarioSupportsStackalloc && (!info.IsByRef || info.RefKind == RefKind.In)) | ||
{ | ||
// stackalloc byte[<_nativeLocalType>.StackBufferSize] |
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.
We should set the no-zero init flag on the method - if it is not set globally on the assembly already.
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.
@jkotas Is the intent here avoid the cost of zero initialing the stack space?
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.
Yup
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'd prefer to do this in a follow up PR. This PR is quite large as it is.
…elab into non-blittable-struct-marshaler
DllImportGenerator/DllImportGenerator/Marshalling/MarshallingGenerator.cs
Outdated
Show resolved
Hide resolved
…ing a byref return Value property.
So I've encountered a slight hiccup with the design we have for enabling users to use the stackalloc optimization. Given the following example managed and native type: using System.Runtime.InteropServices;
using System;
class S
{
public byte c;
}
unsafe ref struct Native
{
private byte* ptr;
private Span<byte> stackBuffer;
public Native(S s) : this()
{
ptr = (byte*)Marshal.AllocCoTaskMem(sizeof(byte));
*ptr = s.c;
}
public Native(S s, Span<byte> buffer) : this()
{
stackBuffer = buffer;
stackBuffer[0] = s.c;
}
public ref byte GetPinnableReference() => ref (ptr != null ? ref *ptr : ref stackBuffer.GetPinnableReference());
public S ToManaged()
{
return new S { c = *ptr };
}
public byte* Value
{
get => ptr != null ? ptr : throw new InvalidOperationException();
set => ptr = value;
}
public void FreeNative()
{
if (ptr != null)
{
Marshal.FreeCoTaskMem((IntPtr)ptr);
}
}
public const int StackBufferSize = 1;
} The following code is generated: partial class Test
{
public static void Method(global::S s, in global::S sIn)
{
unsafe
{
//
// Setup
//
global::Native __s_gen_native__marshaler = default;
global::Native __sIn_gen_native__marshaler = default;
//
// Marshal
//
__s_gen_native__marshaler = new global::Native(s, stackalloc byte[global::Native.StackBufferSize]);
__sIn_gen_native__marshaler = new global::Native(sIn, stackalloc byte[global::Native.StackBufferSize]);
//
// Invoke
//
fixed (byte *__s_gen_native = &__s_gen_native__marshaler.GetPinnableReference())
{
fixed (byte *__sIn_gen_native = &__sIn_gen_native__marshaler.GetPinnableReference())
{
Method__PInvoke__(__s_gen_native, &__sIn_gen_native);
}
}
//
// Cleanup
//
__s_gen_native__marshaler.FreeNative();
__sIn_gen_native__marshaler.FreeNative();
}
}
[System.Runtime.InteropServices.DllImportAttribute("DoesNotExist", EntryPoint = "Method")]
extern private static unsafe void Method__PInvoke__(byte *s, byte **sIn);
} This causes a compile error because - global::Native __s_gen_native__marshaler = default;
+ global::Native __s_gen_native__marshaler = new global::Native(default, stackalloc byte[0]); |
Assign it directly?
|
We can do that today, but once we start emitting the We can't just declare the variable in the try block, because then we won't be able to do cleanup. We can't declare the variable outside the try block and only assign it in the try block because then in the finally it would be considered unassigned and the lifetime error would still happen. And we can't just assign it outside of the try block because then we'd risk leaking resources if another struct that marshals via stack allocation (and as such is assigned outside of the try block) fails and throws an exception. |
Raw pointer for the byte* __s_gen_native__stackPtr = stackalloc byte[global::Native.StackBufferSize];
__s_gen_native__marshaler = new global::Native(s, new Span<byte>(__s_gen_native__stackPtr, global::Native.StackBufferSize)); |
That's what the language folks recommend until a better solution is available. |
That should work and doesn’t look that bad! I’ll try that. |
…elab into non-blittable-struct-marshaler
…elab into non-blittable-struct-marshaler
Signed-off-by: Jeremy Koritzinsky <[email protected]>
DllImportGenerator/DllImportGenerator/Marshalling/CustomNativeTypeMarshaler.cs
Outdated
Show resolved
Hide resolved
DllImportGenerator/DllImportGenerator/Marshalling/CustomNativeTypeMarshaler.cs
Outdated
Show resolved
Hide resolved
Could you also update the example generated code in the description to reflect the latest? |
Updated the generated code examples. |
DllImportGenerator/DllImportGenerator.IntegrationTests/NonBlittableStructTests.cs
Outdated
Show resolved
Hide resolved
DllImportGenerator/DllImportGenerator/Marshalling/MarshallingGenerator.cs
Outdated
Show resolved
Hide resolved
DllImportGenerator/DllImportGenerator/Marshalling/CustomNativeTypeMarshaler.cs
Outdated
Show resolved
Hide resolved
…ng and has a Value property being passed in such a way that pinning cannot be used but data needs to be marshalled to native (pass by ref).
Does anyone have any more comments/requested changes? |
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.
Yay!
DllImportGenerator/DllImportGenerator.IntegrationTests/NonBlittableStructTests.cs
Outdated
Show resolved
Hide resolved
}"; | ||
|
||
await VerifyCS.VerifyAnalyzerAsync(source, | ||
VerifyCS.Diagnostic(NativeTypeMustBePointerSizedRule).WithSpan(22, 5, 22, 71).WithArguments("ref byte", "S"), |
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.
The analyzer testing APIs support marking up the source text with expected diagnostics. For the tests in roslyn-analyzers, the general guideline is to use the markup if possible.
If doing argument validation, you still need to create/pass in the expected diagnostics, but I do find it simpler to at least not have to deal with the span line/column numbers. GeneratedDllImportAnalyzerTests
has some tests that use the markup just for the location and combine it with the argument validation.
Not advocating for switching them all in this change - just for future reference.
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'll do this in a follow up PR
DllImportGenerator/DllImportGenerator/Marshalling/CustomNativeTypeMarshaler.cs
Outdated
Show resolved
Hide resolved
DllImportGenerator/DllImportGenerator/Marshalling/CustomNativeTypeMarshaler.cs
Outdated
Show resolved
Hide resolved
LGTM. I did a quick run through again. I don't see anything that is concerning. |
1c2618f
to
5be8022
Compare
Signed-off-by: Jeremy Koritzinsky <[email protected]>
Implement the marshaller defined in the StructMarshalling.md design doc.
Additionally, extend the design to support a Value property that returns a byref (currently works for by value and in parameters only).
Example generated code is included below.
Native type without Value property
Struct definition:Generated code:
Native type with Value property
Struct definition:Generated code:
Native type with stackalloc support and no Value property
Struct definition:Generated code:
Native type with stackalloc support and Value property
Struct definition:Generated code:
Native type with Managed GetPinnableReference optimization
Struct definition:
Generated code:
Native type with GetPinnableReference method
Struct definition:Generated code: