Opt COM methods out of the new Windows instance-method handling.

[jkoritzinsky]

Some of our users (such as WPF), use a struct to wrap their HRESULT return types on COM members when they use PreserveSig. When we updated CoreCLR to correctly handle the Windows calling convention, we broke this behavior.

Remove tests that were verifying that COM follows the new behavior.

Supercedes #23955

cc: @jeffschwMSFT @davidwrighton

[jkotas]

Thanks!

[jkoritzinsky]

@JeremyKuhne just a heads up that we're reverting the change to supporting returning structures from COM methods in .NET (we're keeping it for ThisCall). So we still won't be able to support mapping DirectX via the built-in COM support without additional work done at some point in the future (likely some sort of explicit opt-in attribute on individual methods).

[jkoritzinsky]

cc: @tannergooding

[tannergooding]

LGTM.

It might be worthwhile adding a COM test explicitly validating the struct wrapper behavior, however.

[AaronRobinsonMSFT]

It might be worthwhile adding a COM test explicitly validating the struct wrapper behavior

I would agree with this. It would also provide a nice location to document the scenario that appears to be so common (i.e. struct HRESULT).

[weltkante]

Can someone explain (or link to the issue) why having a struct HRESULT which has a memory layout equivalent to an integer is preventing a fix to COM/DirectX interop? I'm doing a lot of windows interop (including some DirectX via builtin interop) and never ran into an issue, so I'm really curios.

Isn't a managed struct with explicit layout to 4 byte and int at offset zero, equivalent in memory layout to a plain integer? I can see how there can be bugs by code not looking closely enough at the struct, but why would this prevent correctly handling calling conventions? Is a C struct wrapping an integer not equivalent to a plain integer in these calling conventions?

[jkotas]

Isn't a managed struct with explicit layout to 4 byte and int at offset zero, equivalent in memory layout to a plain integer?

The memory layout is equivalent, but the C++ calling convention rules for these two are not always the same. For example, compile the following C++ snippet on Windows x86:

struct Wrapper
{
    int value;
};

class Test
{
public:
    int M1();
    Wrapper M2();
};

int Test::M1()
{
    return 0;
}

Wrapper Test::M2()
{
    Wrapper ret;
    ret.value = 0;
    return ret;
}

And look at the disassembly for methods M1 and M2:

?M1@Test@@QAEHXZ (public: int __thiscall Test::M1(void)):
  00000000: 33 C0              xor         eax,eax
  00000002: C3                 ret

?M2@Test@@QAE?AUWrapper@@XZ (public: struct Wrapper __thiscall Test::M2(void)):
  00000000: 8B 44 24 04        mov         eax,dword ptr [esp+4]
  00000004: C7 00 00 00 00 00  mov         dword ptr [eax],0
  0000000A: C2 04 00           ret         4

You can see that the value is returned via eax register in the first case, but it is returned via a return buffer stored in memory in the second case.

COM/DirectX interop

Classic COM methods always return HRESULTs. DirectX is using atypical flavor of COM (it is sometimes called nano-COM). DirectX interface methods sometimes return structs instead of HRESULTs. The built-in COM interop does not handle COM methods returning structs correctly. It is the bug that @jkoritzinsky tried to fix.

However, there are number of other reasons why the built-in COM interop is not a great fit for DirectX and why SharpDX was invented. It is unlikely that the fix alone would turn build-in COM interop with DirectX into a success story.

[weltkante]

Thats strange, if I try to return a wrapper struct from a STDCALL non-member function then it goes through EAX for me. Can't find a clear technical specification of STDCALL right now.

struct HRESULT2 { HRESULT hr; };
struct ITest { STDMETHOD_(HRESULT2, Run)() = 0; };
struct ITestVtbl { HRESULT2 (STDMETHODCALLTYPE *Run)(ITest* This); };
struct Test : ITest { STDMETHODIMP_(HRESULT2) Run() { return { E_FAIL }; } };
static STDMETHODIMP_(HRESULT2) RunImpl(ITest* This) { return { E_FAIL }; }

both are STDCALL
Run returns through a buffer
RunImpl returns through eax

