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

Implement the non-blittable type marshalling proposal #302

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Nov 4, 2020

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:
[NativeMarshalling(typeof(Native))]
struct S
{
    public bool b;
}

struct Native
{
    private int value;
    public Native(S s)
    {
        value = s.b ? 1 : 0;
    }

    public S ToManaged() => new S { b = value != 0 };
}

Generated code:

partial class Test
{
    public static partial global::S Method(global::S p, in global::S pIn, ref global::S pRef, out global::S pOut)
    {
        unsafe
        {
            global::Native __p_gen_native = default;
            global::Native __pIn_gen_native = default;
            global::Native __pRef_gen_native = default;
            pOut = default;
            global::Native __pOut_gen_native = default;
            global::S __retVal = default;
            global::Native __retVal_gen_native = default;
            //
            // Marshal
            //
            __p_gen_native = new global::Native(p);
            __pIn_gen_native = new global::Native(pIn);
            __pRef_gen_native = new global::Native(pRef);
            //
            // Invoke
            //
            __retVal_gen_native = Method__PInvoke__(__p_gen_native, &__pIn_gen_native, &__pRef_gen_native, &__pOut_gen_native);
            //
            // Unmarshal
            //
            __retVal = __retVal_gen_native.ToManaged();
            pRef = __pRef_gen_native.ToManaged();
            pOut = __pOut_gen_native.ToManaged();
            return __retVal;
        }
    }

    [System.Runtime.InteropServices.DllImportAttribute("DoesNotExist", EntryPoint = "Method")]
    extern private static unsafe global::Native Method__PInvoke__(global::Native p, global::Native*pIn, global::Native*pRef, global::Native*pOut);
}
Native type with Value property Struct definition:
[NativeMarshalling(typeof(Native))]
struct S
{
    public bool b;
}

struct Native
{
    public Native(S s)
    {
        Value = s.b ? 1 : 0;
    }

    public S ToManaged() => new S { b = Value != 0 };

    public int Value { get; set; }
}

Generated code:

partial class Test
{
    public static partial global::S Method(global::S p, in global::S pIn, ref global::S pRef, out global::S pOut)
    {
        unsafe
        {
            nint __pIn_gen_native = default;
            nint __pRef_gen_native = default;
            pOut = default;
            nint __pOut_gen_native = default;
            global::S __retVal = default;
            nint __retVal_gen_native = default;
            //
            // Setup
            //
            global::Native __retVal_gen_native__marshaler = default;
            global::Native __pIn_gen_native__marshaler = default;
            global::Native __pRef_gen_native__marshaler = default;
            global::Native __pOut_gen_native__marshaler = default;
            //
            // Marshal
            //
            __pIn_gen_native__marshaler = new global::Native(pIn);
            __pIn_gen_native = __pIn_gen_native__marshaler.Value;
            __pRef_gen_native__marshaler = new global::Native(pRef);
            __pRef_gen_native = __pRef_gen_native__marshaler.Value;
            //
            // Invoke
            //
            fixed (void *__p_gen_native = p)
            {
                __retVal_gen_native = Method__PInvoke__((nint)__p_gen_native, &__pIn_gen_native, &__pRef_gen_native, &__pOut_gen_native);
            }

            //
            // Unmarshal
            //
            __retVal_gen_native__marshaler.Value = __retVal_gen_native;
            __retVal = __retVal_gen_native__marshaler.ToManaged();
            __pRef_gen_native__marshaler.Value = __pRef_gen_native;
            pRef = __pRef_gen_native__marshaler.ToManaged();
            __pOut_gen_native__marshaler.Value = __pOut_gen_native;
            pOut = __pOut_gen_native__marshaler.ToManaged();
            return __retVal;
        }
    }

    [System.Runtime.InteropServices.DllImportAttribute("DoesNotExist", EntryPoint = "Method")]
    extern private static unsafe nint Method__PInvoke__(nint p, nint*pIn, nint*pRef, nint*pOut);
}
Native type with stackalloc support and no Value property Struct definition:
[NativeMarshalling(typeof(Native))]
struct S
{
    public bool b;
}

struct Native
{
    private int value;
    public Native(S s, System.Span<byte> b)
    {
        value = s.b ? 1 : 0;
    }

    public S ToManaged() => new S { b = value != 0 };

    public const int StackBufferSize = 1;
}

Generated code:

partial class Test
{
    public static partial global::S Method(global::S p, in global::S pIn, out global::S pOut)
    {
        unsafe
        {
            global::Native __p_gen_native = default;
            global::Native __pIn_gen_native = default;
            pOut = default;
            global::Native __pOut_gen_native = default;
            global::S __retVal = default;
            global::Native __retVal_gen_native = default;
            //
            // Marshal
            //
            byte *p__stackptr = stackalloc byte[global::Native.StackBufferSize];
            __p_gen_native = new global::Native(p, new System.Span<byte>(p__stackptr, global::Native.StackBufferSize));
            byte *pIn__stackptr = stackalloc byte[global::Native.StackBufferSize];
            __pIn_gen_native = new global::Native(pIn, new System.Span<byte>(pIn__stackptr, global::Native.StackBufferSize));
            //
            // Invoke
            //
            __retVal_gen_native = Method__PInvoke__(__p_gen_native, &__pIn_gen_native, &__pOut_gen_native);
            //
            // Unmarshal
            //
            __retVal = __retVal_gen_native.ToManaged();
            pOut = __pOut_gen_native.ToManaged();
            return __retVal;
        }
    }

    [System.Runtime.InteropServices.DllImportAttribute("DoesNotExist", EntryPoint = "Method")]
    extern private static unsafe global::Native Method__PInvoke__(global::Native p, global::Native*pIn, global::Native*pOut);
}
Native type with stackalloc support and Value property Struct definition:
[NativeMarshalling(typeof(Native))]
struct S
{
    public bool b;
}

struct Native
{
    public Native(S s, System.Span<byte> b)
    {
        Value = s.b ? 1 : 0;
    }

    public S ToManaged() => new S { b = Value != 0 };

    public int Value { get; set; }

    public const int StackBufferSize = 1;
}

Generated code:

partial class Test
{
    public static partial global::S Method(global::S p, in global::S pIn, out global::S pOut)
    {
        unsafe
        {
            int __p_gen_native = default;
            int __pIn_gen_native = default;
            pOut = default;
            int __pOut_gen_native = default;
            global::S __retVal = default;
            int __retVal_gen_native = default;
            //
            // Setup
            //
            global::Native __retVal_gen_native__marshaler = default;
            global::Native __p_gen_native__marshaler = default;
            global::Native __pIn_gen_native__marshaler = default;
            global::Native __pOut_gen_native__marshaler = default;
            //
            // Marshal
            //
            byte *p__stackptr = stackalloc byte[global::Native.StackBufferSize];
            __p_gen_native__marshaler = new global::Native(p, new System.Span<byte>(p__stackptr, global::Native.StackBufferSize));
            __p_gen_native = __p_gen_native__marshaler.Value;
            byte *pIn__stackptr = stackalloc byte[global::Native.StackBufferSize];
            __pIn_gen_native__marshaler = new global::Native(pIn, new System.Span<byte>(pIn__stackptr, global::Native.StackBufferSize));
            __pIn_gen_native = __pIn_gen_native__marshaler.Value;
            //
            // Invoke
            //
            __retVal_gen_native = Method__PInvoke__(__p_gen_native, &__pIn_gen_native, &__pOut_gen_native);
            //
            // Unmarshal
            //
            __retVal_gen_native__marshaler.Value = __retVal_gen_native;
            __retVal = __retVal_gen_native__marshaler.ToManaged();
            __pOut_gen_native__marshaler.Value = __pOut_gen_native;
            pOut = __pOut_gen_native__marshaler.ToManaged();
            return __retVal;
        }
    }

    [System.Runtime.InteropServices.DllImportAttribute("DoesNotExist", EntryPoint = "Method")]
    extern private static unsafe int Method__PInvoke__(int p, int *pIn, int *pOut);
}
Native type with Managed GetPinnableReference optimization

Struct definition:

[NativeMarshalling(typeof(Native))]
class S
{
    public int i;

    public ref int GetPinnableReference() => ref i;
}

unsafe struct Native
{
    private int* ptr;
    public Native(S s)
    {
        ptr = (int*)Marshal.AllocHGlobal(sizeof(int));
        *ptr = s.i;
    }

    public S ToManaged() => new S { i = *ptr };

    public nint Value
    {
        get => (nint)ptr;
        set => ptr = (int*)value;
    }
}

Generated code:

partial class Test
{
    public static partial global::S Method(global::S p, in global::S pIn, ref global::S pRef, out global::S pOut)
    {
        unsafe
        {
            nint __pIn_gen_native = default;
            nint __pRef_gen_native = default;
            pOut = default;
            nint __pOut_gen_native = default;
            global::S __retVal = default;
            nint __retVal_gen_native = default;
            //
            // Setup
            //
            global::Native __retVal_gen_native__marshaler = default;
            global::Native __pIn_gen_native__marshaler = default;
            global::Native __pRef_gen_native__marshaler = default;
            global::Native __pOut_gen_native__marshaler = default;
            //
            // Marshal
            //
            __pIn_gen_native__marshaler = new global::Native(pIn);
            __pIn_gen_native = __pIn_gen_native__marshaler.Value;
            __pRef_gen_native__marshaler = new global::Native(pRef);
            __pRef_gen_native = __pRef_gen_native__marshaler.Value;
            //
            // Invoke
            //
            fixed (void *__p_gen_native = p)
            {
                __retVal_gen_native = Method__PInvoke__((nint)__p_gen_native, &__pIn_gen_native, &__pRef_gen_native, &__pOut_gen_native);
            }

            //
            // Unmarshal
            //
            __retVal_gen_native__marshaler.Value = __retVal_gen_native;
            __retVal = __retVal_gen_native__marshaler.ToManaged();
            __pRef_gen_native__marshaler.Value = __pRef_gen_native;
            pRef = __pRef_gen_native__marshaler.ToManaged();
            __pOut_gen_native__marshaler.Value = __pOut_gen_native;
            pOut = __pOut_gen_native__marshaler.ToManaged();
            return __retVal;
        }
    }

    [System.Runtime.InteropServices.DllImportAttribute("DoesNotExist", EntryPoint = "Method")]
    extern private static unsafe nint Method__PInvoke__(nint p, nint*pIn, nint*pRef, nint*pOut);
}
Native type with GetPinnableReference method Struct definition:
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;
}

Generated code:

partial class Test
{
    public static partial 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;
            try
            {
                //
                // Marshal
                //
                byte *s__stackptr = stackalloc byte[global::Native.StackBufferSize];
                __s_gen_native__marshaler = new global::Native(s, new System.Span<byte>(s__stackptr, global::Native.StackBufferSize));
                byte *sIn__stackptr = stackalloc byte[global::Native.StackBufferSize];
                __sIn_gen_native__marshaler = new global::Native(sIn, new System.Span<byte>(sIn__stackptr, 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);
                    }
                }
            }
            finally
            {
                //
                // 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);
}

@jkoritzinsky jkoritzinsky added the area-DllImportGenerator Source Generated stubs for P/Invokes in C# label Nov 4, 2020
public bool b;
}

struct Native
Copy link
Member

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.

Copy link
Member Author

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; }
Copy link
Member

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]
Copy link
Member

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.

Copy link
Member

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup

Copy link
Member Author

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.

@jkoritzinsky
Copy link
Member Author

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 __s_gen_native__marshaler is allowed to escape the method based on its declaration, but the stackalloc'd Span cannot. The pattern I've seen to fix this is to create an instance of the struct at declaration time with a 0-length stackalloc (application in our scenario shown below), but I'm not a big fan of this workaround, so I'm looking for alternative ideas on how to solve this problem.

- global::Native __s_gen_native__marshaler = default;
+ global::Native __s_gen_native__marshaler = new global::Native(default, stackalloc byte[0]);

@jkotas
Copy link
Member

jkotas commented Nov 13, 2020

Assign it directly?

global::Native __s_gen_native__marshaler = new global::Native(s, stackalloc byte[global::Native.StackBufferSize]);

@jkoritzinsky
Copy link
Member Author

We can do that today, but once we start emitting the try and finally blocks to actually support cleanup in exceptional scenarios, that approach breaks down.

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.

@elinor-fung
Copy link
Member

Raw pointer for the stackalloc and explicitly construct the Span over it (I think I've seen this in parts of the runtime libraries)?

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));

@stephentoub
Copy link
Member

The pattern I've seen to fix this is to create an instance of the struct at declaration time with a 0-length stackalloc (application in our scenario shown below), but I'm not a big fan of this workaround

That's what the language folks recommend until a better solution is available.
CC: @jaredpar

@jkoritzinsky
Copy link
Member Author

Raw pointer for the stackalloc and explicitly construct the Span over it (I think I've seen this in parts of the runtime libraries)?

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 should work and doesn’t look that bad! I’ll try that.

@elinor-fung
Copy link
Member

Could you also update the example generated code in the description to reflect the latest?

@jkoritzinsky
Copy link
Member Author

Updated the generated code examples.

@jkoritzinsky
Copy link
Member Author

Does anyone have any more comments/requested changes?

Copy link
Member

@elinor-fung elinor-fung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay!

}";

await VerifyCS.VerifyAnalyzerAsync(source,
VerifyCS.Diagnostic(NativeTypeMustBePointerSizedRule).WithSpan(22, 5, 22, 71).WithArguments("ref byte", "S"),
Copy link
Member

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.

Copy link
Member Author

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

@AaronRobinsonMSFT
Copy link
Member

LGTM. I did a quick run through again. I don't see anything that is concerning.

Signed-off-by: Jeremy Koritzinsky <jekoritz@microsoft.com>
@jkoritzinsky jkoritzinsky merged commit fee212a into dotnet:feature/DllImportGenerator Nov 19, 2020
@jkoritzinsky jkoritzinsky deleted the non-blittable-struct-marshaler branch November 19, 2020 18:50
jkoritzinsky added a commit to jkoritzinsky/runtime that referenced this pull request Sep 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-DllImportGenerator Source Generated stubs for P/Invokes in C#
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants