Enable Invoke and GetValue for ref-returning members (#17732)
authorAtsushi Kanamori <AtsushiKan@users.noreply.github.com>
Mon, 23 Apr 2018 20:42:24 +0000 (13:42 -0700)
committerGitHub <noreply@github.com>
Mon, 23 Apr 2018 20:42:24 +0000 (13:42 -0700)
* Reapply https://github.com/dotnet/coreclr/pull/17639

* tryagain-wip 4/23/2018 7:27:37 AM - Fix Invoke of enum-returning methods

* Assert for refbufargs implying valuetype

* Catch ref to void in managed layer

src/dlls/mscorrc/mscorrc.rc
src/dlls/mscorrc/resource.h
src/mscorlib/Resources/Strings.resx
src/mscorlib/src/System/Reflection/RuntimeMethodInfo.cs
src/vm/invokeutil.cpp
src/vm/invokeutil.h
src/vm/reflectioninvocation.cpp

index 61f5c9a..42c0be4 100644 (file)
@@ -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
index b8e1b2b..49f1d0e 100644 (file)
 #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
index 8ec81b9..6f25d77 100644 (file)
   <data name="NotSupported_ByRefLikeArray" xml:space="preserve">
     <value>Cannot create arrays of ByRef-like values.</value>
   </data>
-  <data name="NotSupported_ByRefReturn" xml:space="preserve">
-    <value>ByRef return value not supported in reflection invocation.</value>
+  <data name="NotSupported_ByRefToByRefLikeReturn" xml:space="preserve">
+    <value>ByRef to ByRefLike return values not supported in reflection invocation.</value>
+  </data>
+  <data name="NotSupported_ByRefToVoidReturn" xml:space="preserve">
+    <value>ByRef to void return values not supported in reflection invocation.</value>
   </data>
   <data name="NotSupported_CallToVarArg" xml:space="preserve">
     <value>Vararg calling convention not supported.</value>
index 1c23f16..8bebfb3 100644 (file)
@@ -41,7 +41,7 @@ namespace System.Reflection
                     //
                     // first take care of all the NO_INVOKE cases. 
                     if (ContainsGenericParameters ||
-                         ReturnType.IsByRef ||
+                         IsDisallowedByRefType(ReturnType) ||
                          (declaringType != null && declaringType.ContainsGenericParameters) ||
                          ((CallingConvention & CallingConventions.VarArgs) == CallingConventions.VarArgs))
                     {
@@ -61,6 +61,15 @@ namespace System.Reflection
                 return m_invocationFlags;
             }
         }
+
+        private bool IsDisallowedByRefType(Type type)
+        {
+            if (!type.IsByRef)
+                return false;
+
+            Type elementType = type.GetElementType();
+            return elementType.IsByRefLike || elementType == typeof(void);
+        }
         #endregion
 
         #region Constructor
@@ -444,10 +453,13 @@ namespace System.Reflection
             {
                 throw new MemberAccessException();
             }
-            // ByRef return are not allowed in reflection
             else if (ReturnType.IsByRef)
             {
-                throw new NotSupportedException(SR.NotSupported_ByRefReturn);
+                Type elementType = ReturnType.GetElementType();
+                if (elementType.IsByRefLike)
+                    throw new NotSupportedException(SR.NotSupported_ByRefToByRefLikeReturn);    
+                if (elementType == typeof(void))
+                    throw new NotSupportedException(SR.NotSupported_ByRefToVoidReturn);
             }
 
             throw new TargetException();
