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

Unexpected results returning struct with floats in RCW #10901

Closed
JeremyKuhne opened this issue Aug 14, 2018 · 11 comments
Closed

Unexpected results returning struct with floats in RCW #10901

JeremyKuhne opened this issue Aug 14, 2018 · 11 comments

Comments

@JeremyKuhne
Copy link
Member

In trying to wrap ID2D1RenderTarget::GetSize which returns [D2D_SIZE_F] the expected way it would corrupt the stack.

Flipping it to an out parameter made it work, but I'm unclear as to why. Discussing with @davidwrighton, there might be a bug here. I was unable to create the same scenario via straight [DllImport]s against a sample project. I'll see if I can get a smaller repro together.

In d2d1.h it is defined as:

STDMETHOD_(D2D1_SIZE_F, GetSize)(
    ) CONST PURE;

Which expands to:

virtual __declspec(nothrow) D2D1_SIZE_F __stdcall GetSize() const = 0;

The struct is:

typedef struct D2D_SIZE_F
{
    FLOAT width;
    FLOAT height;
} D2D_SIZE_F;

I wrapped this here (SizeF is from System.Drawing):

[PreserveSig]
void GetSize(out SizeF size);

// Doesn't Work:
// [PreserveSig]
// SizeF GetSize();

In the WInterop project I have a test Direct2dTests.RenderTarget.GetSize() that exercises it. One can also look at this by tweaking the Direct2dDemo project (i.e. call the method) which runs on .NET Core and sets up a render target that you can use.

As stated, I'll try and get a smaller COM repro together for this.

@AaronRobinsonMSFT
Copy link
Member

cc @luqunl

@jkotas
Copy link
Member

jkotas commented Aug 14, 2018

Flipping it to an out parameter made it work, but I'm unclear as to why.

It will work on some platforms, but not others, depending on calling convention details.

@JeremyKuhne
Copy link
Member Author

It will work on some platforms, but not others, depending on calling convention details.

Thanks. @davidwrighton mentioned that to me too- I should have called that out lest anyone think this is a good workaround. :)

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Aug 14, 2018

I created a repro for when the [PreserveSig] is used, but if removed, the struct is returned successfully around 50 % of the time 😕

This is definitely an issue we need to root cause.

cc @luqunl

@jkotas
Copy link
Member

jkotas commented Aug 14, 2018

Here is what I see with [PreserveSig]:

  • On x86, it reliably throws System.Runtime.InteropServices.MarshalDirectiveException: Method's type signature is not Interop compatible. here: https://github.com/dotnet/coreclr/blob/92d2c4bde42569d2aa22e44550d69f7d743bf9a0/src/vm/dllimport.cpp#L3383 . Notice that somebody ifdefed out this limitation on non-x86.

  • On x64, it reliably crashes with access violation. The managed side expects the value to be returned in register, but the unmanaged side returns it via return buffer. Apparently, the calling convention for returning structs differs between instance methods and static methods on Windows x64. From https://docs.microsoft.com/en-us/cpp/build/return-values-cpp: User-defined types can be returned by value from global functions and static member functions. Notice that this excludes non-static member functions. The bug is that interop marshaling does not differentiate between static and non-static return values on x64.

@luqunl
Copy link
Contributor

luqunl commented Aug 14, 2018

@AaronRobinsonMSFT , are you working on this? if not, I can try to fix this bug.

@JeremyKuhne
Copy link
Member Author

If you need anything else from me, let me know. I'm finding this particular pattern all over the DirectX APIs (struct return).

@jkoritzinsky
Copy link
Member

We have this worked around in SharpGenTools (as @JeremyKuhne noticed, this pattern is common in DirectX).

See the following code:
https://github.com/SharpGenTools/SharpGenTools/blob/master/SharpGen/Transform/MethodTransform.cs#L222
https://github.com/SharpGenTools/SharpGenTools/blob/master/SharpGen/Generator/NativeInvocationCodeGenerator.cs#L33
https://github.com/SharpGenTools/SharpGenTools/blob/master/SharpGen/Generator/ShadowCallbackGenerator.cs#L89

As I read the documentation referenced in the PR I notice that our condition for IsReturnStructLarge isn't perfect, but the codegen when the condition is accurate works perfectly.

@jkotas
Copy link
Member

jkotas commented Sep 17, 2018

We have this worked around in SharpGenTools

Submitted PR to link the workaround to this issue: SharpGenTools/SharpGenTools#82

@luqunl luqunl removed their assignment Oct 11, 2018
@AaronRobinsonMSFT
Copy link
Member

@jkoritzinsky Do we have any additional thoughts on this work for Core 3.0?

@jkoritzinsky
Copy link
Member

Luqun's solution looked like it would work from a cursory glance, so I'm going to try it out and see what I get with it.

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants