COM interfaces returning structs use wrong calling convention

[weltkante]

Some COM interfaces return structs instead of HRESULTs, for example the Direct2D and Direct3D12 APIs have this, some examples:

• DXGI_RGBA ID2D1SolidColorBrush::GetColor()
• D2D_SIZE_U ID2D1RenderTarget::GetPixelSize()
• D3D12_RESOURCE_DESC ID3D12Resource::GetDesc()

Historically .NET only implements the stdcall calling convention which is what is used for P/Invoke methods but is not the calling convention used by COM interfaces, which instead uses a stdcall+thiscall combination. This is (probably?) mostly identical to stdcall but (at least) differs in how structs are returned, stdcall seems to return them in a register while stdcall+thiscall returns them on the stack. See dotnet/coreclr#23974 for a technical discussion.

When you are doing the projection from the metadata you can only return structs using [NativeTypedef] over primitive types (like integers) from a COM interface. If you need to return a "real" struct then you need to actually return it via ref or out to simulate what the calling convention does.

There's been desire to add the proper calling convention to the dotnet runtime (dotnet/runtime#46775) but its not finished and not available on older versions, so unless you want to reject access to these APIs you'll have to work around the shortcomings of the runtime. (Also when this calling convention gets in, I don't know how it will handle HRESULT structs, which aren't actually structs. If you have no way to signify its to be treated differently you have the reverse problem, returning HRESULT on the stack instead of via register.)

PS: I'm no expert on calling conventions and only aware of this because I got broken by it, so you may want to talk to some of the runtime guys who (hopefully) know the details of this calling convention better.

[AArnott]

This sounds very similar to #51, except that was closed because we were discussing just the typedef structs.
I expect this will resolve itself with #26 when we change from native function pointers to real interfaces anyway. But I'm really surprised if none of the existing calling conventions work for COM:

@AaronRobinsonMSFT can you comment?

[AaronRobinsonMSFT]

This sounds very similar to #51, except that was closed because we were discussing just the typedef structs.

@AArnott I missed that being closed - it really shouldn't have been. @tannergooding's point was that the exact issue reported here needs to be handled manually - see #51 (comment) where HRESULT was used as an example. I do admit the subsequent conversation diverged to the narrow topic of a few cases that are special cased but not to the underlying issue. That issue is that the instance member ABI on Windows is non-conforming and needs to be special cased as mentioned in the referenced comment.

I expect this will resolve itself with #26 when we change from native function pointers to real interfaces anyway.

That is possible but only if special casing is performed.

But I'm really surprised if none of the existing calling conventions work for COM:

That is the reason CallConvMemberFunction was proposed, it is a long standing missing gap. Now that .NET has become cross-platform and COM style interfaces are used in cross-platform cases the built-in implementation details need to be exposed.

The DirectX and Direct2D interfaces are notoriously bad in this respect and are common offenders - see dotnet/runtime#10901. All source generation tools (e.g., SharpGenTools) need to manually handle returning structs (> 8 bytes in size) because of this case. We attempted to do the right thing here, but found that WPF and many others have been relying on the offending pattern of returning a wrapped int in a struct so had to back it out. Unfortunately we couldn't have it both ways - follow the ABI and permit the offending pattern.

/cc @jkoritzinsky

[jkoritzinsky]

The correct solution here would be to do a two-pronged approach:

For the typedef structs, you can either do one of the two following options:

• Use the underlying type as the return type for the function pointer. This will enable the correct results if the generated code ever uses CallConvMemberFunction.
• Use the typedef type as the return type. This ends up using the "non-member function" calling convention, which will work in this case. This will continue to work if the code-gen uses the built-in COM support as well (this is the legacy behavior Aaron mentioned), but not if the code ever uses CallConvMemberFunction.

For the non-typedef structs, you can do one of the two following options:

• Manually transform the function pointer/COM method signature to have an out return buffer parameter after the native this parameter and change the method to return void. This will be necessary in the "pointer-free" mode you mentioned. The built-in COM system will never support using the natural signature for this case because it breaks the other case pretty severely.
• Use CallConvMemberFunction.

We're introducing CallConvMemberFunction to enable using the natural function signature with member functions that return structs since we can't change the built-in behavior without breaking people that rely on the "feature" that their HRESULT struct type can be used in place of an HRESULT.

[tannergooding]

@jkoritzinsky, it's worth noting that this is compatible but also not quite right either:

change the method to return void

The underlying ABI also returns a pointer to the return buffer parameter and so for a method such as:

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

The fixup on C# should/can be:

public D3D12_RESOURCE_ALLOCATION_INFO GetResourceAllocationInfo(uint visibleMask, uint numResourceDescs, 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);
}