Seeing that #19474 was probably what triggered this whole thing, unless I made a mistake somewhere that would make the ID2D1RenderTarget::GetSize API inherently nonportable across languages, as you can't call it via C based interop anymore. Usually the Windows headers used to be C compatible, and classic MIDL style COM even generates a vtbl struct of function pointers, but if you would do that for this interface then the calling convention of a function pointer and instance method is not the same ?!

Thats a really unfortunate mess of a calling convention :-(

Thanks a lot for the explanation.

[tannergooding]

both are STDCALL

ID2D1RenderTarget::GetSize and member functions in general are considered THISCALL, which is a slight variant of STDCALL that handles return buffers and the implicit this parameter differently.

You can find more documentation on the Windows calling conventions here and in the neighboring docs: https://docs.microsoft.com/en-us/cpp/cpp/thiscall?view=vs-2019

The primary reason for THISCALL differing, to my knowledge, is so that the signature can better handle some C++ oriented concepts, such as virtualization and implicit members.

[jkotas]

ID2D1RenderTarget::GetSize and member functions in general are considered THISCALL

This is not correct. ID2D1RenderTarget::GetSize is stdcall. THISCALL is the default for C++ member functions, but it can be overwridden to be stdcall. However, overriding thiscall to stdcall does not change convention for struct returns.

Consider this:

struct Wrapper { int x; };

class Test
{
    Wrapper x;
public:
    Wrapper __thiscall M1();
    Wrapper __stdcall M2();

    static Wrapper __stdcall M3(Test* p);
};

Wrapper Test::M1()
{
    return x;
}

Wrapper Test::M2()
{
    return x;
}

Wrapper Test::M3(Test *p)
{
    return p->x;
}

compiles into:

?M1@Test@@QAE?AUWrapper@@XZ (public: struct Wrapper __thiscall Test::M1(void)):
  00000000: 8B 44 24 04        mov         eax,dword ptr [esp+4]
  00000004: 8B 09              mov         ecx,dword ptr [ecx]
  00000006: 89 08              mov         dword ptr [eax],ecx
  00000008: C2 04 00           ret         4

?M2@Test@@QAG?AUWrapper@@XZ (public: struct Wrapper __stdcall Test::M2(void)):
  00000000: 8B 44 24 04        mov         eax,dword ptr [esp+4]
  00000004: 8B 08              mov         ecx,dword ptr [eax]
  00000006: 8B 44 24 08        mov         eax,dword ptr [esp+8]
  0000000A: 89 08              mov         dword ptr [eax],ecx
  0000000C: C2 08 00           ret         8

?M3@Test@@SG?AUWrapper@@PAV1@@Z (public: static struct Wrapper __stdcall Test::M3(class Test *)):
  00000000: 8B 44 24 04        mov         eax,dword ptr [esp+4]
  00000004: 8B 00              mov         eax,dword ptr [eax]
  00000006: C2 04 00           ret         4

[weltkante]

Yeah, what I'm going for is that apparently HRESULT2(__stdcall ITest::*F1)();
and HRESULT2(__stdcall *F2)(ITest*); are not the same, which surprised me because the MIDL generated code usually makes it look they are interchangeable.

The current issue with COM aside, this difference will probably affect the C# language proposals for static delgates / function pointers as well. You probably want to be able to express both, and just specifying STDCALL won't be enough then.

@jkotas do you know if this is already being considered, or if not, where to bring it up in case it is relevant? I'm not up to date anymore with those proposals, but last I've seen (~ half a year ago) they were still being considered as language extensions. I remember some people wanted to use this feature for exactly this kind of interop ...

[jkotas]

@jkotas do you know if this is already being considered, or if not, where to bring it up in case it is relevant?

The design document is in csharplang repo. I have submitted dotnet/csharplang#2432 to capture this point in it. Please take a look.

[AaronRobinsonMSFT]

@jkoritzinsky This looks like a legitimate test failure in COM.

[jkoritzinsky]

Yeah I’ll take a look tomorrow morning when I get into the office to see what’s up.

[jkoritzinsky]

I've verified locally that dialogs in NuGet Package Explorer work with this change.

[jkoritzinsky]

Test failures are from an infra issue. Merging this into master.