Correctly marshal structure return values in member functions on Win-x64 and Win-x86 (#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

16 files changed, 459 insertions, 116 deletions

diff --git a/src/vm/dllimport.cpp b/src/vm/dllimport.cpp
index b200a1bccb..c83f05ce05 100644
--- a/src/vm/dllimport.cpp
+++ b/src/vm/dllimport.cpp
@@ -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),
@@ -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),
@@ -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()))
-#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
@@ -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.
         // Therefore instead we change the signature of calli to return void and make the return buffer as first
@@ -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
     }
 
     //
@@ -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
diff --git a/src/vm/dllimport.h b/src/vm/dllimport.h
index b88d339a84..a6967ebfb4 100644
--- a/src/vm/dllimport.h
+++ b/src/vm/dllimport.h
@@ -494,6 +494,10 @@ public:
 
     void    SetInteropParamExceptionInfo(UINT resID, UINT paramIdx);
     bool    HasInteropParamExceptionInfo();
+    bool    TargetHasThis()
+    {
+        return m_targetHasThis == TRUE;
+    }
 
     void ClearCode();
 
@@ -556,6 +560,7 @@ protected:
     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;
diff --git a/src/vm/ilmarshalers.h b/src/vm/ilmarshalers.h
index c892ae66a9..87d81504e7 100644
--- a/src/vm/ilmarshalers.h
+++ b/src/vm/ilmarshalers.h
@@ -583,7 +583,10 @@ public:
         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())
@@ -599,33 +602,56 @@ public:
                 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))
             {
@@ -742,6 +768,10 @@ public:
                 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)
diff --git a/src/vm/stubgen.cpp b/src/vm/stubgen.cpp
index 67a6ca03e9..78c6831b71 100644
--- a/src/vm/stubgen.cpp
+++ b/src/vm/stubgen.cpp
@@ -1530,7 +1530,7 @@ void ILCodeStream::EmitPOP()
 void ILCodeStream::EmitRET()
 {
     WRAPPER_NO_CONTRACT;
-    INT16 iStackDelta = m_pOwner->m_StubHasVoidReturnType ? 0 : -1;
+    INT16 iStackDelta = m_pOwner->ReturnOpcodePopsStack() ? -1 : 0;
     Emit(CEE_RET, iStackDelta, 0);
 }
 void ILCodeStream::EmitSHR_UN()
@@ -1684,11 +1684,6 @@ void ILCodeStream::EmitCALL(BinderMethodID id, int numInArgs, int numRetArgs)
     EmitCALL(GetToken(MscorlibBinder::GetMethod(id)), numInArgs, numRetArgs);
 }
 
-
-
-
-
-
 void ILStubLinker::SetHasThis (bool fHasThis)
 {
     LIMITED_METHOD_CONTRACT;
@@ -2156,14 +2151,15 @@ static BOOL SigHasVoidReturnType(const Signature &signature)
 
 
 ILStubLinker::ILStubLinker(Module* pStubSigModule, const Signature &signature, SigTypeContext *pTypeContext, MethodDesc *pMD,
-                           BOOL fTargetHasThis, BOOL fStubHasThis, BOOL fIsNDirectStub) :
+                           BOOL fTargetHasThis, BOOL fStubHasThis, BOOL fIsNDirectStub, BOOL fIsReverseStub) :
     m_pCodeStreamList(NULL),
     m_stubSig(signature),
     m_pTypeContext(pTypeContext),
     m_pCode(NULL),
     m_pStubSigModule(pStubSigModule),
     m_pLabelList(NULL),
-    m_StubHasVoidReturnType(0),
+    m_StubHasVoidReturnType(FALSE),
+    m_fIsReverseStub(fIsReverseStub),
     m_iTargetStackDelta(0),
     m_cbCurrentCompressedSigLen(1),
     m_nLocals(0),
@@ -2181,7 +2177,9 @@ ILStubLinker::ILStubLinker(Module* pStubSigModule, const Signature &signature, S
     m_managedSigPtr = signature.CreateSigPointer();
     if (!signature.IsEmpty())
     {
+        // Until told otherwise, assume that the stub has the same return type as the signature.
         m_StubHasVoidReturnType = SigHasVoidReturnType(signature);
+        m_StubTargetHasVoidReturnType = m_StubHasVoidReturnType;
         
         //
         // Get the stub's calling convention.  Set m_fHasThis to match
@@ -2477,10 +2475,13 @@ void ILStubLinker::SetStubTargetReturnType(LocalDesc* pLoc)
 
     m_nativeFnSigBuilder.SetReturnType(pLoc);
 
-    if ((1 != pLoc->cbType) || (ELEMENT_TYPE_VOID != pLoc->ElementType[0]))
+    // Update check for if a stub has a void return type based on the provided return type.
+    m_StubTargetHasVoidReturnType = ((1 == pLoc->cbType) && (ELEMENT_TYPE_VOID == pLoc->ElementType[0])) ? TRUE : FALSE;
+    if (!m_StubTargetHasVoidReturnType)
     {
         m_iTargetStackDelta++;
     }
+
 }
 
 DWORD ILStubLinker::SetStubTargetArgType(CorElementType typ, bool fConsumeStubArg /*= true*/)
diff --git a/src/vm/stubgen.h b/src/vm/stubgen.h
index 044029b0af..73d72708a7 100644
--- a/src/vm/stubgen.h
+++ b/src/vm/stubgen.h
@@ -110,11 +110,11 @@ struct LocalDesc
 
         bool lastElementTypeIsValueType = false;
         
-        if (ElementType[0] == ELEMENT_TYPE_VALUETYPE)
+        if (ElementType[cbType - 1] == ELEMENT_TYPE_VALUETYPE)
         {
             lastElementTypeIsValueType = true;
         }
-        else if ((ElementType[0] == ELEMENT_TYPE_INTERNAL) &&
+        else if ((ElementType[cbType - 1] == ELEMENT_TYPE_INTERNAL) &&
                     (InternalToken.IsNativeValueType() ||
                      InternalToken.GetMethodTable()->IsValueType()))
         {
@@ -378,7 +378,7 @@ class ILStubLinker
 public:
 
     ILStubLinker(Module* pModule, const Signature &signature, SigTypeContext *pTypeContext, MethodDesc *pMD,
-                 BOOL fTargetHasThis, BOOL fStubHasThis, BOOL fIsNDirectStub = FALSE);
+                 BOOL fTargetHasThis, BOOL fStubHasThis, BOOL fIsNDirectStub = FALSE, BOOL fIsReverseStub = FALSE);
     ~ILStubLinker();
     
     void GenerateCode(BYTE* pbBuffer, size_t cbBufferSize);
@@ -424,7 +424,6 @@ public:
     void GetStubReturnType(LocalDesc * pLoc);
     void GetStubReturnType(LocalDesc * pLoc, Module * pModule);
     CorCallingConvention GetStubTargetCallingConv();
-
     
     CorElementType GetStubTargetReturnElementType() { WRAPPER_NO_CONTRACT; return m_nativeFnSigBuilder.GetReturnElementType(); }
 
@@ -504,12 +503,23 @@ protected:
     void SetStubTargetReturnType(LocalDesc* pLoc);
     void SetStubTargetCallingConv(CorCallingConvention uNativeCallingConv);
 
+    bool ReturnOpcodePopsStack()
+    {
+        if ((!m_fIsReverseStub && m_StubHasVoidReturnType) || (m_fIsReverseStub && m_StubTargetHasVoidReturnType))
+        {
+            return false;
+        }
+        return true;
+    }
+
     void TransformArgForJIT(LocalDesc *pLoc);
 
     Module * GetStubSigModule();
     SigTypeContext *GetStubSigTypeContext();
 
     BOOL    m_StubHasVoidReturnType;
+    BOOL    m_StubTargetHasVoidReturnType;
+    BOOL    m_fIsReverseStub;
     INT     m_iTargetStackDelta;
     DWORD   m_cbCurrentCompressedSigLen;
     DWORD   m_nLocals;
diff --git a/tests/src/Interop/CMakeLists.txt b/tests/src/Interop/CMakeLists.txt
index acf4c849db..99bd9937da 100644
--- a/tests/src/Interop/CMakeLists.txt
+++ b/tests/src/Interop/CMakeLists.txt
@@ -29,6 +29,7 @@ add_subdirectory(PInvoke/SizeParamIndex/ReversePInvoke/PassingByRef)
 add_subdirectory(PInvoke/Array/MarshalArrayAsField/LPArrayNative)
 add_subdirectory(PInvoke/Array/MarshalArrayAsParam/LPArrayNative)
 add_subdirectory(PInvoke/Miscellaneous/HandleRef)
+add_subdirectory(PInvoke/Miscellaneous/ThisCall)
 add_subdirectory(PInvoke/Miscellaneous/MultipleAssembliesWithSamePInvoke)
 add_subdirectory(PInvoke/ExactSpelling)
 add_subdirectory(PInvoke/CriticalHandles)
@@ -65,7 +66,6 @@ add_subdirectory(DllImportAttribute/Simple)
 add_subdirectory(ExecInDefAppDom)
 add_subdirectory(ICustomMarshaler/ConflictingNames)
 add_subdirectory(LayoutClass)
-
 if(WIN32)
     add_subdirectory(PInvoke/Attributes/LCID)
     add_subdirectory(PInvoke/Variant)
diff --git a/tests/src/Interop/COM/NETClients/Primitives/NumericTests.cs b/tests/src/Interop/COM/NETClients/Primitives/NumericTests.cs
index 19d0574063..1eaa2fdde3 100644
--- a/tests/src/Interop/COM/NETClients/Primitives/NumericTests.cs
+++ b/tests/src/Interop/COM/NETClients/Primitives/NumericTests.cs
@@ -38,6 +38,7 @@ namespace NetClient
             this.Marshal_Float(a / 100f, b / 100f);
             this.Marshal_Double(a / 100.0, b / 100.0);
             this.Marshal_ManyInts();
+            this.Marshal_Struct_Return();
         }
 
         static private bool EqualByBound(float expected, float actual)
@@ -200,5 +201,39 @@ namespace NetClient
             Console.WriteLine($"{expected.GetType().Name} 12 test invariant: 1 + 2 + 3 + 4 + 5 + 6 + 7 + 8 + 9 + 10 + 11 + 12 = {expected}");
             Assert.IsTrue(expected == this.server.Add_ManyInts12(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12));
         }
+
+        private void Marshal_Struct_Return()
+        {
+            Console.WriteLine("Struct return from member function marshalling with struct > 4 bytes");
+            {
+                var width = 1.0f;
+                var height = 2.0f;
+                Server.Contract.SizeF result = this.server.MakeSize(width, height);
+                
+                Assert.AreEqual(width, result.width);
+                Assert.AreEqual(height, result.height);
+            }
+            Console.WriteLine("Struct return from member function marshalling with struct <= 4 bytes");
+            {
+                byte width = 1;
+                byte height = 2;
+                Server.Contract.Size result = this.server.MakeSizeSmall(width, height);
+                
+                Assert.AreEqual(width, result.width);
+                Assert.AreEqual(height, result.height);
+            }
+            Console.WriteLine("Struct return from member function marshalling with struct > 8 bytes");
+            {
+                var x = 1.0f;
+                var y = 2.0f;
+                var z = 3.0f;
+                var w = 4.0f;
+                Server.Contract.HFA_4 result = this.server.MakeHFA(x, y ,z, w);
+                Assert.AreEqual(x, result.x);
+                Assert.AreEqual(y, result.y);
+                Assert.AreEqual(z, result.z);
+                Assert.AreEqual(w, result.w);
+            }
+        }
     }
 }
diff --git a/tests/src/Interop/COM/NETServer/NumericTesting.cs b/tests/src/Interop/COM/NETServer/NumericTesting.cs
index 4ed713de53..64ca47d318 100644
--- a/tests/src/Interop/COM/NETServer/NumericTesting.cs
+++ b/tests/src/Interop/COM/NETServer/NumericTesting.cs
@@ -200,4 +200,19 @@ public class NumericTesting : Server.Contract.INumericTesting
     {
         return i1 + i2 + i3 + i4 + i5 + i6 + i7 + i8 + i9 + i10 + i11 + i12;
     }
-}
\ No newline at end of file
+
+    public Server.Contract.SizeF MakeSize(float width, float height)
+    {
+        return new Server.Contract.SizeF { width = width, height = height };
+    }
+
+    public Server.Contract.Size MakeSizeSmall(byte width, byte height)
+    {
+        return new Server.Contract.Size { width = width, height = height };
+    }
+
+    public Server.Contract.HFA_4 MakeHFA(float x, float y, float z, float w)
+    {
+        return new Server.Contract.HFA_4 {x = x, y = y, z = z, w = w};
+    }
+}
diff --git a/tests/src/Interop/COM/NativeClients/Primitives/NumericTests.cpp b/tests/src/Interop/COM/NativeClients/Primitives/NumericTests.cpp
index c951c8c527..a11bb77020 100644
--- a/tests/src/Interop/COM/NativeClients/Primitives/NumericTests.cpp
+++ b/tests/src/Interop/COM/NativeClients/Primitives/NumericTests.cpp
@@ -216,6 +216,26 @@ namespace
         THROW_IF_FAILED(numericTesting->Add_ManyInts12(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, &result));
         THROW_FAIL_IF_FALSE(result == expected);
     }
+
+    void MarshalStructReturn(_In_ INumericTesting* numericTesting)
+    {
+        {
+            ::printf("Marshal struct return type with size > 4 bytes\n");
+            float width = 1.0f;
+            float height = 2.0f;
+            SizeF size = numericTesting->MakeSize(width, height);
+            THROW_FAIL_IF_FALSE(width == size.width);
+            THROW_FAIL_IF_FALSE(height == size.height);
+        }
+        {
+            ::printf("Marshal struct return type with size < 4 bytes\n");
+            BYTE width = 1;
+            BYTE height = 2;
+            Size size = numericTesting->MakeSizeSmall(width, height);
+            THROW_FAIL_IF_FALSE(width == size.width);
+            THROW_FAIL_IF_FALSE(height == size.height);
+        }
+    }
 }
 
 void Run_NumericTests()
@@ -245,4 +265,5 @@ void Run_NumericTests()
     MarshalFloat(numericTesting, (float)a / 100.f, (float)b / 100.f);
     MarshalDouble(numericTesting, (double)a / 100.0, (double)b / 100.0);
     MarshalManyInts(numericTesting);
+    MarshalStructReturn(numericTesting);
 }
diff --git a/tests/src/Interop/COM/NativeServer/NumericTesting.h b/tests/src/Interop/COM/NativeServer/NumericTesting.h
index d30427aa3d..95ec2dbc18 100644
--- a/tests/src/Interop/COM/NativeServer/NumericTesting.h
+++ b/tests/src/Interop/COM/NativeServer/NumericTesting.h
@@ -283,6 +283,30 @@ public:
         return S_OK;
     }
 
+    virtual COM_DECLSPEC_NOTHROW SizeF STDMETHODCALLTYPE MakeSize(
+        /*[in]*/ float width,
+        /*[in]*/ float height)
+    {
+        return { width, height };
+    }
+
+    virtual COM_DECLSPEC_NOTHROW Size STDMETHODCALLTYPE MakeSizeSmall(
+        /*[in]*/ BYTE width,
+        /*[in]*/ BYTE height)
+    {
+        return { width, height };
+    }
+
+    virtual COM_DECLSPEC_NOTHROW HFA_4 STDMETHODCALLTYPE MakeHFA(
+        /*[in]*/ float x,
+        /*[in]*/ float y,
+        /*[in]*/ float z,
+        /*[in]*/ float w
+    )
+    {
+        return { x, y, z, w };
+    }
+
 public: // IUnknown
     STDMETHOD(QueryInterface)(
         /* [in] */ REFIID riid,
diff --git a/tests/src/Interop/COM/ServerContracts/Server.Contracts.cs b/tests/src/Interop/COM/ServerContracts/Server.Contracts.cs
index 9c2e680ed7..7c198c7138 100644
--- a/tests/src/Interop/COM/ServerContracts/Server.Contracts.cs
+++ b/tests/src/Interop/COM/ServerContracts/Server.Contracts.cs
@@ -11,6 +11,18 @@ namespace Server.Contract
     using System.Runtime.InteropServices;
     using System.Text;
 
+    public struct SizeF
+    {
+        public float width;
+        public float height;
+    }
+
+    public struct Size
+    {
+        public byte width;
+        public byte height;
+    }
+
     [ComVisible(true)]
     [Guid("05655A94-A915-4926-815D-A9EA648BAAD9")]
     [InterfaceType(ComInterfaceType.InterfaceIsIUnknown)]
@@ -48,6 +60,14 @@ namespace Server.Contract
 
         int Add_ManyInts11(int i1, int i2, int i3, int i4, int i5, int i6, int i7, int i8, int i9, int i10, int i11);
         int Add_ManyInts12(int i1, int i2, int i3, int i4, int i5, int i6, int i7, int i8, int i9, int i10, int i11, int i12);
+
+        [PreserveSig]
+        SizeF MakeSize(float width, float height);
+        [PreserveSig]
+        Size MakeSizeSmall(byte width, byte height);
+
+        [PreserveSig]
+        HFA_4 MakeHFA(float x, float y, float z, float w);
     }
 
     [ComVisible(true)]
diff --git a/tests/src/Interop/COM/ServerContracts/Server.Contracts.h b/tests/src/Interop/COM/ServerContracts/Server.Contracts.h
index b985cd3547..a3629c140c 100644
--- a/tests/src/Interop/COM/ServerContracts/Server.Contracts.h
+++ b/tests/src/Interop/COM/ServerContracts/Server.Contracts.h
@@ -1,10 +1,30 @@
-// Created by Microsoft (R) C/C++ Compiler
+// Created by Microsoft (R) C/C++ Compiler
 
 #pragma once
 #pragma pack(push, 8)
 
 #include <comdef.h>
 
+struct SizeF
+{
+    float width;
+    float height;
+};
+
+struct Size
+{
+    BYTE width;
+    BYTE height;
+};
+
+struct HFA_4
+{
+    float x;
+    float y;
+    float z;
+    float w;
+};
+
 struct __declspec(uuid("05655a94-a915-4926-815d-a9ea648baad9"))
 INumericTesting : IUnknown
 {
@@ -143,6 +163,17 @@ INumericTesting : IUnknown
         /*[in]*/ int i11,
         /*[in]*/ int i12,
         /*[out]*/ int * result ) = 0;
+        virtual COM_DECLSPEC_NOTHROW SizeF STDMETHODCALLTYPE MakeSize(
+        /*[in]*/ float width,
+        /*[in]*/ float height) = 0;
+        virtual COM_DECLSPEC_NOTHROW Size STDMETHODCALLTYPE MakeSizeSmall(
+        /*[in]*/ BYTE width,
+        /*[in]*/ BYTE height) = 0;
+        virtual COM_DECLSPEC_NOTHROW HFA_4 STDMETHODCALLTYPE MakeHFA(
+        /*[in]*/ float x,
+        /*[in]*/ float y,
+        /*[in]*/ float z,
+        /*[in]*/ float w) = 0;
 };
 
 struct __declspec(uuid("7731cb31-e063-4cc8-bcd2-d151d6bc8f43"))
@@ -365,14 +396,6 @@ enum IDispatchTesting_Exception
     IDispatchTesting_Exception_HResult,
 };
 
-struct HFA_4
-{
-    float x;
-    float y;
-    float z;
-    float w;
-};
-
 struct __declspec(uuid("a5e04c1c-474e-46d2-bbc0-769d04e12b54"))
 IDispatchTesting : IDispatch
 {
diff --git a/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/CMakeLists.txt b/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/CMakeLists.txt
new file mode 100644
index 0000000000..a8908b2143
--- /dev/null
+++ b/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/CMakeLists.txt
@@ -0,0 +1,9 @@
+ 
+cmake_minimum_required (VERSION 2.6) 
+project (ThisCallNative) 
+include ("${CLR_INTEROP_TEST_ROOT}/Interop.cmake") 
+set(SOURCES 
+   ThisCallNative.cpp 
+) 
+add_library (ThisCallNative SHARED ${SOURCES})
+install (TARGETS ThisCallNative DESTINATION bin) 
diff --git a/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallNative.cpp b/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallNative.cpp
new file mode 100644
index 0000000000..9f1d56ccb9
--- /dev/null
+++ b/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallNative.cpp
@@ -0,0 +1,37 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+#include <stdio.h>
+#include <xplatform.h>
+#include <platformdefines.h>
+
+struct SizeF
+{
+    float width;
+    float height;
+};
+
+class C
+{
+    int dummy = 0xcccccccc;
+    float width;
+    float height;
+
+public:
+    C(float width, float height)
+        :width(width),
+        height(height)
+    {}
+
+    virtual SizeF GetSize()
+    {
+        return {width, height};
+    }
+};
+
+
+extern "C" DLL_EXPORT C* STDMETHODCALLTYPE CreateInstanceOfC(float width, float height)
+{
+    return new C(width, height);
+}
diff --git a/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallTest.cs b/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallTest.cs
new file mode 100644
index 0000000000..ea7b7dacc3
--- /dev/null
+++ b/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallTest.cs
@@ -0,0 +1,62 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System.Runtime.InteropServices;
+using System;
+using System.Reflection;
+using System.Text;
+using TestLibrary;
+
+unsafe class ThisCallNative
+{
+    public struct C
+    {
+        public struct VtableLayout
+        {
+            public IntPtr getSize;
+        }
+
+        public VtableLayout* vtable;
+        private int c;
+        private float width;
+        private float height;
+    }
+
+    public struct SizeF
+    {
+        public float width;
+        public float height;
+    }
+
+    [UnmanagedFunctionPointer(CallingConvention.ThisCall)]
+    public delegate SizeF GetSizeFn(C* c);
+
+    [DllImport(nameof(ThisCallNative))]
+    public static extern C* CreateInstanceOfC(float width, float height);
+}
+
+class ThisCallTest
+{
+    public unsafe static int Main(string[] args)
+    {
+        try
+        {
+            float width = 1.0f;
+            float height = 2.0f;
+            ThisCallNative.C* instance = ThisCallNative.CreateInstanceOfC(width, height);
+            ThisCallNative.GetSizeFn callback = Marshal.GetDelegateForFunctionPointer<ThisCallNative.GetSizeFn>(instance->vtable->getSize);
+
+            ThisCallNative.SizeF result = callback(instance);
+
+            Assert.AreEqual(width, result.width);
+            Assert.AreEqual(height, result.height);
+        }
+        catch (System.Exception ex)
+        {
+            Console.WriteLine(ex);
+            return 101;
+        }
+        return 100;
+    }
+}
diff --git a/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallTest.csproj b/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallTest.csproj
new file mode 100644
index 0000000000..f457f14c49
--- /dev/null
+++ b/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallTest.csproj
@@ -0,0 +1,32 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
+  <PropertyGroup>
+    <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
+    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
+    <AssemblyName>ThisCallTest</AssemblyName>
+    <SchemaVersion>2.0</SchemaVersion>
+    <ProjectGuid>{F1E66554-8C8E-4141-85CF-D0CD6A0CD0B0}</ProjectGuid>
+    <OutputType>Exe</OutputType>
+    <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
+    <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\..\</SolutionDir>
+    <DefineConstants>$(DefineConstants);STATIC</DefineConstants>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+  </PropertyGroup>
+  <!-- Default configurations to help VS understand the configurations -->
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Debug|x64'"></PropertyGroup>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Release|x64'"></PropertyGroup>
+  <ItemGroup>
+    <CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
+      <Visible>False</Visible>
+    </CodeAnalysisDependentAssemblyPaths>
+  </ItemGroup>
+  <ItemGroup>
+    <Compile Include="ThisCallTest.cs" />
+  </ItemGroup>
+  <ItemGroup>
+    <ProjectReference Include="CMakeLists.txt" />
+  </ItemGroup>
+  <Import Project="../../../Interop.settings.targets" />
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+</Project>