Allow Marshal.GetFunctionPointerForDelegate() to work with generic types

[jonpryor]

There is a difference between Mono & .NET Framework/.NET Core regarding the use of Marshal.GetFunctionPointerForDelegate() and generic delegate types. Consider:

using System;
using System.Runtime.InteropServices;

class App {
    public static void Main ()
    {
        Action<int> a = v => {};
        var p = Marshal.GetFunctionPointerForDelegate (a);
    }
}

On .NET Framework, this app fails:

Unhandled Exception: System.ArgumentException: The specified Type must not be a generic type definition.


Parameter name: delegate

   at System.Runtime.InteropServices.Marshal.GetFunctionPointerForDelegateInternal(Delegate d)

   at App.Main()

This works on Mono.

.NET Core currently has the same check as .NET Framework:

runtime/src/coreclr/src/vm/comdelegate.cpp

Line 1228 in fcd862e

COMPlusThrowArgumentException(W("delegate"), W("Argument_NeedNonGenericType"));

---

Rationale: Xamarin.Android currently uses System.Action<...> and System.Func<...> with Marshal.GetFunctionPointerForDelegate() to register function pointers with JNI. This has always worked on Mono, and Xamarin.Android uses mono, so it's been Fine.

However, it would be nice to use Xamarin.Android's JNI infrastructure on .NET Core. At present, if this were to be done it would fail as soon as we hit Marshal.GetFunctionPointerForDelegate() for method registration.

The Xamarin.Android team could instead alter their binding infrastructure so that Action<...> and Func<...> are not used. The Xamarin.Android team would like to know if this code generator change is required for eventual .NET Core support, or if we can instead forego this change.

---

Thanks to the Similar issues window, #4547 was suggested. I'm not sure if this is entirely duplicative or not, but Issue #4547 is currently Closed, though it was also added to the .NET 5 milestone, so I'm not entirely sure if these are the same or not.

[jkotas]

Marshal.GetFunctionPointerForDelegate for generic types is not straightforward to implement in CoreCLR, and it is even more difficult to implement a good AOT compilation scheme for it.

The recommend way to do this for .NET 5 and beyond is going to use the NativeCallableAttribute (#32462) and C# function pointers (https://github.com/dotnet/csharplang/blob/master/proposals/function-pointers.md). This will remove the delegates from the picture completely and make the whole thing faster, smaller and easy for ahead-of-time compilation.

My recommendation would be:

• Keep GetFunctionPointerForDelegate on Action for now.
• Once the .NET 5 function pointer features comes online, switch Xamarin Android to use it. It will make it compatible with CoreCLR, and also faster and smaller.

cc @AaronRobinsonMSFT

[AaronRobinsonMSFT]

but Issue #4547 is currently Closed, though it was also added to the .NET 5 milestone, so I'm not entirely sure if these are the same or not.

@jonpryor The discussion was had and decided that it wasn't the best solution due to the better approach described by @jkotas above. Does the recommendation above make sense to you?

[jonpryor]

Does the recommendation above make sense to you?

I'm not sure it does, likely because I'm not fully understanding the "function pointers" proposal.

What we need to be able to do is call JNIEnv::RegisterNatives():

typedef struct {
    char *name;
    char *signature;
    void *fnPtr;
} JNINativeMethod;

typedef const struct JNINativeInterface_ *JNIEnv

/* partial */ struct JNINativeInterface_ {
    int (*RegisterNatives)(JNIEnv *env, class class, const JNINativeMethod *methods, int nMethods);
}

What we currently do is emit code such as:

partial class /* Java.Lang. */ Object {

	static Delegate cb_toString;
	static Delegate GetToStringHandler ()
	{
		if (cb_toString == null)
			cb_toString = JNINativeWrapper.CreateDelegate ((Func<IntPtr, IntPtr, IntPtr>) n_ToString);
		return cb_toString;
	}

	static IntPtr n_ToString (IntPtr jnienv, IntPtr native__this)
	{
		Java.Lang.Object __this = global::Java.Lang.Object.GetObject<Java.Lang.Object> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
		return JNIEnv.NewString (__this.ToString ());
	}
}

We then lookup Object.GetToStringHandler() at runtime via Reflection, then execute it, and pass the returned Delegate instance to a JniNativeMethodRegistration instance, which relies on the normal P/Invoke marshaler to marshal the Delegate to a function pointer in native code:

• https://github.com/xamarin/xamarin-android/blob/2fd73856bde0b0800b2455485cf15e6b4b393bec/src/Mono.Android/Android.Runtime/AndroidRuntime.cs#L373-L378
• https://github.com/xamarin/java.interop/blob/27cfd452c6e543374728ace77af05ab65f9952b6/src/Java.Interop/Java.Interop/JniNativeMethodRegistration.cs

We don't explicitly invoke Marshal.GetFunctionPointerForDelegate(). It's implicit via P/Invoke struct marshaling.

I'm not immediately sure how to convert the above code into C#8 function pointers. I'd almost certainly need to drop the use of Delegate entirely, e.g.:

public struct JniNativeMethodRegistration {
	public  string      Name;
	public  string      Signature;
	public  IntPtr      Marshaler;

	public JniNativeMethodRegistration (string name, string signature, IntPtr marshaler)
	{
		Name        = name      ?? throw new ArgumentNullException (nameof (name));
		Signature   = signature ?? throw new ArgumentNullException (nameof (signature));
		Marshaler   = marshaler == IntPtr.Zero ? throw new ArgumentNullException (nameof (marshaler)) : marshaler;
	}
}

But I still need to get that IntPtr, and "know" that the IntPtr is a C callable function pointer. The Function Pointers doc suggests that delegate* can be converted to a void*:

        delegate*<void> ptr1 = &Util.Log;
        void* v = &Util.Log;

but that in no means I can pass v to C code and it can be executed! (That may be intended, but it's not explicit.). Especially when the doc also states:

This means invocation of a delegate* will use calli where invocation of a delegate will use callvirt on the Invoke method

Additionally, note the JNINativeWrapper.CreateDelegate() invocation in the above snippet. This uses System.Reflection.Emit to generate a new delegate, which "wraps" n_ToString() in a try/catch block for exception marshaling purposes.

I'm going to go out on a limb and guess that there's no way to use System.Reflection.Emit-generated delegate instances with delegate*.

I am not immediately convinced that Function Pointers are a path forward, and updating our generator to instead avoid Action<...> and Func<...> by emitting new families of non-generic delegate types for our dispatch infrastructure may be the easier path forward.

[jkotas]

but that in no means I can pass v to C code and it can be executed!

If the method is marked with NativeCallable attribute, you can absolutely pass vto C code and it can be executed! The C# function pointers spec do not have the details on NativeCallable, but I expect that it is something we are going to fix as we work through the end-to-end scenarios. cc @AaronRobinsonMSFT @333fred

So you would produce this (no static fields, no delegate objects):

[NativeCallable]
static IntPtr n_ToString (IntPtr jnienv, IntPtr native__this)
{
    try
    {
        Java.Lang.Object __this = global::Java.Lang.Object.GetObject<Java.Lang.Object> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
        return JNIEnv.NewString (__this.ToString ());
    }
    catch (Exception e)
    {
        ... whatever you generate with Reflection.Emit today ...        
    }
}

And then pass the address of n_ToString to registration.

there's no way to use System.Reflection.Emit-generated delegate instances with delegate*

You should be able to emit method with NativeCallableAttribute, but it seems questionable to me. Statically generated code for this should be a lot better - it would certainly be orders of magnitude smaller and faster than Reflection.Emit-based solution when run on CoreCLR.

[AaronRobinsonMSFT]

@jonpryor See #33005 for the NativeCallableAttribute PR. This is something that should help with your scenario. It is close to complete, but unfortunately isn't as useful as desired until the C# function pointer work is in. See NativeCallableTest.cs for examples of use without C# function pointers.

[SupinePandora43]

Due to lack of recent activity, this issue has been marked as a candidate for backlog cleanup. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will undo this process.

This process is part of our issue cleanup automation.

This is a needed feature that will remove need for casting all Action<T> to their non-generic delegate variants,