Ensure wrapper structs are generated in a safe and compatible way about cswin32 HOT 10 CLOSED

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.

from cswin32.

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

from cswin32.

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.

from cswin32.

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

from cswin32.

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

from cswin32.

@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

from cswin32.

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.

from cswin32.

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.

from cswin32.

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.

from cswin32.

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.

from cswin32.