This is also slightly more convenient than having an additional return statement that is effectively return result.
You can see an example of this behavior here: https://godbolt.org/z/4EofM4 (and can also go browse the MSVC source if needed).

[AArnott]

Thanks for jumping in, folks.

But my head explodes trying to grok all this at once. ABIs are not by strongpoint. So let's simplify it by setting up some rules:

1. We need a solution that works today for no later than C# 9 and works on .NET Framework as well as .NET Core. This means I am not at all interested in discussing CallConvMemberFunction, which does not exist.
2. I want a solution that works on Windows. I don't care about other OSs.
3. I sorta-maybe care about theoretical future CPU architectures that Windows may someday run on. But if we don't run on them today, let's call out which problems are real problems vs which are theoretical.

@tannergooding's point was that the exact issue reported here

That's not how I read it. #51 was explicitly opened about the typedef structs, all of which are 8 bytes or less.
This issue is about other custom structs which are often larger than 8 bytes.
@AaronRobinsonMSFT said in #51 (comment) that (if I understood correctly) we have nothing to worry about for 8 byte or smaller structs. So if that's true, typedef structs are not an issue, but larger ones are. Did I misunderstand?

We attempted to do the right thing here, but found that WPF and many others have been relying on the offending pattern of returning a wrapped int in a struct so had to back it out. Unfortunately we couldn't have it both ways - follow the ABI and permit the offending pattern.

That sounds like it is a pattern that worked in the past and that you won't break it the future. Doesn't it?

All source generation tools (e.g., SharpGenTools) need to manually handle returning structs (> 8 bytes in size) because of this case.

Are you saying that COM interfaces that define methods that return large structs are all broken? How could that be? Is it that most COM methods actually return HRESULT and use an out parameter for their more meaningful structs that I never noticed that larger returned structs fail? And how does non-PreserveSig method definitions work then?

For the non-typedef structs, you can do one of the two following options:

• Manually transform the function pointer/COM method signature to have an out return buffer parameter after the native this parameter and change the method to return void. This will be necessary in the "pointer-free" mode you mentioned. The built-in COM system will never support using the natural signature for this case because it breaks the other case pretty severely.

I'm going to ignore your second "option" because it involves an attribute that does not exist. :)
What if I'm not using function pointers and the method isn't on a COM interface?

@tannergooding Your "fixup" method makes my jaw drop. I appreciate that it looks like something that fits my above criteria, but I'm imagining explaining to customers why the codegen looks like nothing they have ever written (and wasn't even legal before C# 9) while they have been calling into DirectX for years from C# without it. If they can't understand the p/invoke code we emit, they'll be less likely to trust it.

So I guess my high level questions are:

1. How is it possible that the only fixes for this either do not exist (the attribute) or require C# 9 syntax, yet we've been p/invoking into these APIs for over a decade? I'll need this to document to customers why we do it.
2. What must I do to operate correctly under the scenarios I list at the top of this comment?

Thanks for your patience as I struggle to get my head around this.

[tannergooding]

I appreciate that it looks like something that fits my above criteria, but I'm imagining explaining to customers why the codegen looks like nothing they have ever written (and wasn't even legal before C# 9) while they have been calling into DirectX for years from C# without it. If they can't understand the p/invoke code we emit, they'll be less likely to trust it.

The only reason it looks like nothing they've ever written is because it uses function pointers.

If this were instead a method in a C# COM interface, it would look something like:

[Guid("...")]
[InterfaceTypeAttribute(ComInterfaceType.InterfaceIsIUnknown)]
public interface ID3D12Device : ID3D12Object
{
    // More Methods

    [PreserveSig]
    D3D12_RESOURCE_ALLOCATION_INFO* GetResourceAllocationInfo(D3D12_RESOURCE_ALLOCATION_INFO* result, uint visibleMask, uint numResourceDescs, D3D12_RESOURCE_DESC* pResourceDescs);

    // More Methods
}

