Correctly marshal structure return values in member functions on Win-x64 and Win-x86 #23145
Conversation
…struct return type, we need to do a by-ref return.
…n buffer argument correctly in instance signatures on AMD64-Windows.
…for structs <= 8 bytes.
| // code:MarshalInfo.MarhalInfo where we do the optimization on x86). We want to throw only if the structure | ||
| // is too big to be returned in registers. | ||
| if (marshalType != MarshalInfo::MARSHAL_TYPE_BLITTABLEVALUECLASS || | ||
| IsUnmanagedValueTypeReturnedByRef(returnInfo.GetNativeArgSize())) |
There was a problem hiding this comment.
returnInfo.GetNativeArgSize() isn't set up before this, so it was always returning 0, which effectively made this condition equivalent to marshalType != MarshalInfo::MARSHAL_TYPE_BLITTABLEVALUECLASS. So I've simplified the condition and combined it with the below failure condition.
…buffer" fix to functions marked as unmanaged thiscall since they all must be instance methods
jkoritzinsky
changed the title
jkoritzinsky
force-pushed
the
coreclr/19474
branch
from
c556519
to
f590dca
Compare
…turn buffer (if the return buffer exists) on all platforms.
|
Apparently Ubuntu ARM32 treats returning pointer-to-member-functions differently than pointer-to-nonmember-functions, which is what is causing the Ubuntu ARM32 leg to fail. Pointers to nonmember functions don't require a return buffer, but pointers to member functions do. I'll see if I can figure out a good way to fit the "is instance method" check in the right place without making the code significantly messy. |
|
I had forgotten that pointer-to-member functions were really weird (i.e. they aren't actually functions or pointers but implementation-defined structs), which was causing the issue on Ubuntu ARM. So, I've changed the test to dereference into the native vtable of a native object so I can make sure I actually get a function pointer for the test. |
|
|
||
| bool isInstanceMethod = fStubNeedsCOM || fThisCall; | ||
| // We cannot just use pSig.GetReturnType() here since it will return ELEMENT_TYPE_VALUETYPE for enums. | ||
| bool isReturnTypeValueType = msig.GetRetTypeHandleThrowing().GetVerifierCorElementType() == ELEMENT_TYPE_VALUETYPE; | ||
| #if defined(_TARGET_X86_) || defined(_TARGET_ARM_) | ||
| // JIT32 has problems in generating code for pinvoke ILStubs which do a return in return buffer. |
There was a problem hiding this comment.
Have you looked at what it would take to fix the JIT (or whether the JIT got fixed since this comment was written originally)?
It does not feel right to have all these platform-specific calling convention details replicated here.
There was a problem hiding this comment.
I have not looked into the JIT side of this yet. I'll investigate to see if RyuJIT fixed some of this.
There was a problem hiding this comment.
RyuJIT has not done any work to fix this, presumably since the interop space was already working around the limitation when RyuJIT was created.
@dotnet/jit-contrib any ideas on what it would take to support the easy case of x86 return buffers for non-member functions?
|
Do we have ability to run IJW test suite from the C++ compiler team? It has extensive coverage for the thiscall calling convention. It may be worth running it on this change (on both x86 and x64). |
|
We'd have to port over each test individually into our test tree and set it up to run like we currently run IJW tests. So it's possible, but not simple copy-paste. |
|
I did not mean to port the tests over. I meant to run the tests from the managed C++ compiler test suite where they currently are. I believe the C++ compiler team has ability to do that - otherwise, they would not be able to sign off on .NET Core support for managed C++. |
|
We can't run the managed C++ compiler test suite in mixed-mode currently until we get #22636 and dotnet/core-setup#5185 both merged in and the C++ compiler team updates the compiler to link to the new |
|
@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0) |
|
@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test |
|
Test failures are due to #23534 as far as I can tell. I'm going to merge this in and file a tracking issue for moving some of the return-buffer logic to the JIT. |
…x64 and Win-x86 (dotnet#23145) * In Windows-x64, if we have a native member function signature with a struct return type, we need to do a by-ref return. * Implement support for marshalling structure return values via a return buffer argument correctly in instance signatures on AMD64-Windows. * Change field initialization ordering to satisfy warning. * Try to narrow down the conditions that trigger these changes to just COM methods. * Don't bash the return type on AMD64 Windows. Only treat it as a byref return buffer. * PR feedback. * Enable returning structs from COM methods via a return buffer on x86 for structs <= 8 bytes. * Add test for struct returns with ThisCall. Extend the "struct return buffer" fix to functions marked as unmanaged thiscall since they all must be instance methods * Don't include the return-type-bashing switch on AMD64 platforms. * Don't do the signature swapping/copy on non-instance functions with struct returns. * Cast the return type of GetStubTargetCallingConv to the right calling convention enum type. * If we're doing a thiscall, marshal the "this" parameter before the return buffer (if the return buffer exists) on all platforms. * Remove temporary logging code I added in for debugging. * Clean up class naming. * Try using a vtable instead of a pointer-to-member-function. * Remove delete of class with non-virtual destructor
…x64 and Win-x86 (dotnet/coreclr#23145) * In Windows-x64, if we have a native member function signature with a struct return type, we need to do a by-ref return. * Implement support for marshalling structure return values via a return buffer argument correctly in instance signatures on AMD64-Windows. * Change field initialization ordering to satisfy warning. * Try to narrow down the conditions that trigger these changes to just COM methods. * Don't bash the return type on AMD64 Windows. Only treat it as a byref return buffer. * PR feedback. * Enable returning structs from COM methods via a return buffer on x86 for structs <= 8 bytes. * Add test for struct returns with ThisCall. Extend the "struct return buffer" fix to functions marked as unmanaged thiscall since they all must be instance methods * Don't include the return-type-bashing switch on AMD64 platforms. * Don't do the signature swapping/copy on non-instance functions with struct returns. * Cast the return type of GetStubTargetCallingConv to the right calling convention enum type. * If we're doing a thiscall, marshal the "this" parameter before the return buffer (if the return buffer exists) on all platforms. * Remove temporary logging code I added in for debugging. * Clean up class naming. * Try using a vtable instead of a pointer-to-member-function. * Remove delete of class with non-virtual destructor Commit migrated from dotnet/coreclr@6b889ab
On Windows x86 and x64, if we have a native member function signature with a struct return type, we need to do a by-ref return.
Superscedes #19643
Fixes #19474.