Workaround users having strongly typed HRESULTs on instance methods such as PreserveSig'd COM members

src/vm/ilmarshalers.h

@@ -618,6 +618,41 @@ class ILMarshaler
 #else
                 byrefNativeReturn = true;
 #endif
+                // We need to adjust the treatment of a struct with one field to be the type of the field
+                // in the struct for return values on member functions because some teams such as WPF create wrapper structs
+                // for HRESULTs on PreserveSig'd members. Since native HRESULTs are actually ints, they are returned in-register
+                // by the Windows calling convention. However, structs are never returned in registers from member functions on Windows
+                // unless the type is an HFA (on ARM32). So, by defining an HRESULT struct return type for a member function that returns a native HRESULT,
+                // the user is violating the Windows ABI. To avoid breaking people who have been violating the Windows ABI but have been working
+                // because .NET Framework and .NET Core before 3.0 didn't implement this facet of the Windows ABI correctly,
+                // we actively violate the Windows ABI when the user provides
+                // a struct return type of a member function that has only one field.
+                bool forceViolatingByValueReturn = false;
+                TypeHandle th = nativeType.InternalToken;
+                // Every struct will either have zero fields, multiple fields, or a single field.
+                // If it has a single field, get the type of that field.
+                // If that type is a value type with a single field that isn't a primitive, repeat.
+                // Eventually we will reach a type that has zero fields, multiple fields, or a single field of primitive type.
+                // If we reach a single field of primitive type, then set byrefNativeReturn to false.
+                // This also works for ARM HFAs since a struct containing one HFA field would already have set byrefNativeReturn to false.
+                do
+                {
+                    MethodTable *pMT = th.GetMethodTable();
+                    if(pMT == NULL || !pMT->IsValueType() || pMT->GetNumInstanceFields() != 1)
+                    {
+                        break;
+                    }
+
+                    // get the only instance field
+                    PTR_FieldDesc fieldDesc = pMT->GetApproxFieldDescListRaw();
+                    th = fieldDesc->GetApproxFieldTypeHandleThrowing();
+                }
+                while (!th.GetMethodTable()->IsTruePrimitive());
+
+                if (th.AsMethodTable()->IsTruePrimitive())
+                {
+                    byrefNativeReturn = false;
+                }
             }
             else
             {

src/vm/mlinfo.cpp

@@ -1645,30 +1645,9 @@ 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.
-    // 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
-    {
-        SigPointer sigtmp = sig;
-        CorElementType closedElemType = sigtmp.PeekElemTypeClosed(pModule, pTypeContext);
-        if (closedElemType == ELEMENT_TYPE_VALUETYPE)
-        {
-            TypeHandle th = sigtmp.GetTypeHandleThrowing(pModule, pTypeContext); 
-            // If the return type of an instance method is a value-type we need the actual return type.
-            // However, if the return type is an enum, we can normalize it.
-            if (!th.IsEnum())
-            {
-                mtype = closedElemType;
-            }
-        }
-
-    }
+    VerifyAndAdjustNormalizedType(pModule, sig, pTypeContext, &mtype, &nativeType);
 #endif // _TARGET_X86_
 
-
     if (nativeType == NATIVE_TYPE_CUSTOMMARSHALER)
     {
         switch (mtype)
@@ -2774,7 +2753,6 @@ 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*)

tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallNative.cpp

@@ -48,6 +48,11 @@ class C
     {
         return {(int)height};
     }
+
+    virtual int GetInt(int i)
+    {
+        return i;
+    }
 };
 
 

tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallTest.cs

@@ -17,6 +17,7 @@ public struct VtableLayout
             public IntPtr getSize;
             public IntPtr getWidth;
             public IntPtr getHeightAsInt;
+            public IntPtr getInt;
         }
 
         public VtableLayout* vtable;
@@ -31,12 +32,29 @@ public struct SizeF
         public float height;
     }
 
+    [StructLayout(LayoutKind.Explicit)]
     public struct Width
     {
+        // Add a duplicate field so we can still test a 4-byte HFA return type being passed byref
+        // without hitting our workaround for single-field struct returns on instance methods.
+        [FieldOffset(0)]
+        private float dummyField;
+        [FieldOffset(0)]
         public float width;
     }
 
-    public struct IntWrapper
+    [StructLayout(LayoutKind.Explicit)]
+    public struct HeightAsInt
+    {
+        // Add a duplicate field so we can still test a 4-byte non HFA return type being passed byref
+        // without hitting our workaround for single-field struct returns on instance methods.
+        [FieldOffset(0)]
+        private int dummyField;
+        [FieldOffset(0)]
+        public int i;
+    }
+
+    public struct StructWrappingSingleInt
     {
         public int i;
     }
@@ -46,7 +64,12 @@ public struct IntWrapper
     [UnmanagedFunctionPointer(CallingConvention.ThisCall)]
     public delegate Width GetWidthFn(C* c);
     [UnmanagedFunctionPointer(CallingConvention.ThisCall)]
-    public delegate IntWrapper GetHeightAsIntFn(C* c);
+    public delegate HeightAsInt GetHeightAsIntFn(C* c);
+
+    // We specifically don't want to match the return type here so that we can correctly test
+    // the workaround.
+    [UnmanagedFunctionPointer(CallingConvention.ThisCall)]
+    public delegate StructWrappingSingleInt GetIntFn(C* c, int value);
 
     [DllImport(nameof(ThisCallNative))]
     public static extern C* CreateInstanceOfC(float width, float height);
@@ -64,6 +87,7 @@ public unsafe static int Main(string[] args)
             Test8ByteHFA(instance);
             Test4ByteHFA(instance);
             Test4ByteNonHFA(instance);
+            TestSingleFieldStructReturnWorkaround(instance);
         }
         catch (System.Exception ex)
         {
@@ -96,8 +120,18 @@ private static unsafe void Test4ByteNonHFA(ThisCallNative.C* instance)
     {
         ThisCallNative.GetHeightAsIntFn callback = Marshal.GetDelegateForFunctionPointer<ThisCallNative.GetHeightAsIntFn>(instance->vtable->getHeightAsInt);
 
-        ThisCallNative.IntWrapper result = callback(instance);
+        ThisCallNative.HeightAsInt result = callback(instance);
 
         Assert.AreEqual((int)instance->height, result.i);
     }
+
+    private static unsafe void TestSingleFieldStructReturnWorkaround(ThisCallNative.C* instance)
+    {
+        ThisCallNative.GetIntFn callback = Marshal.GetDelegateForFunctionPointer<ThisCallNative.GetIntFn>(instance->vtable->getInt);
+
+        int value = 42;
+        ThisCallNative.StructWrappingSingleInt result = callback(instance, value);
+
+        Assert.AreEqual(value, result.i);
+    }
 }