index 170b0da..a45e81b 100644 (file)
@@ -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;
@@ -663,9 +668,11 @@ OBJECTREF InvokeUtil::CreateObject(TypeHandle th, void * pValue) {
     CONTRACTL_END;
 
     CorElementType type = th.GetSignatureCorElementType();
-    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:
@@ -677,18 +684,6 @@ OBJECTREF InvokeUtil::CreateObject(TypeHandle th, void * pValue) {
         break;
     }
 
-    case ELEMENT_TYPE_FNPTR:
-        pMT = MscorlibBinder::GetElementType(ELEMENT_TYPE_I);
-        goto PrimitiveType;
-
-    case ELEMENT_TYPE_VALUETYPE:
-    {
-        _ASSERTE(!th.IsTypeDesc());
-        pMT = th.AsMethodTable();
-        obj = pMT->Box(pValue);
-        break;
-    }
-
     case ELEMENT_TYPE_CLASS:        // Class
     case ELEMENT_TYPE_SZARRAY:      // Single Dim, Zero
     case ELEMENT_TYPE_ARRAY:        // General Array
@@ -698,35 +693,15 @@ OBJECTREF InvokeUtil::CreateObject(TypeHandle th, void * pValue) {
         obj = *(OBJECTREF *)pValue;
         break;
     
-    case ELEMENT_TYPE_BOOLEAN:      // boolean
-    case ELEMENT_TYPE_I1:           // byte
-    case ELEMENT_TYPE_U1:
-    case ELEMENT_TYPE_I2:           // short
-    case ELEMENT_TYPE_U2:           
-    case ELEMENT_TYPE_CHAR:         // char
-    case ELEMENT_TYPE_I4:           // int
-    case ELEMENT_TYPE_U4:
-    case ELEMENT_TYPE_I8:           // long
-    case ELEMENT_TYPE_U8:       
-    case ELEMENT_TYPE_R4:           // float
-    case ELEMENT_TYPE_R8:           // double
-    case ELEMENT_TYPE_I:
-    case ELEMENT_TYPE_U:
-        _ASSERTE(!th.IsTypeDesc());
-        pMT = th.AsMethodTable();
-    PrimitiveType:
+    case ELEMENT_TYPE_FNPTR:
         {
-            // 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);
+            LPVOID capturedValue = *(LPVOID*)pValue;
+            INDEBUG(pValue = (LPVOID)0xcccccccc); // We're about to allocate a GC object - can no longer trust pValue
+            obj = AllocateObject(MscorlibBinder::GetElementType(ELEMENT_TYPE_I));
+            *(LPVOID*)(obj->UnBox()) = capturedValue;
         }
         break;
     
-    case ELEMENT_TYPE_BYREF:
-        COMPlusThrow(kNotSupportedException, W("NotSupported_ByRefReturn"));
-    case ELEMENT_TYPE_END:
     default:
         _ASSERTE(!"Unknown Type");
         COMPlusThrow(kNotSupportedException);
index 80db622..79ded1d 100644 (file)
@@ -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
index d4f5a99..6dea9b7 100644 (file)
@@ -1092,12 +1092,30 @@ 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) {
+    CorElementType retType = retTH.GetSignatureCorElementType();
+    BOOL hasValueTypeReturn = retTH.IsValueType() && retType != ELEMENT_TYPE_VOID;
+    _ASSERTE(hasValueTypeReturn || !fHasRetBuffArg); // only valuetypes are returned via a return buffer.
+    if (hasValueTypeReturn) {
         gc.retVal = retTH.GetMethodTable()->Allocate();
     }
+    else if (retType == ELEMENT_TYPE_BYREF)
+    {
+        refReturnTargetTH = retTH.AsTypeDesc()->GetTypeParam();
+
+        // If the target of the byref is a value type, we need to preallocate a boxed object to hold the managed return value.
+        if (refReturnTargetTH.IsValueType())
+        {
+            _ASSERTE(refReturnTargetTH.GetSignatureCorElementType() != ELEMENT_TYPE_VOID); // Managed Reflection layer has a bouncer for "ref void" returns.
+            hasRefReturnAndNeedsBoxing = TRUE;
+            gc.retVal = refReturnTargetTH.GetMethodTable()->Allocate();
+        }
+    }
 
     // Copy "this" pointer
     if (!pMeth->IsStatic()) {
@@ -1323,13 +1341,23 @@ FCIMPL5(Object*, RuntimeMethodHandle::InvokeMethod,
         gc.retVal = Nullable::NormalizeBox(gc.retVal);
     }
     else
-    if (retType == ELEMENT_TYPE_VALUETYPE)
+    if (hasValueTypeReturn || 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());
         }
@@ -1344,9 +1372,20 @@ FCIMPL5(Object*, RuntimeMethodHandle::InvokeMethod,
             // If the return type is a Nullable<T> 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) {