The function pointer merely mirrors this for the actual native call and the wrapper mirrors what the exposed C++ API looks like (which doesn't have a result parameter and which returns D3D12_RESOURCE_ALLOCATION_INFO, not D3D12_RESOURCE_ALLOCATION_INFO*).

[tannergooding]

How is it possible that the only fixes for this either do not exist (the attribute) or require C# 9 syntax, yet we've been p/invoking into these APIs for over a decade? I'll need this to document to customers why we do it.

There aren't many COM APIs that return something other than HRESULT and fewer that don't return what is effectively a primitive wrapper struct.

For the cases that do exist, there are alternatives available both with regular delegates and with COM interface definitions.

What must I do to operate correctly under the scenarios I list at the top of this comment?

Using function pointers in the wrapper methods for the struct scenario and a method like what I gave above for the COM interface scenario are the two best options.

The only catch with function pointers is that they are not considered blittable pre .NET 5 and so you can't have them as fields in a struct or passed directly as method parameters.
Using them within a method body, like the wrapper I showed, is fine.

[weltkante]

@AaronRobinsonMSFT said in #51 (comment) that (if I understood correctly) we have nothing to worry about for 8 byte or smaller structs. So if that's true, typedef structs are not an issue, but larger ones are. Did I misunderstand?

Yes, he probably was talking about the inverse case, i.e. you have nothing to worry for small C# structs when in C++ they are primitives (i.e. the HRESULT case). The behavior of C++ (i.e. the ABI) and C# is asymetric here.

• C++ makes a difference between returning primitive types and structs (at least for small sizes)
• C# historically didn't differ (at least for small sizes), allowing you to use HRESULT structs instead of integers

Maybe for large sizes everything is on the stack, regardless of being a struct or primitive? Not sure about it.

Regardless, the current projection is definitely broken, the native ABI definitely makes a distinction between small primitive types and small structs (not sure about what the ABI is for large primitives, never seen a COM method return a primitive larger than 8 bytes). I've seen the exact projection you are doing with a different Direct2D projection and it broke, which is why I'm aware of the problem, and I'd like to DirectX/Direct2D be usable with cswin32. If you want a test case its probably easy to construct one.

How is it possible that the only fixes for this either do not exist (the attribute) or require C# 9 syntax, yet we've been p/invoking into these APIs for over a decade?

Its the newer Direct2D / DirectX APIs which started to return structs, older APIs never did this, or were so obscure that people weren't invoking them from .NET. When Direct2D started to become popular people were running into problems, which is why we are aware of them and support is being worked on.

You can work around it with older syntax I believe, you can pass the return struct as ref/out argument and declare the return value as IntPtr? You just need to adjust the method signature instead of translating it literally from the C++ header.

[tannergooding]

not sure about what the ABI is for large primitives, never seen a COM method return a primitive larger than 8 bytes

D3D12 has a few, one of which is the GetResourceAllocationInfo example I keep giving. D3D12_RESOURCE_ALLOCATION_INFO is a 16-byte struct containing 2x UINT64 fields.
You can find many examples (currently 107) of these types of fixups by grepping for result; in my TerraFX.Interop.Windows project: https://github.com/terrafx/terrafx.interop.windows/tree/main/sources/Interop/Windows, https://source.terrafx.dev/#TerraFX.Interop.Windows/um/d3d12/ID3D12Device.cs,8b9b043be241c1ea. I've provided my own grep results here for convenience: result.txt

Edit: I misread the comment as struct, not primitive. As far as the ABI is concerned, things like __int128 aren't true primitives but rather their own type. __m128 and friends are primitives and respected by the ABI.
These correspond to Vector128<T> and friends in .NET and are only available in .NET Core 3.0+. They are currently blocked in P/Invokes because we don't correctly handle the return scenario on Windows.

[AArnott]

What must I do to operate correctly under the scenarios I list at the top of this comment?

Using function pointers in the wrapper methods for the struct scenario and a method like what I gave above for the COM interface scenario are the two best options.

Thank you. That's a very simple answer that I think I can work with. :)

The only catch with function pointers is that they are not considered blittable pre .NET 5 and so you can't have them as fields in a struct or passed directly as method parameters.

I already do though. All our COM structs define vtbl's with function pointers as fields, and they work on net472.
I did notice that on .NET Framework stackalloc PWSTR[1] fails, which is very interesting (no function pointers either), but changing it to new PWSTR[1] solved that problem.

[tannergooding]

All our COM structs define vtbl's with function pointers as fields, and they work on net472.

Do you have an example of what is being generated? I recall fairly simple examples showing this fails.

[jkoritzinsky]

Just hopping back in to clarify one response.

@AaronRobinsonMSFT said in #51 (comment) that (if I understood correctly) we have nothing to worry about for 8 byte or smaller structs. So if that's true, typedef structs are not an issue, but larger ones are. Did I misunderstand?

We do have to worry about structs of all sizes. Here's a modified example that we use in the CoreCLR test tree:

struct IntWrapper { int i; };

struct S
{
     virtual int __stdcall GetI()
     {
           return 0;
     }
     virtual IntWrapper __stdcall GetIWrapper()
     {
           return {0};
     }
};

The C-style signature of S::GetI is int S_GetI(S*). The C-style signature of GetIWrapper is IntWrapper* S_GetIWrapper(S*, IntWrapper*).

Even though IntWrapper is less than 8-bytes, the fact that it is a non-primitive type causes MSVC to make it use a return buffer. So even structs of smaller than 8 bytes need to have their native signature transformed.

As we've chatted about, typedefs do not have this issue. A typedef of a primitive (for example HRESULT), counts as a primitive for the ABI.

[AArnott]

All our COM structs define vtbl's with function pointers as fields, and they work on net472.

Do you have an example of what is being generated? I recall fairly simple examples showing this fails.

Sure, @tannergooding. This struct ends up being a field member of another struct:

private struct Vtbl
{
    internal delegate *unmanaged[Stdcall]<ISpellCheckerFactory*, global::System.Guid*, void **, HRESULT>QueryInterface_1;
    internal delegate *unmanaged[Stdcall]<ISpellCheckerFactory*, uint>AddRef_2;
    internal delegate *unmanaged[Stdcall]<ISpellCheckerFactory*, uint>Release_3;
    internal delegate *unmanaged[Stdcall]<ISpellCheckerFactory*, IEnumString**, HRESULT>get_SupportedLanguages_4;
    internal delegate *unmanaged[Stdcall]<ISpellCheckerFactory*, PCWSTR, bool *, HRESULT>IsSupported_5;
    internal delegate *unmanaged[Stdcall]<ISpellCheckerFactory*, PCWSTR, ISpellChecker**, HRESULT>CreateSpellChecker_6;
}

And the wrapping struct then is used with a pointer:

CoCreateInstance(
    typeof(SpellCheckerFactory).GUID,
    null,
    (uint)CLSCTX.CLSCTX_INPROC_SERVER, // https://github.com/microsoft/win32metadata/issues/185
    typeof(ISpellCheckerFactory).GUID,
    out ISpellCheckerFactory* spellCheckerFactory).ThrowOnFailure();

[AaronRobinsonMSFT]

internal delegate *unmanaged[Stdcall]<ISpellCheckerFactory*, PCWSTR, bool *, HRESULT>IsSupported_5;

@AArnott This is a bit dangerous. According to the doc, that bool* is actually a BOOL* so it is technically pointing to 4 bytes not 1. This will likely be okay unless the API follows the loose definition of true which is simply non-zero. I've encountered Win32 APIs over the years that follow that and return non-zero values that are greater than 1 to indicate true which means if any implementation decides to write to more than a single byte memory corruption will result.

[weltkante]

which means if any implementation decides to write to more than a single byte memory corruption will result.

Isn't this every implementation then? Writing void f(BOOL *p) { *p = TRUE; } in C/C++ should always write the full 4 bytes? If the C# bool is packed tightly this can always overwrite adjacent memory, I think this is another one of the cases where ref bool has worked in classic marshaling (being marshaled as 4 bytes) but pointers change the size/alignment (the side discussion started in #108)

(Of course besides potential memory corruption there's also the case of missing high bits, writing 0x100 as "true" value would not be visible as nonzero when read from a pointer-to-bool in C#, assuming the memory corruption was swallowed by padding)

[AaronRobinsonMSFT]

Isn't this every implementation then? Writing void f(BOOL *p) { *p = TRUE; } in C/C++ should always write the full 4 bytes?

@weltkante Oh gosh. Yes you're right. Since BOOL is defined as 4 bytes on Windows the full 4 bytes will always be written and potentially cause issues.

[AArnott]

As we've chatted about, typedefs do not have this issue. A typedef of a primitive (for example HRESULT), counts as a primitive for the ABI.

@jkoritzinsky What's the difference between HRESULT and your example struct that has the problem? Sure, on the native side HRESULT is a typedef, but on the managed side, it's just another struct. Are you saying that while .NET will turn our HRESULT into a pointer, that's ok because the native side uses a pointer too?

[AArnott]

Ok, I'll update our native function pointers to not use bool.

[jkoritzinsky]

@AArnott you're close, but not quite right.

Let's take my example again, with some modifications. For the following C++:

typedef int HRESULT;

struct IntWrapper { int i; };

struct S
{
     virtual int __stdcall GetI()
     {
           return 0;
     }
     virtual HRESULT __stdcall GetHRESULT()
     {
           return 0;
     }
     virtual IntWrapper __stdcall GetIWrapper()
     {
           return {0};
     }
};

I believe CsWin32 would generate the following vtable signatures and structs, or something similar to it:

struct HRESULT
{
     int value;
}

struct IntWrapper
{
	int value;
}

struct S
{
	struct Vtable
	{
		delegate* unmanaged[Stdcall]<S*, int> GetI;
		delegate* unmanaged[Stdcall]<S*, HRESULT> GetHRESULT;
		delegate* unmanaged[Stdcall]<S*, IntWrapper> GetIWrapper;
	}
}

However, here's the equivalent signatures at the C level of the C++ methods:

typedef int HRESULT;

struct IntWrapper { int i; };

int S_GetI(S* _this);
HRESULT S_GetHRESULT(S* _this);
IntWrapper* S_GetIWrapper(S* _this, IntWrapper* retbuf);

As you can see, the native C++ compiler does not generate a return buffer for a typedef of a primitive return from a member function (it's important to remember this is member-function only). It only generates a return buffer in the case of the return type being a struct of any size.

Now, for the .NET side, .NET will generate code assuming that any struct of size 1, 2, 4, or 8 bytes will be returned in registers, the same rules as non-member functions.

Let's look at each of the 3 methods on S one by one and compare the behavior.

GetI

On the C++ side, GetI returns a primitive int. The C++ compiler will generate code that returns it in a register.

On the .NET side, GetI returns a type that is 4 bytes. As a result, .NET will generate code assuming the callee returns the result in a register.

GetHRESULT

On the C++ side, GetHRESULT returns the type HRESULT. This type is a typedef for the int type, so the return type is still a primitive int with a different name. The C++ compiler will generate code that returns it in a register.

On the .NET side, GetHRESULT returns a type (the managed HRESULT struct) that is 4 bytes. As a result, .NET will generate code assuming the callee returns the result in a register.

Since the managed HRESULT struct is 4 bytes, .NET accidentally gets the behavior right, since the native typedef HRESULT is treated the same as the underlying int type. This accidental success is the "back-compat" case mentioned previously.

GetIWrapper

On the C++ side, GetIWrapper returns a struct. As a result, C++ will insert a return buffer after the native this arg and will emit code that returns the return buffer address.

On the .NET side, GetIWrapper returns a type that is 4 bytes. As a result, .NET will generate code assuming the callee returns the result in a register.

This is the failure case. C++ assumes it will be passed a return buffer and returns a return buffer, but .NET assumes that it does not need to pass a return buffer and that it will receive the result from the return register(s).

In this case, CsWin32 will need to manually adjust the signature to insert the return buffer since .NET will not do the correct thing without the new CallConvMemberFunction type.

[AArnott]

In this case, CsWin32 will need to manually adjust the signature to insert the return buffer since .NET will not do the correct thing without the new CallConvMemberFunction type.

Ok, so I need to modify the return type to be a pointer, and I need to add that same pointer type as an additional parameter in the method as well (always in last position).
And I do this when the return type is a struct that wasn't a typedef on the native side. For COM interface methods only.
Right?

[tannergooding]

.NET accidentally gets the behavior right, since the native typedef HRESULT is treated the same as the underlying int type. This accidental success is the "back-compat" case mentioned previously.

@jkoritzinsky, do we know if the accidental success here is RyuJIT/Legacy JIT only or if it also applies to Mono and Mono/AOT?

[jkoritzinsky]

In this case, CsWin32 will need to manually adjust the signature to insert the return buffer since .NET will not do the correct thing without the new CallConvMemberFunction type.

Ok, so I need to modify the return type to be a pointer, and I need to add that same pointer type as an additional parameter in the method as well (always in last position).

You'll need to add the additional parameter after the this parameter and before all other parameters.

And I do this when the return type is a struct that wasn't a typedef on the native side. For COM interface methods only.
Right?

Yes.

.NET accidentally gets the behavior right, since the native typedef HRESULT is treated the same as the underlying int type. This accidental success is the "back-compat" case mentioned previously.

do we know if the accidental success here is RyuJIT/Legacy JIT only or if it also applies to Mono and Mono/AOT?

I do not know if Mono has the same problem with their COM Interop support, but I wouldn't be shocked if it did. I haven't had a chance to test it yet. They'll definitely have the same problem with function pointers though.

[ghord]

For people (like me) that are searching for a performant workaround for the issue, you can disable COM mashaling and manually use new MemberFunction calling convention which resolves the issue:

private unsafe D3D12_CPU_DESCRIPTOR_HANDLE GetDescriptorHandle(ID3D12DescriptorHeap* heap)
{
    var vtable = *(nint**)heap;
    var getDescriptor = (delegate* unmanaged[Stdcall, MemberFunction]<ID3D12DescriptorHeap*, D3D12_CPU_DESCRIPTOR_HANDLE>)(vtable[9]); // index in vtable needs to be found by inspecting code generated by cswin32
    var getDescriptorResult = getDescriptor(heap);
    return getDescriptorResult;
}

[JeremyKuhne]

I'm hitting this as well now.

// ID2D1RenderTarget::GetSize()

// What is generated:
public winmdroot.Graphics.Direct2D.Common.D2D_SIZE_F GetSize()
{
	return ((delegate *unmanaged [Stdcall]<ID2D1RenderTarget*,winmdroot.Graphics.Direct2D.Common.D2D_SIZE_F>)lpVtbl[53])((ID2D1RenderTarget*)Unsafe.AsPointer(ref this));
}

// What actually works:
D2D_SIZE_F result;
return *((delegate* unmanaged<ID2D1RenderTarget*, D2D_SIZE_F*, D2D_SIZE_F*>)lpVtbl[53])((ID2D1RenderTarget*)Unsafe.AsPointer(ref this), &result);

// OR

return ((delegate* unmanaged[Stdcall, MemberFunction]<ID2D1RenderTarget*, D2D_SIZE_F>)lpVtbl[53])((ID2D1RenderTarget*)Unsafe.AsPointer(ref this));

@jkoritzinsky I presume it would be safe to simply always emit [Stdcall, MemberFunction] for all COM APIs?

@AArnott if this is true, can we emit this when targeting .NET 6+?

[jkoritzinsky]

@JeremyKuhne yes, using [Stdcall, MemberFunction] is always correct for COM APIs.

[AArnott]

I suppose I can only add that syntax in the struct-based approach, where I actually handle the function calls natively, right? Is this a problem for folks where CsWin32 just declares the interfaces?

[weltkante]

yes, when the C++/Headers return a struct the interface (without stdcall/memberfunction) would need to return it as out parameter in last position to get the ABI right (returning the result via register vs. the stack) if you are supporting classic COM interop - this was done because (among others) WPF is returning a HRESULT struct commonly and "fixing" .NET to use stdcall/memberfunction by default breaks the common usecase of wrapping such returns like HRESULT in a struct. This was fixed in the past, regressed people, so rolled back to how Desktop Framework behaves. Net result is that C++ struct returns need to be special cased in COM APIs (and for Desktop Framework it always was this way anyways).

The question is whether you can differentiate struct returns from typedef returns in your API source from which you generate, I haven't looked at it for a while since I went back to transcribing APIs manually again. HRESULT would be a typedef and returned via register as if it were just an integer, even if C# defines a wrapper struct for it, while D3D_SIZE_F above is a "real" C++ struct and need to be returned on the stack (out parameter or stdcall/memberfunction convention)

[JeremyKuhne]

So anything that has [NativeTypedef] on the return needs to stay as is.

From the winmd:

// Windows.Win32.Foundation.HRESULT
using Windows.Win32.Foundation.Metadata;

[NativeTypedef]
public struct HRESULT
{
    public int Value;
}

// Windows.Win32.Graphics.Direct2D.Common.D2D_SIZE_F
using Windows.Win32.Foundation.Metadata;

public struct D2D_SIZE_F
{
    public float width;
    public float height;
}

So if it is a class member that is StdCall (almost all cases), as long as it doesn't return a struct with [NativeTypeDef] it should be [Stdcall, MemberFunction]. Otherwise it just stays [StdCall].

I think this also means that we'd likely want to put [NativeTypedef] on the generated structs. Might be useful for other scenarios too.

[tannergooding]

@JeremyKuhne yes, using [Stdcall, MemberFunction] is always correct for COM APIs.

@jkoritzinsky its worth noting this isn't actually the case, unfortunately 😢

COM APIs are instance members and so MemberFunction is needed. Additionally, most COM APIs are explicitly exported as __stdcall and so [StdCall, MemberFunction] is correct.

However, there do exist some COM APIs which are not explicitly __stdcall. For example, some of these are on IDebugControl in um/DbgEng.h which use STDMETHODV instead (this is defined as virtual COM_DECLSPEC_NOTHROW HRESULT STDMETHODVCALLTYPE method in um/combaseapi.h, where STDMETHODVCALLTYPE is __cdecl in um/winnt.h).

Some other files that includes such COM methods are um/MethodCo.h, um/MspAddr.h, um/MspCall.h, um/MspStrm.h, uim/Mspterm.h, um/Msptrmac.h, um/Msptrmar.h, um/Msptrmvc.h, um/Provider.h, um/TextServ.h, um/axextend.idl, um/baseaudioprocessingobject.h, um/dxcapi.h, um/shdeprecated.h, um/strmif.h, um/vswriter.h, and um/xapobase.h.

There's even one that uses __fastcall in um/baseaudioprocessingobject.h

[tannergooding]

yes, when the C++/Headers return a struct the interface (without stdcall/memberfunction) would need to return it as out parameter in last position to get the ABI right

@weltkante, it's worth noting this isn't correct. Struct returns are handled as a hidden "out parameter" yes, but it is not the last parameter positition, but rather the last implicit parameter. That is, it proceeds the implicit this and precedes all arguments.

Something like:

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

Is therefore with a correct binding:

delegate* unmanaged[StdCall, MemberFunction]<
    ID3D12Device*,    // implicit this
    uint,    // visibleMask
    uint,    // numResourceDescs
    D3D12_RESOURCE_DESC*,    // pResourceDescs
    D3D12_RESOURCE_ALLOCATION_INFO    // return value
>;

or with an emulated binding:

delegate* unmanaged[StdCall]<
    ID3D12Device*,    // implicit this
    D3D12_RESOURCE_ALLOCATION_INFO*, // implicit return buffer
    uint,    // visibleMask
    uint,    // numResourceDescs
    D3D12_RESOURCE_DESC*,    // pResourceDescs
    D3D12_RESOURCE_ALLOCATION_INFO*    // return value, this is the same as the implicit return buffer passed in
>

Noting that the emulated binding is technically only guaranteed to be correct for x86/x64 Windows. There are platforms, such as Arm64, where this can differ and where the emulated transform can break due to other underlying ABI requirements.

[JeremyKuhne]

@tannergooding it looks like the winmd might not contain this info (or I'm missing something).

[tannergooding]

Which info in particular?

[JeremyKuhne]

The calling convention.

[weltkante]

So if it is a class member, as long as it doesn't return a struct with [NativeTypeDef] it should be [Stdcall, MemberFunction]. Otherwise it just stays [StdCall].

Right, your requirement sounded backwards, because obviously the C++ ABI will always be [StdCall, MemberFunction], but if you're returning a wrapper struct with NativeTypeDef you have to "lie" about the ABI. Should be safe to do so since (as far as I know) the only differnce between the both is how structs are returned. [edit] see posts below, should be safe for x86/x64 since the two calling conventions only differ in how they treat struct returns as far as I know, but might not be safe for new platforms like arm64.

The "correct" way (which is slightly more work) is to not lie about the ABI and never return a typedef as struct on the ABI layer, instead unwrap it to the underlying member field and e.g. Unsafe.As cast it so the ABI sees the same type as C++

However, there do exist some COM APIs which are not explicitly __stdcall.

Yes, those calling conventions would have to explicitly be annotate per-method in the source dataset, there even exist interfaces which have some members in one and some in another calling convention.

[weltkante]

it looks like the winmd might not contain this info (or I'm missing something).

I made an issue to them to include these annotations I believe, when I stumbled upon one of these interfaces: microsoft/win32metadata#1053

They wanted to annotate any deviations from [StdCall, MemberFunction] if I read that right. If they are missing some interface member annotations it should be easy to get them added by making a similar request.

[tannergooding]

Should be safe to do so since (as far as I know) the only differnce between the both is how structs are returned.

I would not rely on this, as it can break on any future platforms. Doing this is effectively relying on an implementation detail of the x64 platform and historical JIT support. Platforms such as Arm64 or theoretical future platforms such as RISC-V or LoongArch can break from doing this (just as emulating the correct x64 signature does already break on Arm64 for some cases today).

It's better to simply ensure your function pointers and P/Invokes are 1-to-1 to prevent any potential breaks (it's the only surefire correct thing). You can hide the direct P/Invoke and expose a wrapper which only lets users see the nice HRESULT wrapper struct if that is the surface area you want to expose.

[tannergooding]

That being said, if the runtime opted to support [TransparentStruct] attribute I've proposed/discussed in various issues (basically extending the support it already has for CLong, CULong, and NFloat to user-defined types wrapping a single field), this would become infinitely simpler.

Dev's would not be forced to define such wrapper methods for trivial (and super common) helper types like this, allowing them to do the more optimal thing and allowing the runtime to handle the blittable unwrapping correctly (rust has this via [repr(transparent)]).

-- CC. @jkoritzinsky (I know I've bugged you on this a lot 😄)

[jkoritzinsky]

As I've mentioned before, as an interop-only feature it's too expensive for across the whole runtime ecosystem. We'd rather have a more generalized "transparent struct" feature instead that improves other areas of the runtime as well as interop scenarios.

We've added support for "transparent struct" specifically for the HResult scenario in LibraryImport/ComInterfaceGenerator through [MarshalAs(UnmanagedType.Error)] and using Unsafe.BitCast under the hood.

[tannergooding]

Could you elaborate a bit on what's expensive about it? We already "have" the necessary JIT support (because it's required for CLong, CULong, and NFloat and so already covers integer and floating-point primitives). So seemingly it'd just be recognize the attribute and changing the current hardcoded type checks to check for the attribute (or rather a cached flag indicating the attribute was set) instead.

[tannergooding]

Which is to say, I expect it's under 50 lines of code to enable for struct S { T Value; } where T is byte, sbyte, short, ushort, int, uint, long, ulong, nint, nuint, float, or double.

I could see some complexities for char or bool and I could see complexities for cases where users try to do more than 1 field or a non-primitive struct, but we already have logic to trivially throw a TypeLoadException (explicit layout with value type/reference type overlapping) for other similar invalid attributions and cases where we ignore it for invalid specifiers (incorrect packing specified), so we could just pick a right behavior and it still stays simple.

[AffluentOwl]

Based upon @tannergooding's suggestion, I've found the following workaround that still allows re-using as much of the CsWin32 generated code as possible, without having to find magic virtual table offsets in the c++ compiler kingdom and assembly fairyland. My limited testing did not seem to need the [PreserveSig()] attribute. I wish I could find a workaround to eliminate the unused functions.

    [Guid("8EFB471D-616C-4F49-90F7-127BB763FA51"), InterfaceType(ComInterfaceType.InterfaceIsIUnknown), ComImport()]
    internal interface ID3D12DescriptorHeapExtended : ID3D12DescriptorHeap
    {
      void Unused1();
      void Unused2();
      void Unused3();
      void Unused4();
      void Unused5();
      void Unused6();

      unsafe D3D12_CPU_DESCRIPTOR_HANDLE* GetCPUDescriptorHandleForHeapStart(D3D12_CPU_DESCRIPTOR_HANDLE* result);

      // Remaining unused functions unneeded
    }

EDIT: Don't forget to use PreserveSig

[AaronRobinsonMSFT]

@AffluentOwl You can play with the _VTblGap prefix for those functions if you'd like to remove them. This is how Embedded PIAs are implemented. In this case, you could redefine this to be:

    [Guid("8EFB471D-616C-4F49-90F7-127BB763FA51"), InterfaceType(ComInterfaceType.InterfaceIsIUnknown), ComImport()]
    internal interface ID3D12DescriptorHeapExtended : ID3D12DescriptorHeap
    {
      void _VTblGap_6(); // Represents 6 vtable slots

      unsafe D3D12_CPU_DESCRIPTOR_HANDLE* GetCPUDescriptorHandleForHeapStart(D3D12_CPU_DESCRIPTOR_HANDLE* result);

      // Remaining unused functions unneeded
    }

[AffluentOwl]

After 1 month of trying to figure out why my program crashes 50% of the time, turns out [PreserveSig()] is required whenever a COM function does not return an HRESULT. Otherwise whenever the pointer returned by GetCPUDescriptorHandleForHeapStart (which is also the same as the pointer passed in) has its 32nd bit set, a System.Runtime.InteropServices.COMException will be returned, because the return value is interpreted as a failing HRESULT value, SUCCEEDED(hr) fails, and thus an exception is thrown.

[AArnott]

That makes sense, since PreserveSig=false assumes an HRESULT return value, so if that assumption is false you must use PreserveSig=true. Sorry you had to find out the hard way though, @AffluentOwl.

[AaronRobinsonMSFT]

After 1 month of trying to figure out why my program crashes 50% of the time, turns out [PreserveSig()] is required whenever a COM function does not return an HRESULT. Otherwise whenever the pointer returned by GetCPUDescriptorHandleForHeapStart (which is also the same as the pointer passed in) has its 32nd bit set, a System.Runtime.InteropServices.COMException will be returned, because the return value is interpreted as a failing HRESULT value, SUCCEEDED(hr) fails, and thus an exception is thrown.

Right. The PreserveSig in this case will help the marshaller know that the value in the eax register isn't to be checked. If PreserveSig isn't present and returns void, then the runtime assumes the HRESULT, generally stored in eax, should be validated. IUnknown based interfaces that don't return HRESULT aren't really valid COM and play heck with how COM interop in .NET is implemented. This is particularly true with the graphics related interfaces, which don't really want COM they want the IUnknown contract. It regularly causes issues.

I highly recommend TerraFX tooling for this sort of thing. @tannergooding has done an amazing job of projecting these APIs properly.

[mgaffigan]

I greatly appreciate CsWin32 making P/Invoke substantially easier in C#, but this is an extremely frustrating gap to have to find out through crashes. Regardless of whether this issue is metadata, coreclr, cswin32, implementation, or documentation, it needs to be improved.

If CsWin32 is not expected to generate valid clients for these interfaces, then please don't produce them (produce an error, a vtbl_gap, or hide them behind a flag).

If CsWin32 is expected to generate valid clients, then please fix them (even if a better or more complete fix is coming long-term).

The worst scenario is what we have today: subtle issues that cause memory corruption and crashes. TerraFX is great, I'm sure, but CsWin32 has become the default for all things Windows interop for our team.