Ensure wrapper structs are generated in a safe and compatible way

[tannergooding]

Wrapper structs are convenient. However, they are not always efficient nor compatible with the underlying ABI.

The best known case of this is with C++ instance members, where (on Windows) class C { MyStruct M(...); } is actually MyStruct* M(C* pThis, MyStruct* retVal, ...) and where this holds true for any struct, not just complex structs.
This means that if the native signature is, for example, class C { LRESULT M(...); }; where typedef intptr_t LRESULT; then the C# side function pointer must be delegate* unmanaged[Thiscall]<T*, ..., nint> and not delegate* unmanaged[Thiscall]<T*, ..., LRESULT>

This, in particular, crops up frequently when working with COM bindings, such as for DirectX and while I am not presently aware of any other OS/Architectures where this shows, it is not guaranteed that it never will.
This is likewise why the new CallConvMemberFunction modifier is being introduced, so function pointers for COM and other scenarios can be correctly annotated as delegate* unmanaged[Stdcall, MemberFunction] and the JIT can automatically handle the required differences from the non-member function calling convention.

These differences can lead to subtle pit of failures, particularly when working with delegates and function pointers, and as such CsWin32 should ensure that its own generated signatures and exposed types are compatible (both with known cases today and potential future cases as more platforms are supported and more ABIs to support exist; such as the support for ARM32/ARM64 or future COM support).

[AArnott]

We might modify the projection to emit the low-level primitive types for the extern private method and then wrap it with the typedef-aware types in a more exposed method.

[AArnott]

Reading over this again, it's not clear to me what is or may be broken. Can you point to anything in particular @tannergooding?

[tannergooding]

@AArnott, basically for instance methods (which namely occur in COM scenarios) struct Wrapper { int value; } is not compatible with int when you are talking about returns.
This means, for example, that you cannot return a HRESULT wrapper for something like QueryInterface because it is fundamentally incompatible.

This means that COM wrappers you generate need to handle this difference as a type of "return fixup". For example:

public HRESULT QueryInterface([NativeTypeName("const IID &")] Guid* riid, [NativeTypeName("void **")] void** ppvObject)
{
    int result = ((delegate* unmanaged<ID3D12Device*, Guid*, void**, int>)(lpVtbl[0]))((ID3D12Device*)Unsafe.AsPointer(ref this), riid, ppvObject);
    return new HRESULT(value);
}

is necessary for the cpp signature:

virtual HRESULT STDMETHODCALLTYPE QueryInterface( 
    /* [in] */ REFIID riid,
    /* [iid_is][out] */ _COM_Outptr_ void __RPC_FAR *__RPC_FAR *ppvObject) = 0;

There are no known calling conventions that hit this for non-instance methods.

Additionally, there is a known issue in the handling of struct returns in general that require an additional return fixup. .NET 6 is getting a new CallConvMemberFunction modifier that will help with this scenario, but it will be .NET 6 only. For example:

public D3D12_RESOURCE_ALLOCATION_INFO GetResourceAllocationInfo([NativeTypeName("UINT")] uint visibleMask, [NativeTypeName("UINT")] uint numResourceDescs, [NativeTypeName("const D3D12_RESOURCE_DESC *")] D3D12_RESOURCE_DESC* pResourceDescs)
{
    D3D12_RESOURCE_ALLOCATION_INFO result;
    return *((delegate* unmanaged<ID3D12Device*, D3D12_RESOURCE_ALLOCATION_INFO*, uint, uint, D3D12_RESOURCE_DESC*, D3D12_RESOURCE_ALLOCATION_INFO*>)(lpVtbl[25]))((ID3D12Device*)Unsafe.AsPointer(ref this), &result, visibleMask, numResourceDescs, pResourceDescs);
}

is necessary for the cpp signature:

virtual D3D12_RESOURCE_ALLOCATION_INFO STDMETHODCALLTYPE GetResourceAllocationInfo( 
    _In_  UINT visibleMask,
    _In_  UINT numResourceDescs,
    _In_reads_(numResourceDescs)  const D3D12_RESOURCE_DESC *pResourceDescs) = 0;

While on .NET 6, it can just be:

public D3D12_RESOURCE_ALLOCATION_INFO GetResourceAllocationInfo([NativeTypeName("UINT")] uint visibleMask, [NativeTypeName("UINT")] uint numResourceDescs, [NativeTypeName("const D3D12_RESOURCE_DESC *")] D3D12_RESOURCE_DESC* pResourceDescs)
{
    return ((delegate* unmanaged[CallConvMemberFunction]<ID3D12Device*, uint, uint, D3D12_RESOURCE_DESC*, D3D12_RESOURCE_ALLOCATION_INFO>)(lpVtbl[25]))((ID3D12Device*)Unsafe.AsPointer(ref this), visibleMask, numResourceDescs, pResourceDescs);
}

