Clean up GetDelegateForFunctionPointer() code and test paths (#34655)
authorAaron Robinson <arobins@microsoft.com>
Wed, 8 Apr 2020 04:50:05 +0000 (21:50 -0700)
committerGitHub <noreply@github.com>
Wed, 8 Apr 2020 04:50:05 +0000 (21:50 -0700)
Update comments for GetDelegateForFunctionPointer() code path
Remove dead code
Update GetDelegateForFunctionPointer() tests in CoreCLR.
Removed duplicate tests that are covered in libraries test suite.

src/coreclr/src/vm/comdelegate.cpp
src/coreclr/src/vm/ilmarshalers.cpp
src/coreclr/tests/src/Interop/MarshalAPI/FunctionPointer/FunctionPointer.cs [new file with mode: 0644]
src/coreclr/tests/src/Interop/MarshalAPI/FunctionPointer/FunctionPointerNative.cpp
src/coreclr/tests/src/Interop/MarshalAPI/FunctionPointer/GetDelForFcnPtr_Negative_Catchable.cs [deleted file]
src/coreclr/tests/src/Interop/MarshalAPI/FunctionPointer/GetFcnPtrForDel_Negative_Catchable.cs [deleted file]
src/coreclr/tests/src/Interop/MarshalAPI/FunctionPointer/GetFcnPtrForDel_Negative_Security.cs [deleted file]
src/coreclr/tests/src/Interop/MarshalAPI/FunctionPointer/SingleMulticastTest.cs [new file with mode: 0644]
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs

index 9964e30..52e55a3 100644 (file)
@@ -1336,28 +1336,13 @@ OBJECTREF COMDelegate::ConvertToDelegate(LPVOID pCallback, MethodTable* pMT)
     if (DelegateHnd != (LPVOID)INVALIDENTRY)
     {
         // Found a managed callsite
-        OBJECTREF pDelegate = NULL;
-        GCPROTECT_BEGIN(pDelegate);
-
-        pDelegate = ObjectFromHandle((OBJECTHANDLE)DelegateHnd);
-
-        // Make sure we're not trying to sneak into another domain.
-        SyncBlock* pSyncBlock = pDelegate->GetSyncBlock();
-        _ASSERTE(pSyncBlock);
-
-        InteropSyncBlockInfo* pInteropInfo = pSyncBlock->GetInteropInfo();
-        _ASSERTE(pInteropInfo);
-
-        pUMEntryThunk = (UMEntryThunk*)pInteropInfo->GetUMEntryThunk();
-        _ASSERTE(pUMEntryThunk);
-
-        GCPROTECT_END();
-        return pDelegate;
+        return ObjectFromHandle((OBJECTHANDLE)DelegateHnd);
     }
 
     // Validate the MethodTable is a delegate type
+    // See Marshal.GetDelegateForFunctionPointer() for exception details.
     if (!pMT->IsDelegate())
-        COMPlusThrow(kArgumentException);
+        COMPlusThrowArgumentException(W("t"), W("Arg_MustBeDelegate"));
 
     //////////////////////////////////////////////////////////////////////////////////////////////////////////
     // This is an unmanaged callsite. We need to create a new delegate.
index 242d090..8ec77a0 100644 (file)
@@ -143,15 +143,16 @@ void ILDelegateMarshaler::EmitConvertContentsNativeToCLR(ILCodeStream* pslILEmit
     // COMPAT: There is a subtle difference between argument and field marshaling with Delegate types.
     // During field marshaling, the plain Delegate type can be used even though that type doesn't
     // represent a concrete type. Argument marshaling doesn't permit this so we use the public
-    // API which will validate that Delegate and MulticastDelegate aren't directly used.
+    // API which will validate that Delegate isn't directly used.
     if (IsFieldMarshal(m_dwMarshalFlags))
     {
         // Delegate System.Marshal.GetDelegateForFunctionPointerInternal(IntPtr p, Type t)
         pslILEmit->EmitCALL(METHOD__MARSHAL__GET_DELEGATE_FOR_FUNCTION_POINTER_INTERNAL, 2, 1);
         EmitStoreManagedValue(pslILEmit);
 
-        // Field marshalling of delegates supports marshalling back null from native code.
-        // Parameter and return value marshalling does not.
+        // Field marshaling of delegates supports marshaling back null from native code.
+        // COMPAT: Parameter and return value marshalling does not marshal back a null value.
+        //    e.g. `extern static void SetNull(ref Action f);` <- f will not be null on return.
         ILCodeLabel* pFinishedLabel = pslILEmit->NewCodeLabel();
         pslILEmit->EmitBR(pFinishedLabel);
         pslILEmit->EmitLabel(pNullLabel);
diff --git a/src/coreclr/tests/src/Interop/MarshalAPI/FunctionPointer/FunctionPointer.cs b/src/coreclr/tests/src/Interop/MarshalAPI/FunctionPointer/FunctionPointer.cs
new file mode 100644 (file)
index 0000000..9aec97a
--- /dev/null
@@ -0,0 +1,75 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+using System;
+using System.Runtime.InteropServices;
+using TestLibrary;
+
+public partial class FunctionPtr
+{
+    static class FunctionPointerNative
+    {
+        [DllImport(nameof(FunctionPointerNative))]
+        public static extern IntPtr GetVoidVoidFcnPtr();
+
+        [DllImport(nameof(FunctionPointerNative))]
+        public static extern bool CheckFcnPtr(IntPtr fcnptr);
+    }
+
+    delegate void VoidDelegate();
+
+    public static void RunGetDelForFcnPtrTest()
+    {
+        Console.WriteLine($"Running {nameof(RunGetDelForFcnPtrTest)}...");
+
+        // Delegate -> FcnPtr -> Delegate
+        {
+            VoidDelegate md = new VoidDelegate(VoidVoidMethod);
+            IntPtr fcnptr = Marshal.GetFunctionPointerForDelegate<VoidDelegate>(md);
+
+            VoidDelegate del = (VoidDelegate)Marshal.GetDelegateForFunctionPointer(fcnptr, typeof(VoidDelegate));
+            Assert.AreEqual(md.Target, del.Target, "Failure - the Target of the funcptr->delegate should be equal to the original method.");
+            Assert.AreEqual(md.Method, del.Method, "Failure - The Method of the funcptr->delegate should be equal to the MethodInfo of the original method.");
+        }
+
+        // Native FcnPtr -> Delegate
+        {
+            IntPtr fcnptr = FunctionPointerNative.GetVoidVoidFcnPtr();
+            VoidDelegate del = (VoidDelegate)Marshal.GetDelegateForFunctionPointer(fcnptr, typeof(VoidDelegate));
+            Assert.AreEqual(null, del.Target, "Failure - the Target of the funcptr->delegate should be null since we provided native funcptr.");
+            Assert.AreEqual("Invoke", del.Method.Name, "Failure - The Method of the native funcptr->delegate should be the Invoke method.");
+
+            // Round trip of a native function pointer is never legal for a non-concrete Delegate type
+            Assert.Throws<ArgumentException>(() =>
+            {
+                Marshal.GetDelegateForFunctionPointer(fcnptr, typeof(Delegate));
+            });
+
+            Assert.Throws<ArgumentException>(() =>
+            {
+                Marshal.GetDelegateForFunctionPointer(fcnptr, typeof(MulticastDelegate));
+            });
+        }
+
+        void VoidVoidMethod()
+        {
+            Console.WriteLine("Simple method to get a delegate for");
+        }
+    }
+
+    public static int Main()
+    {
+        try
+        {
+            RunGetDelForFcnPtrTest();
+            RunGetFcnPtrSingleMulticastTest();
+        }
+        catch (Exception e)
+        {
+            Console.WriteLine($"Test Failure: {e}");
+            return 101;
+        }
+
+        return 100;
+    }
+}
index 4463158..476f966 100644 (file)
@@ -5,9 +5,22 @@
 #include <xplatform.h>
 #include <platformdefines.h>
 
-extern "C" DLL_EXPORT bool __cdecl CheckFcnPtr(bool(STDMETHODCALLTYPE *fcnptr)(__int64))
+namespace
 {
-    if (fcnptr == 0)
+    void VoidVoidImpl()
+    {
+        // NOP
+    }
+}
+
+extern "C" DLL_EXPORT void* GetVoidVoidFcnPtr()
+{
+    return (void*)&VoidVoidImpl;
+}
+
+extern "C" DLL_EXPORT bool CheckFcnPtr(bool(STDMETHODCALLTYPE *fcnptr)(long long))
+{
+    if (fcnptr == nullptr)
     {
         printf("CheckFcnPtr: Unmanaged received a null function pointer");
         return false;
@@ -17,8 +30,3 @@ extern "C" DLL_EXPORT bool __cdecl CheckFcnPtr(bool(STDMETHODCALLTYPE *fcnptr)(_
         return fcnptr(999999999999);
     }
 }
-
-int Return100()
-{
-    return 100;
-}
diff --git a/src/coreclr/tests/src/Interop/MarshalAPI/FunctionPointer/GetDelForFcnPtr_Negative_Catchable.cs b/src/coreclr/tests/src/Interop/MarshalAPI/FunctionPointer/GetDelForFcnPtr_Negative_Catchable.cs
deleted file mode 100644 (file)
index f649c61..0000000
+++ /dev/null
@@ -1,167 +0,0 @@
-// Licensed to the .NET Foundation under one or more agreements.
-// The .NET Foundation licenses this file to you under the MIT license.
-// See the LICENSE file in the project root for more information.
-using System;
-using System.Security;
-using System.Threading;
-using System.Globalization;
-using System.Runtime.InteropServices;
-#pragma warning disable 618
-
-public partial class FunctionPtr
-{  
-
-    public static int RunGetDelForFcnPtrTest()
-    {
-        int retVal = 100;
-        VoidDelegate md = new VoidDelegate(FunctionPtr.Method);
-        IntPtr fcnptr = (IntPtr)0;
-        Console.WriteLine("\r\nTesting Marshal.GetDelegateForFunctionPointer().");
-
-        try
-        {
-            fcnptr = Marshal.GetFunctionPointerForDelegate<VoidDelegate>(md);
-        }
-        catch (Exception e)
-        {
-            retVal = 0;
-            Console.WriteLine("Exception during initialization: {0}", e.ToString());
-            return retVal;
-        }
-
-        try
-        {
-            Marshal.GetDelegateForFunctionPointer((IntPtr)0, typeof(VoidDelegate));
-            retVal = 0;
-            Console.WriteLine("Failure - did not receive an exception while passing 0 as the function pointer");
-        }
-        catch (ArgumentNullException e)
-        {
-            Console.WriteLine("Pass - threw the right exception passing a 0 function pointer");
-        }
-        catch (Exception e)
-        {
-            retVal = 0;
-            Console.WriteLine("Failure - receive an incorrect exception while passing 0 as the function pointer");
-            Console.WriteLine(e);
-        }
-
-        try
-        {
-            Marshal.GetDelegateForFunctionPointer(fcnptr, null);
-            retVal = 0;
-            Console.WriteLine("Failure - did not receive an exception while passing a null type");
-        }
-        catch (ArgumentException e)
-        {
-             Console.WriteLine("Pass - passing a null type received the right exception type, wrong message, message was '{0}'", e.Message);
-        }
-        catch (Exception e)
-        {
-            retVal = 0;
-            Console.WriteLine("Failure - receive an incorrect exception while passing a null type");
-            Console.WriteLine(e);
-        }
-
-        try
-        {
-            Marshal.GetDelegateForFunctionPointer(fcnptr, typeof(Object));
-            retVal = 0;
-            Console.WriteLine("Failure - did not receive an exception while passing a non-delegate type");
-        }
-        catch (ArgumentException e)
-        {
-            Console.WriteLine("Pass - threw the right exception passing a non-delegate type, but a wrong message, message was '{0}'", e.Message);
-        }
-        catch (Exception e)
-        {
-            retVal = 0;
-            Console.WriteLine("Failure - receive an incorrect exception while passing a non-delegate type");
-            Console.WriteLine(e);
-        }
-
-        // Delegate -> FcnPtr -> Delegate
-        try
-        {
-            VoidDelegate del = (VoidDelegate)Marshal.GetDelegateForFunctionPointer(fcnptr, typeof(VoidDelegate));
-            if (del.Target != md.Target)
-            {
-                retVal = 0;
-                Console.WriteLine("Failure - the Target of the funcptr->delegate should be equal to the original method.");
-                Console.WriteLine(del.Target);
-            }
-
-            if (del.Method != md.Method)
-            {
-                retVal = 0;
-                Console.WriteLine("Failure - The Method of the funcptr->delegate should be equal to the MethodInfo of the original method.");
-                Console.WriteLine(del.Method);
-            }
-
-            // Try to call it
-            del();
-
-            Console.WriteLine("Pass - got a delegate for the function pointer.");
-        }
-        catch (Exception e)
-        {
-            retVal = 0;
-            Console.WriteLine("Failure - received exception while converting funcptr to delegate.");
-            Console.WriteLine(e);
-        }
-
-        // Native FcnPtr -> Delegate
-        IntPtr pNativeMemory = IntPtr.Zero;
-        try
-        {
-            // Allocate a piece of native memory which in no way resembles valid function entry point.
-            // CLR will read the first couple of bytes and try to match it to known patterns. We need something
-            // which doesn't look like a reverse pinvoke thunk.
-            pNativeMemory = Marshal.AllocCoTaskMem(64);
-            Marshal.WriteInt32(pNativeMemory, 0);
-            Marshal.WriteInt32(pNativeMemory + 4, 0);
-
-            VoidDelegate del = (VoidDelegate)Marshal.GetDelegateForFunctionPointer(pNativeMemory, typeof(VoidDelegate));
-            if (del.Target != null)
-            {
-                retVal = 0;
-                Console.WriteLine("Failure - the Target of the funcptr->delegate should be null since we provided native funcptr.");
-                Console.WriteLine(del.Target);
-            }
-
-            if (del.Method.Name != "Invoke")
-            {
-                retVal = 0;
-                Console.WriteLine("Failure - The Method of the native funcptr->delegate should be the Invoke method.");
-                Console.WriteLine(del.Method);
-            }
-
-            // Don't try to call it - it's a random address.
-
-            Console.WriteLine("Pass - got a delegate for the function pointer.");
-        }
-        catch (Exception e)
-        {
-            retVal = 0;
-            Console.WriteLine("Failure - received exception while converting funcptr to delegate.");
-            Console.WriteLine(e);
-        }
-        finally
-        {
-            if (pNativeMemory != IntPtr.Zero)
-            {
-                Marshal.FreeCoTaskMem(pNativeMemory);
-            }
-        }
-
-
-        Console.WriteLine(retVal == 100 ? "Done - PASSED" : "Done - FAILED");
-        return retVal;
-    }
-
-    public static void Method()
-    {
-        Console.WriteLine("Simple method to get a delegate for");
-    }
-}
-#pragma warning restore 618
diff --git a/src/coreclr/tests/src/Interop/MarshalAPI/FunctionPointer/GetFcnPtrForDel_Negative_Catchable.cs b/src/coreclr/tests/src/Interop/MarshalAPI/FunctionPointer/GetFcnPtrForDel_Negative_Catchable.cs
deleted file mode 100644 (file)
index 01fc898..0000000
+++ /dev/null
@@ -1,49 +0,0 @@
-// Licensed to the .NET Foundation under one or more agreements.
-// The .NET Foundation licenses this file to you under the MIT license.
-// See the LICENSE file in the project root for more information.
-using System;
-using System.Security;
-using System.Threading;
-using System.Globalization;
-using System.Runtime.InteropServices;
-#pragma warning disable 618
-public partial class FunctionPtr
-{
-    delegate void VoidDelegate();
-
-    public static int Main()
-    {
-        int retVal1 = RunGetFncSecTest();
-
-        int retVal2 = 100;
-        VoidDelegate md = new VoidDelegate(FunctionPtr.Method);
-        Console.WriteLine("\r\nTesting Marshal.GetFunctionPointerForDelegate().");
-
-        try
-        {
-            Marshal.GetFunctionPointerForDelegate<Object>(null);
-            retVal2 = 0;
-            Console.WriteLine("Failure - did not receive an exception while passing null as the delegate");
-        }
-        catch (ArgumentNullException e)
-        {
-            Console.WriteLine("Pass - threw the right exception passing null as the delegate");
-        }
-        catch (Exception e)
-        {
-            retVal2 = 0;
-            Console.WriteLine("Failure - receive an incorrect exception while passing null as the delegate");
-            Console.WriteLine(e);
-        }
-
-        int retVal3 = RunGetDelForFcnPtrTest();
-
-        if (retVal1 != 100)
-            return retVal1;
-        if (retVal2 != 100)
-            return retVal2;
-        return retVal3;
-    }
-  
-}
-#pragma warning restore 618
diff --git a/src/coreclr/tests/src/Interop/MarshalAPI/FunctionPointer/GetFcnPtrForDel_Negative_Security.cs b/src/coreclr/tests/src/Interop/MarshalAPI/FunctionPointer/GetFcnPtrForDel_Negative_Security.cs
deleted file mode 100644 (file)
index de74495..0000000
+++ /dev/null
@@ -1,87 +0,0 @@
-// Licensed to the .NET Foundation under one or more agreements.
-// The .NET Foundation licenses this file to you under the MIT license.
-// See the LICENSE file in the project root for more information.
-using System;
-using System.Security;
-using System.Runtime.InteropServices;
-#pragma warning disable 618
-
-partial class FunctionPtr
-{
-    [DllImport("FunctionPointerNative", CallingConvention=CallingConvention.Cdecl)]
-    public static extern bool CheckFcnPtr(IntPtr fcnptr);
-
-    public delegate bool DelegateWithLong(long l); //Singlecast delegate
-    public delegate void MultiDelegateWithLong(long l); //Multicast delegate
-
-    public static DelegateWithLong del = new DelegateWithLong(FunctionPtr.Method);
-    public static MultiDelegateWithLong multidel = new MultiDelegateWithLong(FunctionPtr.Method2);
-
-    private static IntPtr fcnptr;
-
-    public static int RunGetFncSecTest()
-    {
-        Console.WriteLine("\r\nTesting Marshal.GetDelegateForFunctionPointer().");
-
-        bool pass = true;
-
-        try
-        {
-            fcnptr = Marshal.GetFunctionPointerForDelegate<DelegateWithLong>(del);
-            if (CheckFcnPtr(fcnptr) == true)
-            {
-                Console.WriteLine("\tPass - singlecast case");
-            }
-            else
-            {
-                pass = false;
-                Console.WriteLine("\tFail - singlecast case, created a function pointer but the call failed");
-            }
-        }
-        catch (Exception e)
-        {
-            pass = false;
-            Console.WriteLine("\tFailure - singlecast case");
-            Console.WriteLine(e);
-        }
-
-        try
-        {
-            fcnptr = Marshal.GetFunctionPointerForDelegate<MultiDelegateWithLong>(multidel);
-            CheckFcnPtr(fcnptr);
-            Console.WriteLine("\tPass - multicast case");
-        }
-        catch (Exception e)
-        {
-            pass = false;
-            Console.WriteLine("\tFailure - multicast case");
-            Console.WriteLine(e);
-        }
-
-        if (pass)
-        {
-            Console.WriteLine("Pass - the base case");
-            return 100;
-        }
-        else
-        {
-            Console.WriteLine("Fail - the base case");
-            return 99;
-        }
-    }
-
-    public static bool Method(long l)
-    {
-        if (l != 999999999999)
-            return false;
-        else
-            return true;
-    }
-
-    public static void Method2(long l)
-    {
-        if (l != 999999999999)
-            throw new Exception("Failed multicast call");
-    }
-}
-#pragma warning restore 618
diff --git a/src/coreclr/tests/src/Interop/MarshalAPI/FunctionPointer/SingleMulticastTest.cs b/src/coreclr/tests/src/Interop/MarshalAPI/FunctionPointer/SingleMulticastTest.cs
new file mode 100644 (file)
index 0000000..fb30bb9
--- /dev/null
@@ -0,0 +1,44 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+using System;
+using System.Runtime.InteropServices;
+using TestLibrary;
+
+partial class FunctionPtr
+{
+    public delegate bool DelegateWithLong(long l); //Singlecast delegate
+    public delegate void MultiDelegateWithLong(long l); //Multicast delegate
+
+    public static void RunGetFcnPtrSingleMulticastTest()
+    {
+        Console.WriteLine($"Running {nameof(RunGetFcnPtrSingleMulticastTest)}...");
+
+        DelegateWithLong del = new DelegateWithLong(Method);
+        MultiDelegateWithLong multidel = new MultiDelegateWithLong(Method2);
+
+        {
+            IntPtr fcnptr = Marshal.GetFunctionPointerForDelegate<DelegateWithLong>(del);
+            Assert.IsTrue(FunctionPointerNative.CheckFcnPtr(fcnptr));
+        }
+
+        {
+            IntPtr fcnptr = Marshal.GetFunctionPointerForDelegate<MultiDelegateWithLong>(multidel);
+            FunctionPointerNative.CheckFcnPtr(fcnptr);
+        }
+
+        bool Method(long l)
+        {
+            if (l != 999999999999)
+                return false;
+            else
+                return true;
+        }
+
+        void Method2(long l)
+        {
+            if (l != 999999999999)
+                throw new Exception("Failed multicast call");
+        }
+    }
+}
index f7bbc8b..227e84e 100644 (file)
@@ -873,6 +873,9 @@ namespace System.Runtime.InteropServices
                 throw new ArgumentException(SR.Argument_NeedNonGenericType, nameof(t));
             }
 
+            // COMPAT: This block of code isn't entirely correct.
+            // Users passing in typeof(MulticastDelegate) as 't' skip this check
+            // since Delegate is a base type of MulticastDelegate.
             Type? c = t.BaseType;
             if (c != typeof(Delegate) && c != typeof(MulticastDelegate))
             {