From 04f97c9923156b0c695f5fe441ffd14624de8076 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 5 Apr 2019 12:52:00 -0700 Subject: [PATCH] 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. --- src/vm/clrtocomcall.cpp | 2 +- src/vm/comtoclrcall.cpp | 6 +-- src/vm/dispatchinfo.cpp | 4 +- src/vm/dllimport.cpp | 12 +++-- src/vm/ilmarshalers.h | 25 ++++++---- src/vm/mlinfo.cpp | 16 ++++-- src/vm/mlinfo.h | 1 + .../Miscellaneous/ThisCall/ThisCallNative.cpp | 20 ++++++++ .../PInvoke/Miscellaneous/ThisCall/ThisCallTest.cs | 57 +++++++++++++++++++--- 9 files changed, 111 insertions(+), 32 deletions(-) diff --git a/src/vm/clrtocomcall.cpp b/src/vm/clrtocomcall.cpp index e56c4b4..4aba529 100644 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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(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(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(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(instance->vtable->getHeightAsInt); + + ThisCallNative.IntWrapper result = callback(instance); + + Assert.AreEqual((int)instance->height, result.i); + } } -- 2.7.4