JNI Marshal Methods & [UnmanagedCallersOnly]?

[jonpryor]

Context: dotnet/runtime#65853

generator emits "JNI Marshal Methods", which are methods to be called by Java/JNI, which marshals parameters and return types in order to "forward" to virtual method overrides:

https://github.com/xamarin/java.interop/blob/1987829f96d58c3298fa1e3d86ef3cd0e12e6c31/tests/generator-Tests/expected.xaji/NormalMethods/Xamarin.Test.A.cs#L52-L56

As these methods are intended to only be called by Java, it would make sense to apply the UnmanagedCallersOnlyAttribute custom attribute to them. This wouldn't be immediately useful, but would permit future optimization opportunities.

Unfortunately, "just" adding [UnmanagedCallersOnly] to generator output is insufficient:

If you call GetFunctionPointerForDelegate on this delegate, you would end up doing the transition twice, so you'd end up in the right GC mode afterwards, but it might put the runtime into a weird state as it would transition from native to managed twice and then from managed to native twice.

which just sounds Bad™.

Thus, in order to use [UnmanagedCallersOnly], we would also need to:

1. Stop using System.Reflection.Emit / JNINativeWrapper.CreateDelegate() entirely (which might halt this entire thought process), as [UnmanagedCallersOnly] can't be called by managed code.

2. Turn JniNativeMethodRegistration.Marshaler into a "union", so that it can be either a Delegate or an `IntPtr.

   Can that even be done without breaking ABI?

3. Update JNI marshal method lookup…somehow… so that the looked-up method uses RuntimeMethodHandle.GetFunctionPointer() to set JniNativeMethodRegistration.Marshaler-as-IntPtr instead of -as-Delegate. (Plus figure out how RegisterNativeMembers() is supposed to know when it should be using the Delegate codepath vs. the IntPtr codepath…)

[jonpryor]

Why does everything keep coming back to making jnimarshalmethod-gen work? :-/

[jonpryor]

Turn JniNativeMethodRegistration.Marshaler into a "union", so that it can be either a Delegate or an `IntPtr.

Can that even be done without breaking ABI?

Lol, no. Marshaling Classes, Structures, and Unions > Unions sample mentions:

In managed code, value types and reference types are not permitted to overlap.

Which means that this C# type is not valid:

[StructLayout(LayoutKind.Explicit)]
public struct JniNativeMethodRegistration_Marshaler {
    [FieldOffset(0)] public Delegate Delegate;
    [FieldOffset(0)] public IntPtr Pointer;
}

Attempting to use this type will result in a runtime exception:

System.TypeLoadException: Could not load type 'JniNativeMethodRegistration_Marshaler' from assembly '…' because it contains an object field at offset 0 that is incorrectly aligned or overlapped by a non-object field

Thus, turning JniNativeMethodRegistration.Marshaler into a union is a non-starter, even before we get to potential field type changes.

Thus, we would need to instead:

1. Add a new member to JniNativeMethodRegistration:

   partial struct JniNativeMethodRegistration {
       public JniNativeMethodRegistration (string name, string signature, IntPtr marshaler);
       public IntPtr FunctionMarshaler;
   }

2. Update all public APIs which use JniNativeMethodRegistration so that JniNativeMethodRegistration is no longer the P/Invoke marshal type. Instead, the JniNativeMethodRegistration instance would need to be copied into a new (internal!) marshal type.

3. We would migrate from "implicit" P/Invoke delegate marshaling to explicit P/Invoke delegate marshaling.

(2) and (3) would result in e.g.

struct _JniNativeMethod {
    public string name, signature;
    public IntPtr fnPtr;
}
partial class JniEnvironment {
    partial class Types {
        // Updated from/via jnienv-gen
        internal static unsafe int _RegisterNatives (JniObjectReference type, _JniNativeMethod [] methods, int numMethods) => …

        // Update: https://github.com/xamarin/java.interop/blob/1987829f96d58c3298fa1e3d86ef3cd0e12e6c31/src/Java.Interop/Java.Interop/JniEnvironment.Types.cs#L175
        public static void RegisterNatives (JniObjectReference type, JniNativeMethodRegistration [] methods, int numMethods)
        {
            …
            _JniNativeMethod[] realMethods = new _JniNativeMethod[methods.Length];
            for (int i = 0; i < realMethods.Length; ++i) {
                realMethods[i].name = methods[i].Name;
                realMethods[i].signature = methods[i].Signature;
                if (methods[i].FunctionMarshaler != IntPtr.Zero && methods[i].Marshaler != null) throw new Exception(…);
                realMethods[i].fnPtr = methods[i].FunctionMarshaler;
                if (realMethods[i].fnPtr == IntPtr.Zero)
                     realMethods[i] = Marshal.GetFunctionPointerForDelegate(methods[i].Marshaler);
            }
            int r   = _RegisterNatives (type, realMethods, numMethods);
            …
        }
    }
}

This should use stackalloc instead of new …, but that aside, this means there's O(n) overhead to every JniEnvironment.Types.RegisterNatives() invocation, as JniNativeMethodRegistration is "marshaled" into _JniNativeMethod. 🙁

[jonpryor]

This is relatedly good to know:

If you need to get a delegate for an UnmanagedCallersOnly method, you can call Marshal.GetDelegateForFunctionPointer on the return value from RuntimeMethodHandle.GetFunctionPointer(). As mentioned previously, the docs aren't quite up-to-date in this area. This is slightly less efficient, but if you prefer to do delegate-based interop, it should work for your scenario.
…
With this snippet, you now have a delegate that is safe to call from managed code and do all the things you can do to a delegate

This might be perfect for us. If it results in a delegate which is safe to call from managed code, then it means we don't need to worry about removing our System.Reflection.Emit usage just yet; instead, we'd just have to update our "JNI marshal infrastructure":

			static Delegate cb_setCustomDimension_I;
#pragma warning disable 0169
			static Delegate GetSetCustomDimension_IHandler ()
			{
				if (cb_setCustomDimension_I == null) {
					var x = Marshal.GetDelegateForFunctionPointer<_JniMarshal_PPI_L>(
						typeof(A).GetMethod(nameof(n_SetCustomDimension_I)).MethodHandle.GetFunctionPointer());
					cb_setCustomDimension_I = JNINativeWrapper.CreateDelegate (x);
				}
				return cb_setCustomDimension_I;
			}

			[UnmanagedCallersOnly]
			static IntPtr n_SetCustomDimension_I (IntPtr jnienv, IntPtr native__this, int index)
			{
				var __this = global::Java.Lang.Object.GetObject<global::Xamarin.Test.A.B> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
				return JNIEnv.ToLocalJniHandle (__this.SetCustomDimension (index));
			}
#pragma warning restore 0169

While this looks like it should work -- the "binding assembly" & AndroidRuntime interaction remains Delegate based, so no form of special-casing is needed -- I'm concerned about the perf implications of adding typeof(T).GetMethod(_X) instead of just referencing _X.

This might not be useful because of performance concerns.