[AArnott]

Thanks. At the moment our COM "interfaces" are actually structs. Do we have a problem now that there are no "instance" methods? Or maybe by instance methods you refer to the fact that they are implemented on the COM object rather than that they are declared on a .NET interface. The struct method looks like this:

internal unsafe HRESULT QueryInterface(global::System.Guid*riid, void **ppvObject)
{
    fixed (IUnknown*pThis = &this)
        return lpVtbl->QueryInterface_1(pThis, riid, ppvObject);
}

Where the vtbl is defined like this:

private struct Vtbl
{
    internal delegate *unmanaged[Stdcall]<IUnknown*, global::System.Guid*, void **, HRESULT>QueryInterface_1;
    internal delegate *unmanaged[Stdcall]<IUnknown*, uint>AddRef_2;
    internal delegate *unmanaged[Stdcall]<IUnknown*, uint>Release_3;
}

Also: when we migrate to proper interfaces, I won't be able to implement the method in order to wrap it as you have done here. But would declaring the interface like this work? If so, I can expose the method as returning HRESULT rather than int which would really help.

[Guid("B6FD0B71-E2BC-4653-8D05-F197E412770B")]
interface ISpellChecker
{
    [PreserveSig]
    [return: MarshalAs(UnmanagedType.U4)]
    HRESULT get_LanguageTag([MarshalAs(UnmanagedType.LPWStr)] out string value);
}

[tannergooding]

Or maybe by instance methods you refer to the fact that they are implemented on the COM object rather than that they are declared on a .NET interface

Correct

The struct method looks like this:

I believe this is problematic and may result in incorrect handling.

Also: when we migrate to proper interfaces

I believe this "works" due to legacy handling in the JIT that had to be preserved for back-compat.

@AaronRobinsonMSFT may be able to give a more definitive statement about what works due to back compat, what is expected to break because the ABIs differ, and what will require CallConvMemberFunction to avoid needing a fixup handler.

The handling may also differ for Mono as compared to RyuJIT.

[tannergooding]

@AaronRobinsonMSFT, In particular, I think the two important scenarios that need comment are:

• Native returns a primitive, but managed returns a struct wrapper
  • I know this "works" in some scenarios for back-compat
• Native returns a struct
  • I know this requires fixups or CallConvMemberFunction, I think in all scenarios

[AaronRobinsonMSFT]

The struct method looks like this:
I believe this is problematic and may result in incorrect handling.

Ugh... I really wish this pattern could be excised from all COM interop. This is likely okay because we special case returning value types that are <= 8 bytes if I recall. We did this because when we tried to fix it, this incorrect pattern has proliferated so far that we broke WPF - see dotnet/coreclr#23974 - and thus put it back in.

• Native returns a struct
  • I know this requires fixups or CallConvMemberFunction, I think in all scenarios

Except for the size cases mentioned above the special handling @tannergooding mentions is needed without the attribute.

/cc @jkoritzinsky

Edit Update the <= 8 bytes.

[AArnott]

This is likely okay because we special case returning values types that are less than 8 bytes

What about exactly 8 bytes (like an IntPtr in 64-bit processes)? Is that not safe to return as a struct that wraps a single 64-bit field then?

is needed without the attribute

Does that mean that if I add [return: MarshalAs(UnmanagedType.U4)] but the return type is HRESULT with an int inside I'm ok?
None of the MarshalAs options allow me to set "pointer sized", so I don't know what I would put for a HANDLE type.

[AaronRobinsonMSFT]

What about exactly 8 bytes (like an IntPtr in 64-bit processes)? Is that not safe to return as a struct that wraps a single 64-bit field then?

Oops, sorry about that. I should have been more precise here. I meant "less than or equal to 8 bytes" - I've updated the comment. A special note is that IntPtr is not a struct at the IL level and handled as an intrinsic (i.e. native int).

Does that mean that if I add [return: MarshalAs(UnmanagedType.U4)] but the return type is HRESULT with an int inside I'm ok?

Hmmm. I honestly don't know how that would fall out. You don't need the MarshalAs for sure, but if it is applied I am not sure how we would handle that. It wouldn't surprise me if it just works because of how we special case these smaller value types. I would need to do some experimentation to say for sure.

None of the MarshalAs options allow me to set "pointer sized", so I don't know what I would put for a HANDLE type.

This should be similar to the struct scenario for sizes <= 8.

[AArnott]

Thanks, @AaronRobinsonMSFT. It sounds like we have nothing to worry about then, since all our typedef structs that may be returned are never more than 8 bytes in length.