From 90234b857d507d10eeec3ddeb6436f5b91b249ee Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 12 Apr 2019 17:25:27 -0700 Subject: [PATCH 1/2] Workaround users having strongly typed HRESULTs on instance methods such as PreserveSig'd COM members. Add tests verifying behavior. --- src/vm/ilmarshalers.h | 35 ++++++++++++++++ src/vm/mlinfo.cpp | 23 +---------- .../Miscellaneous/ThisCall/ThisCallNative.cpp | 5 +++ .../Miscellaneous/ThisCall/ThisCallTest.cs | 40 +++++++++++++++++-- 4 files changed, 78 insertions(+), 25 deletions(-) diff --git a/src/vm/ilmarshalers.h b/src/vm/ilmarshalers.h index 866b3ec63cb7..032623a8b773 100644 --- a/src/vm/ilmarshalers.h +++ b/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 { diff --git a/src/vm/mlinfo.cpp b/src/vm/mlinfo.cpp index 985eefef1eb2..aeed6f82470a 100644 --- a/src/vm/mlinfo.cpp +++ b/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) diff --git a/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallNative.cpp b/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallNative.cpp index 3424dbf20aa1..88a1b1f66692 100644 --- a/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallNative.cpp +++ b/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallNative.cpp @@ -48,6 +48,11 @@ class C { return {(int)height}; } + + virtual int GetInt(int i) + { + return i; + } }; diff --git a/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallTest.cs b/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallTest.cs index a39a0fa128da..89799e2eaae3 100644 --- a/tests/src/Interop/PInvoke/Miscellaneous/ThisCall/ThisCallTest.cs +++ b/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(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(instance->vtable->getInt); + + int value = 42; + ThisCallNative.StructWrappingSingleInt result = callback(instance, value); + + Assert.AreEqual(value, result.i); + } } From ddf0154a81659543f5dc4d780135f1bf18614b30 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 12 Apr 2019 17:41:43 -0700 Subject: [PATCH 2/2] Undo another check on x86 to cut out our special denormalization work that is no longer needed --- src/vm/mlinfo.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/vm/mlinfo.cpp b/src/vm/mlinfo.cpp index aeed6f82470a..2f432f05f0e8 100644 --- a/src/vm/mlinfo.cpp +++ b/src/vm/mlinfo.cpp @@ -2753,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*)