From e88dc7f910b08a4d3edc4972147ff035d164c4c4 Mon Sep 17 00:00:00 2001 From: Atsushi Kanamori Date: Wed, 18 Apr 2018 13:56:42 -0700 Subject: [PATCH] Enable Invoke and GetValue for ref-returning members (#17639) * Enable Invoke and GetValue for ref-returning members https://github.com/dotnet/corefx/issues/15960 Returned magic object is the object pointed to by the ref. If the ref is null, NullReferenceException. * Fix error message * Found way to reuse InvokeUtil::CreateObject * Remove comment, BYREF case. --- src/dlls/mscorrc/mscorrc.rc | 1 + src/dlls/mscorrc/resource.h | 2 + src/mscorlib/Resources/Strings.resx | 4 +- .../src/System/Reflection/RuntimeMethodInfo.cs | 8 ++-- src/vm/invokeutil.cpp | 31 ++++++++++------ src/vm/invokeutil.h | 2 +- src/vm/reflectioninvocation.cpp | 43 ++++++++++++++++++++-- 7 files changed, 69 insertions(+), 22 deletions(-) diff --git a/src/dlls/mscorrc/mscorrc.rc b/src/dlls/mscorrc/mscorrc.rc index 61f5c9a..42c0be4 100644 --- a/src/dlls/mscorrc/mscorrc.rc +++ b/src/dlls/mscorrc/mscorrc.rc @@ -1538,6 +1538,7 @@ BEGIN IDS_EE_TORNSTATE "Unexpected change made to file '%1'." + IDS_INVOKE_NULLREF_RETURNED "The target method returned a null reference." END // These strings are generated from within the EE for streams diff --git a/src/dlls/mscorrc/resource.h b/src/dlls/mscorrc/resource.h index b8e1b2b..49f1d0e 100644 --- a/src/dlls/mscorrc/resource.h +++ b/src/dlls/mscorrc/resource.h @@ -898,3 +898,5 @@ #define IDS_EE_NDIRECT_LOADLIB_MAC 0x263f #define IDS_EE_NDIRECT_GETPROCADDRESS_UNIX 0x2640 #define IDS_EE_ERROR_COM 0x2641 + +#define IDS_INVOKE_NULLREF_RETURNED 0x2642 diff --git a/src/mscorlib/Resources/Strings.resx b/src/mscorlib/Resources/Strings.resx index c843be1..30df7c5 100644 --- a/src/mscorlib/Resources/Strings.resx +++ b/src/mscorlib/Resources/Strings.resx @@ -2935,8 +2935,8 @@ Cannot create arrays of ByRef-like values. - - ByRef return value not supported in reflection invocation. + + ByRef to ByRefLike return values not supported in reflection invocation. Vararg calling convention not supported. diff --git a/src/mscorlib/src/System/Reflection/RuntimeMethodInfo.cs b/src/mscorlib/src/System/Reflection/RuntimeMethodInfo.cs index 0b4f4f2..0d5d2de 100644 --- a/src/mscorlib/src/System/Reflection/RuntimeMethodInfo.cs +++ b/src/mscorlib/src/System/Reflection/RuntimeMethodInfo.cs @@ -41,7 +41,7 @@ namespace System.Reflection // // first take care of all the NO_INVOKE cases. if (ContainsGenericParameters || - ReturnType.IsByRef || + (ReturnType.IsByRef && ReturnType.GetElementType().IsByRefLike) || (declaringType != null && declaringType.ContainsGenericParameters) || ((CallingConvention & CallingConventions.VarArgs) == CallingConventions.VarArgs)) { @@ -443,10 +443,10 @@ namespace System.Reflection { throw new MemberAccessException(); } - // ByRef return are not allowed in reflection - else if (ReturnType.IsByRef) + // ByRef to ByRefLike returns are not allowed in reflection + else if (ReturnType.IsByRef && ReturnType.GetElementType().IsByRefLike) { - throw new NotSupportedException(SR.NotSupported_ByRefReturn); + throw new NotSupportedException(SR.NotSupported_ByRefToByRefLikeReturn); } throw new TargetException(); diff --git a/src/vm/invokeutil.cpp b/src/vm/invokeutil.cpp index 4c1dd4d..823cefa 100644 --- a/src/vm/invokeutil.cpp +++ b/src/vm/invokeutil.cpp @@ -649,9 +649,14 @@ void InvokeUtil::ValidField(TypeHandle th, OBJECTREF* value) COMPlusThrow(kArgumentException,W("Arg_ObjObj")); } -// InternalCreateObject -// This routine will create the specified object from the value -OBJECTREF InvokeUtil::CreateObject(TypeHandle th, void * pValue) { +// +// CreateObjectAfterInvoke +// This routine will create the specified object from the value returned by the Invoke target. +// +// This does not handle the ELEMENT_TYPE_VALUETYPE case. The caller must preallocate the box object and +// copy the value type into it afterward. +// +OBJECTREF InvokeUtil::CreateObjectAfterInvoke(TypeHandle th, void * pValue) { CONTRACTL { THROWS; GC_TRIGGERS; @@ -666,6 +671,9 @@ OBJECTREF InvokeUtil::CreateObject(TypeHandle th, void * pValue) { MethodTable *pMT = NULL; OBJECTREF obj = NULL; + // WARNING: pValue can be an inner reference into a managed object and it is not protected from GC. You must do nothing that + // triggers a GC until the all the data it points to has been captured in a GC-protected location. + // Handle the non-table types switch (type) { case ELEMENT_TYPE_VOID: @@ -682,12 +690,8 @@ OBJECTREF InvokeUtil::CreateObject(TypeHandle th, void * pValue) { goto PrimitiveType; case ELEMENT_TYPE_VALUETYPE: - { - _ASSERTE(!th.IsTypeDesc()); - pMT = th.AsMethodTable(); - obj = pMT->Box(pValue); + _ASSERTE(!"You cannot use this function for arbitrary value types. You must preallocate a box object and copy the value in yourself."); break; - } case ELEMENT_TYPE_CLASS: // Class case ELEMENT_TYPE_SZARRAY: // Single Dim, Zero @@ -718,14 +722,17 @@ OBJECTREF InvokeUtil::CreateObject(TypeHandle th, void * pValue) { { // Don't use MethodTable::Box here for perf reasons PREFIX_ASSUME(pMT != NULL); - obj = AllocateObject(pMT); DWORD size = pMT->GetNumInstanceFieldBytes(); - memcpyNoGCRefs(obj->UnBox(), pValue, size); + + UINT64 capturedValue; + memcpyNoGCRefs(&capturedValue, pValue, size); // Must capture the primitive value before we allocate the boxed object which can trigger a GC. + + INDEBUG(pValue = (LPVOID)0xcccccccc); // We're about to allocate a GC object - can no longer trust pValue + obj = AllocateObject(pMT); + memcpyNoGCRefs(obj->UnBox(), &capturedValue, size); } break; - case ELEMENT_TYPE_BYREF: - COMPlusThrow(kNotSupportedException, W("NotSupported_ByRefReturn")); case ELEMENT_TYPE_END: default: _ASSERTE(!"Unknown Type"); diff --git a/src/vm/invokeutil.h b/src/vm/invokeutil.h index ec8114f..99450d0 100644 --- a/src/vm/invokeutil.h +++ b/src/vm/invokeutil.h @@ -105,7 +105,7 @@ public: // Given a type, this routine will convert an return value representing that // type into an ObjectReference. If the type is a primitive, the // value is wrapped in one of the Value classes. - static OBJECTREF CreateObject(TypeHandle th, void * pValue); + static OBJECTREF CreateObjectAfterInvoke(TypeHandle th, void * pValue); // This is a special purpose Exception creation function. It // creates the TargetInvocationExeption placing the passed diff --git a/src/vm/reflectioninvocation.cpp b/src/vm/reflectioninvocation.cpp index 12a3863..515f173 100644 --- a/src/vm/reflectioninvocation.cpp +++ b/src/vm/reflectioninvocation.cpp @@ -1165,12 +1165,28 @@ FCIMPL5(Object*, RuntimeMethodHandle::InvokeMethod, // if we have the magic Value Class return, we need to allocate that class // and place a pointer to it on the stack. + BOOL hasRefReturnAndNeedsBoxing = FALSE; // Indicates that the method has a BYREF return type and the target type needs to be copied into a preallocated boxed object. + TypeHandle retTH = gc.pSig->GetReturnTypeHandle(); + TypeHandle refReturnTargetTH; // Valid only if retType == ELEMENT_TYPE_BYREF. Caches the TypeHandle of the byref target. BOOL fHasRetBuffArg = argit.HasRetBuffArg(); CorElementType retType = retTH.GetInternalCorElementType(); if (retType == ELEMENT_TYPE_VALUETYPE || fHasRetBuffArg) { gc.retVal = retTH.GetMethodTable()->Allocate(); } + else if (retType == ELEMENT_TYPE_BYREF) + { + refReturnTargetTH = retTH.AsTypeDesc()->GetTypeParam(); + CorElementType refReturnTargetType = refReturnTargetTH.GetInternalCorElementType(); + + // If the target of the byref is a general valuetype (i.e. not one of the primitives), we need to preallocate a boxed object + // to hold the managed return value. + if (refReturnTargetType == ELEMENT_TYPE_VALUETYPE) + { + hasRefReturnAndNeedsBoxing = TRUE; + gc.retVal = refReturnTargetTH.GetMethodTable()->Allocate(); + } + } // Copy "this" pointer if (!pMeth->IsStatic()) { @@ -1396,13 +1412,23 @@ FCIMPL5(Object*, RuntimeMethodHandle::InvokeMethod, gc.retVal = Nullable::NormalizeBox(gc.retVal); } else - if (retType == ELEMENT_TYPE_VALUETYPE) + if (retType == ELEMENT_TYPE_VALUETYPE || hasRefReturnAndNeedsBoxing) { _ASSERTE(gc.retVal != NULL); + if (hasRefReturnAndNeedsBoxing) + { + // Method has BYREF return and the target type is one that needs boxing. We need to copy into the boxed object we have allocated for this purpose. + LPVOID pReturnedReference = *(LPVOID*)&callDescrData.returnValue; + if (pReturnedReference == NULL) + { + COMPlusThrow(kNullReferenceException, IDS_INVOKE_NULLREF_RETURNED); + } + CopyValueClass(gc.retVal->GetData(), pReturnedReference, gc.retVal->GetMethodTable(), gc.retVal->GetAppDomain()); + } // if the structure is returned by value, then we need to copy in the boxed object // we have allocated for this purpose. - if (!fHasRetBuffArg) + else if (!fHasRetBuffArg) { CopyValueClass(gc.retVal->GetData(), &callDescrData.returnValue, gc.retVal->GetMethodTable(), gc.retVal->GetAppDomain()); } @@ -1417,9 +1443,20 @@ FCIMPL5(Object*, RuntimeMethodHandle::InvokeMethod, // If the return type is a Nullable box it into the correct form gc.retVal = Nullable::NormalizeBox(gc.retVal); } + else if (retType == ELEMENT_TYPE_BYREF) + { + // WARNING: pReturnedReference is an unprotected inner reference so we must not trigger a GC until the referenced value has been safely captured. + LPVOID pReturnedReference = *(LPVOID*)&callDescrData.returnValue; + if (pReturnedReference == NULL) + { + COMPlusThrow(kNullReferenceException, IDS_INVOKE_NULLREF_RETURNED); + } + + gc.retVal = InvokeUtil::CreateObjectAfterInvoke(refReturnTargetTH, pReturnedReference); + } else { - gc.retVal = InvokeUtil::CreateObject(retTH, &callDescrData.returnValue); + gc.retVal = InvokeUtil::CreateObjectAfterInvoke(retTH, &callDescrData.returnValue); } while (byRefToNullables != NULL) { -- 2.7.4