Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Correctly marshal structure return values in member functions on Win-x64 and Win-x86 #23145

Merged
merged 20 commits into from
Mar 29, 2019
Merged

Correctly marshal structure return values in member functions on Win-x64 and Win-x86 #23145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
076689f
In Windows-x64, if we have a native member function signature with a …
jkoritzinsky Mar 8, 2019
8f9a62b
Implement support for marshalling structure return values via a retur…
jkoritzinsky Mar 11, 2019
48bd34e
Change field initialization ordering to satisfy warning.
jkoritzinsky Mar 11, 2019
13da0e7
Try to narrow down the conditions that trigger these changes to just …
jkoritzinsky Mar 11, 2019
67db13f
Don't bash the return type on AMD64 Windows. Only treat it as a byref…
jkoritzinsky Mar 12, 2019
2695fe0
PR feedback.
jkoritzinsky Mar 12, 2019
2e8094c
Enable returning structs from COM methods via a return buffer on x86 …
jkoritzinsky Mar 12, 2019
f7f7776
Merge branch 'master' of https://github.com/dotnet/coreclr into corec…
jkoritzinsky Mar 12, 2019
988e4e2
Add test for struct returns with ThisCall. Extend the "struct return …
jkoritzinsky Mar 13, 2019
bc81307
Merge branch 'master' into coreclr/19474
jkoritzinsky Mar 13, 2019
389a0f3
Don't include the return-type-bashing switch on AMD64 platforms.
jkoritzinsky Mar 13, 2019
10bc296
Don't do the signature swapping/copy on non-instance functions with s…
jkoritzinsky Mar 13, 2019
f590dca
Cast the return type of GetStubTargetCallingConv to the right calling…
jkoritzinsky Mar 13, 2019
1d8f29a
If we're doing a thiscall, marshal the "this" parameter before the re…
jkoritzinsky Mar 13, 2019
c27a327
Remove temporary logging code I added in for debugging.
jkoritzinsky Mar 13, 2019
e14e205
Clean up class naming.
jkoritzinsky Mar 14, 2019
807e56e
Try using a vtable instead of a pointer-to-member-function.
jkoritzinsky Mar 15, 2019
656b0eb
Remove delete of class with non-virtual destructor
jkoritzinsky Mar 18, 2019
9d75c28
Merge branch 'master' of https://github.com/dotnet/coreclr into corec…
jkoritzinsky Mar 26, 2019
4528c30
Merge branch 'master' of https://github.com/dotnet/coreclr into corec…
jkoritzinsky Mar 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
179 changes: 99 additions & 80 deletions src/vm/dllimport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1737,7 +1737,7 @@ NDirectStubLinker::NDirectStubLinker(
int iLCIDParamIdx,
BOOL fTargetHasThis,
BOOL fStubHasThis)
: ILStubLinker(pModule, signature, pTypeContext, pTargetMD, fTargetHasThis, fStubHasThis, !SF_IsCOMStub(dwStubFlags)),
: ILStubLinker(pModule, signature, pTypeContext, pTargetMD, fTargetHasThis, fStubHasThis, !SF_IsCOMStub(dwStubFlags), SF_IsReverseStub(dwStubFlags)),
m_pCleanupFinallyBeginLabel(NULL),
m_pCleanupFinallyEndLabel(NULL),
m_pSkipExceptionCleanupLabel(NULL),
Expand All @@ -1747,6 +1747,7 @@ NDirectStubLinker::NDirectStubLinker(
m_fHasCleanupCode(FALSE),
m_fHasExceptionCleanupCode(FALSE),
m_fCleanupWorkListIsSetup(FALSE),
m_targetHasThis(fTargetHasThis),
m_dwThreadLocalNum(-1),
m_dwCleanupWorkListLocalNum(-1),
m_dwRetValLocalNum(-1),
Expand Down Expand Up @@ -3661,42 +3662,19 @@ static MarshalInfo::MarshalType DoMarshalReturnValue(MetaSig& msig,

if (SF_IsCOMStub(dwStubFlags))
{
if (marshalType == MarshalInfo::MARSHAL_TYPE_VALUECLASS ||
marshalType == MarshalInfo::MARSHAL_TYPE_BLITTABLEVALUECLASS ||
// We don't support native methods that return VARIANTs, non-blittable structs, GUIDs, or DECIMALs directly.
if (marshalType == MarshalInfo::MARSHAL_TYPE_OBJECT ||
marshalType == MarshalInfo::MARSHAL_TYPE_VALUECLASS ||
marshalType == MarshalInfo::MARSHAL_TYPE_GUID ||
marshalType == MarshalInfo::MARSHAL_TYPE_DECIMAL)
{
#ifndef _TARGET_X86_
// We cannot optimize marshalType to MARSHAL_TYPE_GENERIC_* because the JIT works with exact types
// and would refuse to compile the stub if it implicitly converted between scalars and value types (also see
// 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()))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#endif // _TARGET_X86_
if (!SF_IsHRESULTSwapping(dwStubFlags) && !SF_IsCOMLateBoundStub(dwStubFlags))
{
if (!SF_IsHRESULTSwapping(dwStubFlags) && !SF_IsCOMLateBoundStub(dwStubFlags))
{
// Note that this limitation is very likely not needed anymore and could be lifted if we care.
COMPlusThrow(kMarshalDirectiveException, IDS_EE_COM_UNSUPPORTED_SIG);
}
COMPlusThrow(kMarshalDirectiveException, IDS_EE_COM_UNSUPPORTED_SIG);
}

pss->MarshalReturn(&returnInfo, argOffset);
}
else
{
// We don't support native methods that return VARIANTs directly.
if (marshalType == MarshalInfo::MARSHAL_TYPE_OBJECT)
{
if (!SF_IsHRESULTSwapping(dwStubFlags) && !SF_IsCOMLateBoundStub(dwStubFlags))
{
COMPlusThrow(kMarshalDirectiveException, IDS_EE_COM_UNSUPPORTED_SIG);
}
}

pss->MarshalReturn(&returnInfo, argOffset);
}
pss->MarshalReturn(&returnInfo, argOffset);
}
else
#endif // FEATURE_COMINTEROP
Expand Down Expand Up @@ -3953,14 +3931,21 @@ static void CreateNDirectStubWorker(StubState* pss,
// Normally we would like this to be false so that we use the correct signature
// in the IL_STUB, (i.e if it returns a value class then the signature will use that)
// When this bool is true we change the return type to void and explicitly add a
// return buffer argument as the first argument.
BOOL fMarshalReturnValueFirst = false;
// return buffer argument as the first argument so as to match the native calling convention correctly.
BOOL fMarshalReturnValueFirst = FALSE;

BOOL fReverseWithReturnBufferArg = FALSE;

// We can only change fMarshalReturnValueFirst to true when we are NOT doing HRESULT-swapping!
//
// When we are HRESULT-swapping, the managed return type is actually the type of the last parameter and not the return type.
// The native return type of an HRESULT-swapped function is an HRESULT, which never uses a return-buffer argument.
// Since the managed return type is actually the last parameter, we need to marshal it after the last parameter in the managed signature
// to make sure we match the native signature correctly (when marshalling parameters, we add them to the native stub signature).
if (!SF_IsHRESULTSwapping(dwStubFlags))
{

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not looked into the JIT side of this yet. I'll investigate to see if RyuJIT fixed some of this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

// Therefore instead we change the signature of calli to return void and make the return buffer as first
Expand All @@ -3972,55 +3957,18 @@ static void CreateNDirectStubWorker(StubState* pss,
#ifdef UNIX_X86_ABI
// For functions with value type class, managed and unmanaged calling convention differ
fMarshalReturnValueFirst = HasRetBuffArgUnmanagedFixup(&msig);
#else // UNIX_X86_ABI
#elif defined(_TARGET_ARM_)
fMarshalReturnValueFirst = HasRetBuffArg(&msig);
#else
// On Windows-X86, the native signature might need a return buffer when the managed doesn't (specifically when the native signature is a member function).
fMarshalReturnValueFirst = HasRetBuffArg(&msig) || (isInstanceMethod && isReturnTypeValueType);
#endif // UNIX_X86_ABI

#elif defined(_TARGET_AMD64_)
fMarshalReturnValueFirst = isInstanceMethod && isReturnTypeValueType;
#endif // defined(_TARGET_X86_) || defined(_TARGET_ARM_)

}

if (fMarshalReturnValueFirst)
{
marshalType = DoMarshalReturnValue(msig,
pParamTokenArray,
nlType,
nlFlags,
0,
pss,
fThisCall,
argOffset,
dwStubFlags,
pMD,
nativeStackSize,
fStubNeedsCOM,
0
DEBUG_ARG(pSigDesc->m_pDebugName)
DEBUG_ARG(pSigDesc->m_pDebugClassName)
);

if (marshalType == MarshalInfo::MARSHAL_TYPE_DATE ||
marshalType == MarshalInfo::MARSHAL_TYPE_CURRENCY ||
marshalType == MarshalInfo::MARSHAL_TYPE_ARRAYWITHOFFSET ||
marshalType == MarshalInfo::MARSHAL_TYPE_HANDLEREF ||
marshalType == MarshalInfo::MARSHAL_TYPE_ARGITERATOR
#ifdef FEATURE_COMINTEROP
|| marshalType == MarshalInfo::MARSHAL_TYPE_OLECOLOR
#endif // FEATURE_COMINTEROP
)
{
// These are special non-blittable types returned by-ref in managed,
// but marshaled as primitive values returned by-value in unmanaged.
}
else
{
// This is an ordinary value type - see if it is returned by-ref.
MethodTable *pRetMT = msig.GetRetTypeHandleThrowing().AsMethodTable();
if (IsUnmanagedValueTypeReturnedByRef(pRetMT->GetNativeSize()))
{
nativeStackSize += sizeof(LPVOID);
}
}
#ifdef _WIN32
fReverseWithReturnBufferArg = fMarshalReturnValueFirst && SF_IsReverseStub(dwStubFlags);
#endif
}

//
Expand Down Expand Up @@ -4088,6 +4036,77 @@ static void CreateNDirectStubWorker(StubState* pss,
// Marshal the parameters
int argidx = 1;
int nativeArgIndex = 0;

// If we are generating a return buffer on a member function that is marked as thiscall (as opposed to being a COM method)
// then we need to marshal the this parameter first and the return buffer second.
// We don't need to do this for COM methods because the "this" is implied as argument 0 by the signature of the stub.
if (fThisCall && fMarshalReturnValueFirst)
{
msig.NextArg();

MarshalInfo &info = pParamMarshalInfo[argidx - 1];
pss->MarshalArgument(&info, argOffset, GetStackOffsetFromStackSize(nativeStackSize, fThisCall));
nativeStackSize += info.GetNativeArgSize();

fStubNeedsCOM |= info.MarshalerRequiresCOM();

// make sure that the first parameter is enregisterable
if (info.GetNativeArgSize() > sizeof(SLOT))
COMPlusThrow(kMarshalDirectiveException, IDS_EE_NDIRECT_BADNATL_THISCALL);

argidx++;
}

// If we're doing a native->managed call and are generating a return buffer,
// we need to move all of the actual arguments over one and have the return value be the first argument (after the this pointer if applicable).
if (fReverseWithReturnBufferArg)
{
++argOffset;
}

if (fMarshalReturnValueFirst)
{
marshalType = DoMarshalReturnValue(msig,
pParamTokenArray,
nlType,
nlFlags,
0,
pss,
fThisCall,
argOffset,
dwStubFlags,
pMD,
nativeStackSize,
fStubNeedsCOM,
0
DEBUG_ARG(pSigDesc->m_pDebugName)
DEBUG_ARG(pSigDesc->m_pDebugClassName)
);

if (marshalType == MarshalInfo::MARSHAL_TYPE_DATE ||
marshalType == MarshalInfo::MARSHAL_TYPE_CURRENCY ||
marshalType == MarshalInfo::MARSHAL_TYPE_ARRAYWITHOFFSET ||
marshalType == MarshalInfo::MARSHAL_TYPE_HANDLEREF ||
marshalType == MarshalInfo::MARSHAL_TYPE_ARGITERATOR
#ifdef FEATURE_COMINTEROP
|| marshalType == MarshalInfo::MARSHAL_TYPE_OLECOLOR
#endif // FEATURE_COMINTEROP
)
{
// These are special non-blittable types returned by-ref in managed,
// but marshaled as primitive values returned by-value in unmanaged.
}
else
{
// This is an ordinary value type - see if it is returned by-ref.
MethodTable *pRetMT = msig.GetRetTypeHandleThrowing().AsMethodTable();
if (IsUnmanagedValueTypeReturnedByRef(pRetMT->GetNativeSize()))
{
nativeStackSize += sizeof(LPVOID);
}
}
}

while (argidx <= numArgs)
{
#ifdef FEATURE_COMINTEROP
Expand Down
5 changes: 5 additions & 0 deletions src/vm/dllimport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,10 @@ class NDirectStubLinker : public ILStubLinker

void SetInteropParamExceptionInfo(UINT resID, UINT paramIdx);
bool HasInteropParamExceptionInfo();
bool TargetHasThis()
{
return m_targetHasThis == TRUE;
}

void ClearCode();

Expand Down Expand Up @@ -556,6 +560,7 @@ class NDirectStubLinker : public ILStubLinker
BOOL m_fHasCleanupCode;
BOOL m_fHasExceptionCleanupCode;
BOOL m_fCleanupWorkListIsSetup;
BOOL m_targetHasThis;
DWORD m_dwThreadLocalNum; // managed-to-native only
DWORD m_dwArgMarshalIndexLocalNum;
DWORD m_dwCleanupWorkListLocalNum;
Expand Down
54 changes: 42 additions & 12 deletions src/vm/ilmarshalers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,10 @@ class ILMarshaler
bool byrefNativeReturn = false;
CorElementType typ = ELEMENT_TYPE_VOID;
UINT32 nativeSize = 0;

bool nativeMethodIsMemberFunction = (m_pslNDirect->TargetHasThis() && IsCLRToNative(m_dwMarshalFlags))
|| (m_pslNDirect->HasThis() && !IsCLRToNative(m_dwMarshalFlags))
|| ((CorInfoCallConv)m_pslNDirect->GetStubTargetCallingConv() == CORINFO_CALLCONV_THISCALL);

// we need to convert value type return types to primitives as
// JIT does not inline P/Invoke calls that return structures
if (nativeType.IsValueClass())
Expand All @@ -599,33 +602,56 @@ class ILMarshaler
nativeSize = wNativeSize;
}

#if defined(_TARGET_X86_)
#if defined(_TARGET_X86_) || (defined(_TARGET_AMD64_) && defined(_WIN32))
// JIT32 and JIT64 (which is only used on the Windows Desktop CLR) has a problem generating
// code for the pinvoke ILStubs which do a return using a struct type. Therefore, we
// change the signature of calli to return void and make the return buffer as first argument.

// for X86 and AMD64-Windows we bash the return type from struct to U1, U2, U4 or U8
// For Windows AMD64 and x86, we need to use a return buffer for native member functions returning structures.
// for X86 Windows non-member functions we bash the return type from struct to U1, U2, U4 or U8
// and use byrefNativeReturn for all other structs.
// for UNIX_X86_ABI, we always need a return buffer argument for any size of structs.
switch (nativeSize)
#if defined(_WIN32)
if (nativeMethodIsMemberFunction)
{
byrefNativeReturn = true;
}
else
#endif // _WIN32
{
#ifndef _TARGET_AMD64_
switch (nativeSize)
{
#ifndef UNIX_X86_ABI
case 1: typ = ELEMENT_TYPE_U1; break;
case 2: typ = ELEMENT_TYPE_U2; break;
case 4: typ = ELEMENT_TYPE_U4; break;
case 8: typ = ELEMENT_TYPE_U8; break;
#endif
default: byrefNativeReturn = true; break;
case 1: typ = ELEMENT_TYPE_U1; break;
case 2: typ = ELEMENT_TYPE_U2; break;
case 4: typ = ELEMENT_TYPE_U4; break;
case 8: typ = ELEMENT_TYPE_U8; break;
#endif // UNIX_X86_ABI
default: byrefNativeReturn = true; break;
}
#endif // _TARGET_AMD64_
}
#endif
#endif // defined(_TARGET_X86_) || (defined(_TARGET_AMD64_) && defined(_WIN32))
}

if (IsHresultSwap(dwMarshalFlags) || (byrefNativeReturn && IsCLRToNative(dwMarshalFlags)))
if (IsHresultSwap(dwMarshalFlags) || (byrefNativeReturn && (IsCLRToNative(m_dwMarshalFlags) || nativeMethodIsMemberFunction)))
{
LocalDesc extraParamType = nativeType;
extraParamType.MakeByRef();

m_pcsMarshal->SetStubTargetArgType(&extraParamType, false);
if (byrefNativeReturn && !IsCLRToNative(m_dwMarshalFlags))
{
// If doing a native->managed call and returning a structure by-ref,
// the native signature has an extra param for the struct return
// than the managed signature. Adjust the target stack delta to account this extra
// parameter.
m_pslNDirect->AdjustTargetStackDeltaForExtraParam();
// We also need to account for the lack of a return value in the native signature.
// To do this, we adjust the stack delta again for the return parameter.
m_pslNDirect->AdjustTargetStackDeltaForExtraParam();
}

if (IsHresultSwap(dwMarshalFlags))
{
Expand Down Expand Up @@ -742,6 +768,10 @@ class ILMarshaler
m_nativeHome.EmitCopyToByrefArgWithNullCheck(m_pcsUnmarshal, &nativeType, argidx);
m_pcsUnmarshal->EmitLDC(S_OK);
}
else if (byrefNativeReturn && nativeMethodIsMemberFunction)
{
m_nativeHome.EmitCopyToByrefArg(m_pcsUnmarshal, &nativeType, argidx);
}
else
{
if (typ != ELEMENT_TYPE_VOID)
Expand Down
Loading