Enable return buffers on Windows ARM64 for struct-returning member functions (#23625)

* Enable return buffers on Windows ARM64 for struct-returning member functions. Fixes #23577.

* Update comment to match new condition.

* Enable byref return on all Windows (excluding arm64 HFA. Add more test cases for ThisCall.

* On x86, if we have a normalized return value in an instance method, get back the actual type so that we correctly marshal it via a return buffer.

* Fix param ordering.

* Clean up based on PR feedback.

* Fix accidental variable shadowing.

diff --git a/src/vm/clrtocomcall.cpp b/src/vm/clrtocomcall.cpp
index e56c4b4..4aba529 100644 (file)
--- a/src/vm/clrtocomcall.cpp
+++ b/src/vm/clrtocomcall.cpp
@@ -526,7 +526,7 @@ I4ARRAYREF SetUpWrapperInfo(MethodDesc *pMD)
 
             MarshalInfo Info(msig.GetModule(), msig.GetArgProps(), msig.GetSigTypeContext(), params[iParam],
                              MarshalInfo::MARSHAL_SCENARIO_COMINTEROP, (CorNativeLinkType)0, (CorNativeLinkFlags)0,
-                             TRUE, iParam, numArgs, BestFit, ThrowOnUnmappableChar, FALSE, pMD, TRUE
+                             TRUE, iParam, numArgs, BestFit, ThrowOnUnmappableChar, FALSE, TRUE, pMD, TRUE
     #ifdef _DEBUG
                              , pMD->m_pszDebugMethodName, pMD->m_pszDebugClassName, iParam
     #endif

diff --git a/src/vm/comtoclrcall.cpp b/src/vm/comtoclrcall.cpp
index 98391b9..89ef589 100644 (file)
--- a/src/vm/comtoclrcall.cpp
+++ b/src/vm/comtoclrcall.cpp
@@ -1149,7 +1149,7 @@ void ComCallMethodDesc::InitNativeInfo()
 
             MarshalInfo info(fsig.GetModule(), fsig.GetArgProps(), fsig.GetSigTypeContext(), pFD->GetMemberDef(), MarshalInfo::MARSHAL_SCENARIO_COMINTEROP,
                              (CorNativeLinkType)0, (CorNativeLinkFlags)0, 
-                             FALSE, 0, fsig.NumFixedArgs(), BestFit, ThrowOnUnmappableChar, FALSE, NULL, FALSE
+                             FALSE, 0, fsig.NumFixedArgs(), BestFit, ThrowOnUnmappableChar, FALSE, TRUE, NULL, FALSE
 #ifdef _DEBUG
                              , szDebugName, szDebugClassName, 0
 #endif
@@ -1255,7 +1255,7 @@ void ComCallMethodDesc::InitNativeInfo()
                 MarshalInfo info(msig.GetModule(), msig.GetArgProps(), msig.GetSigTypeContext(), params[iArg],
                                  WinRTType ? MarshalInfo::MARSHAL_SCENARIO_WINRT : MarshalInfo::MARSHAL_SCENARIO_COMINTEROP,
                                  (CorNativeLinkType)0, (CorNativeLinkFlags)0,
-                                 TRUE, iArg, numArgs, BestFit, ThrowOnUnmappableChar, FALSE, pMD, FALSE
+                                 TRUE, iArg, numArgs, BestFit, ThrowOnUnmappableChar, FALSE, TRUE, pMD, FALSE
 #ifdef _DEBUG
                                  , szDebugName, szDebugClassName, iArg
 #endif
@@ -1317,7 +1317,7 @@ void ComCallMethodDesc::InitNativeInfo()
                 MarshalInfo info(msig.GetModule(), msig.GetReturnProps(), msig.GetSigTypeContext(), params[0],
                                     WinRTType ? MarshalInfo::MARSHAL_SCENARIO_WINRT : MarshalInfo::MARSHAL_SCENARIO_COMINTEROP,
                                     (CorNativeLinkType)0, (CorNativeLinkFlags)0,
-                                    FALSE, 0, numArgs, BestFit, ThrowOnUnmappableChar, FALSE, pMD, FALSE
+                                    FALSE, 0, numArgs, BestFit, ThrowOnUnmappableChar, FALSE, TRUE, pMD, FALSE
 #ifdef _DEBUG
                                 ,szDebugName, szDebugClassName, 0
 #endif

diff --git a/src/vm/dispatchinfo.cpp b/src/vm/dispatchinfo.cpp
index 3683ddd..c59e2c2 100644 (file)
--- a/src/vm/dispatchinfo.cpp
+++ b/src/vm/dispatchinfo.cpp
@@ -906,7 +906,7 @@ void DispatchMemberInfo::SetUpMethodMarshalerInfo(MethodDesc *pMD, BOOL bReturnV
 
             MarshalInfo Info(msig.GetModule(), msig.GetArgProps(), msig.GetSigTypeContext(), paramDef, MarshalInfo::MARSHAL_SCENARIO_COMINTEROP,
                              (CorNativeLinkType)0, (CorNativeLinkFlags)0,
-                             TRUE, iParam, numArgs, BestFit, ThrowOnUnmappableChar, FALSE, pMD, TRUE
+                             TRUE, iParam, numArgs, BestFit, ThrowOnUnmappableChar, FALSE, TRUE, pMD, TRUE
     #ifdef _DEBUG
                      , pMD->m_pszDebugMethodName, pMD->m_pszDebugClassName, iParam
     #endif
@@ -943,7 +943,7 @@ void DispatchMemberInfo::SetUpMethodMarshalerInfo(MethodDesc *pMD, BOOL bReturnV
     {
         MarshalInfo Info(msig.GetModule(), msig.GetReturnProps(), msig.GetSigTypeContext(), returnParamDef, MarshalInfo::MARSHAL_SCENARIO_COMINTEROP,
                          (CorNativeLinkType)0, (CorNativeLinkFlags)0,
-                         FALSE, 0, numArgs, BestFit, ThrowOnUnmappableChar, FALSE, pMD, TRUE
+                         FALSE, 0, numArgs, BestFit, ThrowOnUnmappableChar, FALSE, TRUE, pMD, TRUE
 #ifdef _DEBUG
                          , pMD->m_pszDebugMethodName, pMD->m_pszDebugClassName, 0
 #endif

diff --git a/src/vm/dllimport.cpp b/src/vm/dllimport.cpp
index 252ec56..924a50f 100644 (file)
--- a/src/vm/dllimport.cpp
+++ b/src/vm/dllimport.cpp
@@ -3579,7 +3579,7 @@ static MarshalInfo::MarshalType DoMarshalReturnValue(MetaSig&           msig,
                                                      CorNativeLinkFlags nlFlags,
                                                      UINT               argidx,  // this is used for reverse pinvoke hresult swapping
                                                      StubState*         pss,
-                                                     BOOL               fThisCall,
+                                                     BOOL               isInstanceMethod,
                                                      int                argOffset,
                                                      DWORD              dwStubFlags,
                                                      MethodDesc         *pMD,
@@ -3647,6 +3647,7 @@ static MarshalInfo::MarshalType DoMarshalReturnValue(MetaSig&           msig,
                                 SF_IsBestFit(dwStubFlags),
                                 SF_IsThrowOnUnmappableChar(dwStubFlags),
                                 TRUE,
+                                isInstanceMethod,
                                 pMD,
                                 TRUE
                                 DEBUG_ARG(pDebugName)
@@ -3955,6 +3956,7 @@ static void CreateNDirectStubWorker(StubState*         pss,
     BOOL fMarshalReturnValueFirst = FALSE;
 
     BOOL fReverseWithReturnBufferArg = FALSE;
+    bool isInstanceMethod = fStubNeedsCOM || fThisCall;
     
     // 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.
@@ -3963,7 +3965,6 @@ static void CreateNDirectStubWorker(StubState*         pss,
     // 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_)
@@ -3983,7 +3984,7 @@ static void CreateNDirectStubWorker(StubState*         pss,
         // 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_)
+#elif defined(_TARGET_AMD64_) || defined (_TARGET_ARM64_)
         fMarshalReturnValueFirst = isInstanceMethod && isReturnTypeValueType;
 #endif // defined(_TARGET_X86_) || defined(_TARGET_ARM_)
 #ifdef _WIN32
@@ -4036,6 +4037,7 @@ static void CreateNDirectStubWorker(StubState*         pss,
                                                  SF_IsBestFit(dwStubFlags),
                                                  SF_IsThrowOnUnmappableChar(dwStubFlags),
                                                  TRUE,
+                                                 isInstanceMethod ? TRUE : FALSE,
                                                  pMD,
                                                  TRUE
                                                  DEBUG_ARG(pSigDesc->m_pDebugName)
@@ -4092,7 +4094,7 @@ static void CreateNDirectStubWorker(StubState*         pss,
                                            nlFlags,
                                            0,
                                            pss,
-                                           fThisCall,
+                                           isInstanceMethod,
                                            argOffset,
                                            dwStubFlags,
                                            pMD,
@@ -4205,7 +4207,7 @@ static void CreateNDirectStubWorker(StubState*         pss,
                              nlFlags,
                              argidx,
                              pss,
-                             fThisCall,
+                             isInstanceMethod,
                              argOffset,
                              dwStubFlags,
                              pMD,

diff --git a/src/vm/ilmarshalers.h b/src/vm/ilmarshalers.h
index 87d8150..866b3ec 100644 (file)
--- a/src/vm/ilmarshalers.h
+++ b/src/vm/ilmarshalers.h
@@ -602,37 +602,42 @@ public:
                 nativeSize = wNativeSize;
             }
 
-#if defined(_TARGET_X86_) || (defined(_TARGET_AMD64_) && defined(_WIN32))
+#if defined(PLATFORM_WINDOWS)
             // 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 Windows AMD64 and x86, we need to use a return buffer for native member functions returning structures.
+            // For Windows, we need to use a return buffer for native member functions returning structures.
+            // On Windows arm we need to respect HFAs and not use a return buffer if the return type is an HFA
             // 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.
-#if defined(_WIN32)
             if (nativeMethodIsMemberFunction)
             {
+#ifdef _TARGET_ARM_
+                byrefNativeReturn = !nativeType.InternalToken.GetMethodTable()->IsNativeHFA();
+#else
                 byrefNativeReturn = true;
+#endif
             }
             else
-#endif // _WIN32
             {
-#ifndef _TARGET_AMD64_
+#ifdef _TARGET_X86_
                 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 // UNIX_X86_ABI
                     default: byrefNativeReturn = true; break;
                 }
-#endif // _TARGET_AMD64_
+#endif // _TARGET_X86_
             }
-#endif // defined(_TARGET_X86_) || (defined(_TARGET_AMD64_) && defined(_WIN32))
+#endif // defined(PLATFORM_WINDOWS)
+
+            // for UNIX_X86_ABI, we always need a return buffer argument for any size of structs.
+#ifdef UNIX_X86_ABI
+            byrefNativeReturn = true;
+#endif
         }
 
         if (IsHresultSwap(dwMarshalFlags) || (byrefNativeReturn && (IsCLRToNative(m_dwMarshalFlags) || nativeMethodIsMemberFunction)))

diff --git a/src/vm/mlinfo.cpp b/src/vm/mlinfo.cpp
index cb512bb..2e675fd 100644 (file)
--- a/src/vm/mlinfo.cpp
+++ b/src/vm/mlinfo.cpp
@@ -1412,6 +1412,7 @@ MarshalInfo::MarshalInfo(Module* pModule,
                          BOOL BestFit,
                          BOOL ThrowOnUnmappableChar,
                          BOOL fEmitsIL,
+                         BOOL onInstanceMethod,
                          MethodDesc* pMD,
                          BOOL fLoadCustomMarshal
 #ifdef _DEBUG
@@ -1514,8 +1515,9 @@ MarshalInfo::MarshalInfo(Module* pModule,
     }
    
     nativeType = ParamInfo.m_NativeType;
+
     corElemType = sig.PeekElemTypeNormalized(pModule, pTypeContext); 
-    mtype       = corElemType;
+    mtype = corElemType;
 
 #ifdef FEATURE_COMINTEROP
     if (IsWinRTScenario() && nativeType != NATIVE_TYPE_DEFAULT)
@@ -1643,8 +1645,15 @@ MarshalInfo::MarshalInfo(Module* pModule,
     // "un-normalized" signature type. It has to be verified that all the value types
     // that have been normalized away have default marshaling or MarshalAs(Struct).
     // In addition, the nativeType must be updated with the type of the real primitive inside.
-    // 
-    VerifyAndAdjustNormalizedType(pModule, sig, pTypeContext, &mtype, &nativeType);
+    // We don't normalize on return values of member functions since struct return values need to be treated as structures.
+    if (isParam || !onInstanceMethod)
+    {
+        VerifyAndAdjustNormalizedType(pModule, sig, pTypeContext, &mtype, &nativeType);
+    }
+    else
+    {
+        mtype = sig.PeekElemTypeClosed(pModule, pTypeContext);
+    }
 #endif // _TARGET_X86_
 
 
@@ -2753,6 +2762,7 @@ MarshalInfo::MarshalInfo(Module* pModule,
                         // (returning small value types by value in registers) is already done in JIT64.
                         if (        !m_byref   // Permit register-sized structs as return values
                                  && !isParam
+                                 && !onInstanceMethod
                                  && CorIsPrimitiveType(m_pMT->GetInternalCorElementType())
                                  && !IsUnmanagedValueTypeReturnedByRef(nativeSize)
                                  && managedSize <= sizeof(void*)

diff --git a/src/vm/mlinfo.h b/src/vm/mlinfo.h
index 00a417b..46057d5 100644 (file)
--- a/src/vm/mlinfo.h
+++ b/src/vm/mlinfo.h
@@ -455,6 +455,7 @@ public:
                 BOOL BestFit,
                 BOOL ThrowOnUnmappableChar,
                 BOOL fEmitsIL,
+                BOOL onInstanceMethod,
                 MethodDesc* pMD = NULL,
                 BOOL fUseCustomMarshal = TRUE
 #ifdef _DEBUG

diff --git a/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallNative.cpp b/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallNative.cpp
index 9f1d56c..3424dbf 100644 (file)
--- a/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallNative.cpp
+++ b/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallNative.cpp
@@ -12,6 +12,16 @@ struct SizeF
     float height;
 };
 
+struct Width
+{
+    float width;
+};
+
+struct IntWrapper
+{
+    int i;
+};
+
 class C
 {
     int dummy = 0xcccccccc;
@@ -28,6 +38,16 @@ public:
     {
         return {width, height};
     }
+
+    virtual Width GetWidth()
+    {
+        return {width};
+    }
+
+    virtual IntWrapper GetHeightAsInt()
+    {
+        return {(int)height};
+    }
 };
 
 

diff --git a/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallTest.cs b/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallTest.cs
index ea7b7da..a39a0fa 100644 (file)
--- a/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallTest.cs
+++ b/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallTest.cs
@@ -15,12 +15,14 @@ unsafe class ThisCallNative
         public struct VtableLayout
         {
             public IntPtr getSize;
+            public IntPtr getWidth;
+            public IntPtr getHeightAsInt;
         }
 
         public VtableLayout* vtable;
         private int c;
-        private float width;
-        private float height;
+        public readonly float width;
+        public readonly float height;
     }
 
     public struct SizeF
@@ -29,8 +31,22 @@ unsafe class ThisCallNative
         public float height;
     }
 
+    public struct Width
+    {
+        public float width;
+    }
+
+    public struct IntWrapper
+    {
+        public int i;
+    }
+
     [UnmanagedFunctionPointer(CallingConvention.ThisCall)]
     public delegate SizeF GetSizeFn(C* c);
+    [UnmanagedFunctionPointer(CallingConvention.ThisCall)]
+    public delegate Width GetWidthFn(C* c);
+    [UnmanagedFunctionPointer(CallingConvention.ThisCall)]
+    public delegate IntWrapper GetHeightAsIntFn(C* c);
 
     [DllImport(nameof(ThisCallNative))]
     public static extern C* CreateInstanceOfC(float width, float height);
@@ -45,12 +61,9 @@ class ThisCallTest
             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);
+            Test8ByteHFA(instance);
+            Test4ByteHFA(instance);
+            Test4ByteNonHFA(instance);
         }
         catch (System.Exception ex)
         {
@@ -59,4 +72,32 @@ class ThisCallTest
         }
         return 100;
     }
+
+    private static unsafe void Test8ByteHFA(ThisCallNative.C* instance)
+    {
+        ThisCallNative.GetSizeFn callback = Marshal.GetDelegateForFunctionPointer<ThisCallNative.GetSizeFn>(instance->vtable->getSize);
+
+        ThisCallNative.SizeF result = callback(instance);
+
+        Assert.AreEqual(instance->width, result.width);
+        Assert.AreEqual(instance->height, result.height);
+    }
+
+    private static unsafe void Test4ByteHFA(ThisCallNative.C* instance)
+    {
+        ThisCallNative.GetWidthFn callback = Marshal.GetDelegateForFunctionPointer<ThisCallNative.GetWidthFn>(instance->vtable->getWidth);
+
+        ThisCallNative.Width result = callback(instance);
+
+        Assert.AreEqual(instance->width, result.width);
+    }
+
+    private static unsafe void Test4ByteNonHFA(ThisCallNative.C* instance)
+    {
+        ThisCallNative.GetHeightAsIntFn callback = Marshal.GetDelegateForFunctionPointer<ThisCallNative.GetHeightAsIntFn>(instance->vtable->getHeightAsInt);
+
+        ThisCallNative.IntWrapper result = callback(instance);
+
+        Assert.AreEqual((int)instance->height, result.i);
+    }
 }