Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[generator] Avoid non-blittable types in native callback methods #1296

Merged
merged 2 commits into from
Feb 11, 2025

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Jan 29, 2025

Fixes: #1027

To make marshaling callbacks from Java easier we should avoid non-blittable types in native callback methods. This means we should marshal bool as sbyte and char as ushort.

generator has built in support for conversions when the native type does not match the managed type. (ISymbol.[To|From]Native). However this affects marshaling both ways: managed to Java and Java to managed. We do not want to affect managed to Java marshaling, so we introduce a new ISymbol.OnlyFormatOnMarshal property to control this.

This also requires some changes in dotnet\android via this PR: dotnet/android#9724.

@jpobst jpobst force-pushed the dev/jpobst/blittable branch from 099c5a8 to db62a97 Compare January 29, 2025 21:35
@jonpryor
Copy link
Member

jonpryor commented Feb 3, 2025

Looks like char will need to be replaced with uint16 as well; see e.g. https://github.com/dotnet/runtime/blob/3ab8574307f12f4afbb813fbe708734334f87914/docs/design/libraries/LibraryImportGenerator/Compatibility.md#char-marshalling

[char marshaling] will not be supported when configured with any of the following:

  • No explicit marshalling information…

@jpobst jpobst force-pushed the dev/jpobst/blittable branch from ae914d7 to be273b5 Compare February 6, 2025 22:50
@jpobst jpobst marked this pull request as ready for review February 10, 2025 18:54
@jonpryor
Copy link
Member

Draft commit message:

Fixes: https://github.com/dotnet/java-interop/issues/1027

Context: https://github.com/dotnet/android/commit/8bc7a3e84f95e70fe12790ac31ecd97957771cb2
Context: 356485ee41fb61570e84c11ba120d6f8cf81ae09
Context: https://github.com/dotnet/android/pull/9724

`generator --codegen-target=XAJavaInterop1` -emitted marshal methods
can involve non-[blittable][0] types, specifically `bool` and `char`.
Consider:

	namespace Java.Nio.Charset;
	partial class CharsetEncoder {
	    public virtual unsafe bool CanEncode (char c) => …

	    static bool n_CanEncode_C (IntPtr jnienv, IntPtr native__this, char c) => …
	}

Historically this hadn't mattered, because we always had a runtime
marshaler.

This started changing with the introduction of LLVM Marshal Methods
in dotnet/android@8bc7a3e8: instead of the previous method registration
approach, in which a bunch of `Delegate` instances were collected and
passed to `JNIEnv::RegisterNatives()`, the idea was to instead emit
LLVM-IR for a `Java_…` native symbol which would lookup and invoke
a [`[UnmanagedCallersOnly]`][1]-attributed method, and
`[UnmanagedCallersOnly]` requires that all parameter and return types
be *blittable*.  There were two potential solutions:

 1. Add a wrapper around the marshal method which converts parameter
    and return types, or

 2. Skip them entirely, and require use of `JNIEnv::RegisterNatives()`
    for types overriding Java methods which involve `bool` or `char`.

dotnet/android@8bc7a3e8 went with (2).  ((1) is complicated.)

We now introduce solution (3): update `generator` so that the
generated marshal methods always use blittable types:

  * We marshal `bool` as `sbyte`
  * We marshal `char` as `ushort`.

`generator` has built in support for conversions when the native type
does not match the managed type, via `ISymbol.ToNative()` and
`ISymbol.FromNative()`.

However this affects marshaling both ways: managed to Java and Java to
managed.  We do not want to affect managed to Java marshaling, so we
introduce a new `ISymbol.OnlyFormatOnMarshal` property to control this.

This also requires some changes in `dotnet/android`;
see dotnet/android#9724.

[0]: https://learn.microsoft.com/en-us/dotnet/framework/interop/blittable-and-non-blittable-types
[1]: https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.unmanagedcallersonlyattribute?view=net-6.0

@jonpryor jonpryor merged commit 57f7bc8 into main Feb 11, 2025
4 checks passed
@jonpryor jonpryor deleted the dev/jpobst/blittable branch February 11, 2025 20:42
jonpryor pushed a commit to dotnet/android that referenced this pull request Feb 12, 2025
Changes: dotnet/java-interop@6bc87e8...57f7bc8

  * dotnet/java-interop@57f7bc84: [generator] Avoid non-blittable types in native callback methods (dotnet/java-interop#1296)
  * dotnet/java-interop@9369d4fd: [tests] fix `NU1510` warning as error (dotnet/java-interop#1306)

Context: dotnet/java-interop@57f7bc8
Context: dotnet/java-interop#1027

dotnet/java-interop@57f7bc84 updated
`generator --codegen-target=XAJavaInterop1` marshal methods to
contain only blittable types in parameter and return types.
Instead of `bool`, use `sbyte`; instead of `char`, use `ushort`.

This change causes warnings when `MarshalMethodsClassifier` verifies
that the marshal types match the native types:

	…/Xamarin.Android.Common.targets(1502,3): Method 'System.SByte Java.Lang.Object::n_Equals_Ljava_lang_Object_(System.IntPtr,System.IntPtr,System.IntPtr)' doesn't match native callback signature (invalid return type: expected 'System.Boolean', found 'System.SByte')
	…/Xamarin.Android.Common.targets(1502,3): Unable to find native callback method 'n_Equals_Ljava_lang_Object_' in type 'Java.Lang.Object', matching the 'System.Boolean Java.Lang.Object::Equals(Java.Lang.Object)' signature (jniName: 'equals') [Arch: X86_64; Assembly: obj/Release/android-x64/linked/Mono.Android.dll]
	…/Xamarin.Android.Common.targets(1502,3): Method 'System.Boolean Java.Lang.Object::Equals(Java.Lang.Object)' will be registered dynamically [Arch: X86_64; Assembly: obj/Release/android-x64/linked/Mono.Android.dll]
	…/Xamarin.Android.Common.targets(1502,3): Method 'System.SByte Java.Lang.Object::n_Equals_Ljava_lang_Object_(System.IntPtr,System.IntPtr,System.IntPtr)' doesn't match native callback signature (invalid return type: expected 'System.Boolean', found 'System.SByte')
	…/Xamarin.Android.Common.targets(1502,3): Unable to find native callback method 'n_Equals_Ljava_lang_Object_' in type 'Java.Lang.Object', matching the 'System.Boolean Java.Lang.Object::Equals(Java.Lang.Object)' signature (jniName: 'equals') [Arch: Arm64; Assembly: obj/Release/android-arm64/linked/Mono.Android.dll]
	…/Xamarin.Android.Common.targets(1502,3): Method 'System.Boolean Java.Lang.Object::Equals(Java.Lang.Object)' will be registered dynamically [Arch: Arm64; Assembly: obj/Release/android-arm64/linked/Mono.Android.dll]
	…/Xamarin.Android.Common.targets(1502,3): Type 'Android.Runtime.JavaObject, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065' will register some of its Java override methods dynamically. This may adversely affect runtime performance. See preceding warnings for names of dynamically registered methods.
	…/Xamarin.Android.Common.targets(1502,3): [Arm64] Number of methods in the project that will be registered dynamically: 1
	…/Xamarin.Android.Common.targets(1502,3): [X86_64] Number of methods in the project that will be registered dynamically: 1

These warnings occur because the marshal method signature now differs
from the public API signature.  Consider:

	namespace Java.Lang;

	partial class Object {
	    static sbyte n_Equals_Ljava_lang_Object_ (IntPtr jnienv, IntPtr native__this, IntPtr native_obj) => …

	    [Register ("equals", "(Ljava/lang/Object;)Z", "GetEquals_Ljava_lang_Object_Handler")]
	    public virtual unsafe bool Equals (Java.Lang.Object? obj) => …
	}

`MarshalMethodsClassifier` uses the 3rd `connector` parameter to
`RegisterAttribute` to determine the name of the marshal method --
string processing `GetEquals_Ljava_lang_Object_Handler` to obtain
`n_Equals_Ljava_lang_Object_` -- and then checks that the parameter
and return types match between the two methods.

Now that we're emitting blittable types, they *don't* match, thus the
warning.

Fix this by treating our blittable types as equivalent to their
native types in `MarshalMethodsClassifier`.

Additionally, `Mono.Android` has "built-in delegates" that marshal
`bool` types, e.g. `_JniMarshal_PP_Z`.  Because `generator` no longer
creates these delegates -- it will now create `_JniMarshal_PP_B`
instead -- we need to add them manually.
jonathanpeppers pushed a commit to dotnet/android that referenced this pull request Feb 12, 2025
Changes: dotnet/java-interop@6bc87e8...57f7bc8

  * dotnet/java-interop@57f7bc84: [generator] Avoid non-blittable types in native callback methods (dotnet/java-interop#1296)
  * dotnet/java-interop@9369d4fd: [tests] fix `NU1510` warning as error (dotnet/java-interop#1306)

Context: dotnet/java-interop@57f7bc8
Context: dotnet/java-interop#1027

dotnet/java-interop@57f7bc84 updated
`generator --codegen-target=XAJavaInterop1` marshal methods to
contain only blittable types in parameter and return types.
Instead of `bool`, use `sbyte`; instead of `char`, use `ushort`.

This change causes warnings when `MarshalMethodsClassifier` verifies
that the marshal types match the native types:

	…/Xamarin.Android.Common.targets(1502,3): Method 'System.SByte Java.Lang.Object::n_Equals_Ljava_lang_Object_(System.IntPtr,System.IntPtr,System.IntPtr)' doesn't match native callback signature (invalid return type: expected 'System.Boolean', found 'System.SByte')
	…/Xamarin.Android.Common.targets(1502,3): Unable to find native callback method 'n_Equals_Ljava_lang_Object_' in type 'Java.Lang.Object', matching the 'System.Boolean Java.Lang.Object::Equals(Java.Lang.Object)' signature (jniName: 'equals') [Arch: X86_64; Assembly: obj/Release/android-x64/linked/Mono.Android.dll]
	…/Xamarin.Android.Common.targets(1502,3): Method 'System.Boolean Java.Lang.Object::Equals(Java.Lang.Object)' will be registered dynamically [Arch: X86_64; Assembly: obj/Release/android-x64/linked/Mono.Android.dll]
	…/Xamarin.Android.Common.targets(1502,3): Method 'System.SByte Java.Lang.Object::n_Equals_Ljava_lang_Object_(System.IntPtr,System.IntPtr,System.IntPtr)' doesn't match native callback signature (invalid return type: expected 'System.Boolean', found 'System.SByte')
	…/Xamarin.Android.Common.targets(1502,3): Unable to find native callback method 'n_Equals_Ljava_lang_Object_' in type 'Java.Lang.Object', matching the 'System.Boolean Java.Lang.Object::Equals(Java.Lang.Object)' signature (jniName: 'equals') [Arch: Arm64; Assembly: obj/Release/android-arm64/linked/Mono.Android.dll]
	…/Xamarin.Android.Common.targets(1502,3): Method 'System.Boolean Java.Lang.Object::Equals(Java.Lang.Object)' will be registered dynamically [Arch: Arm64; Assembly: obj/Release/android-arm64/linked/Mono.Android.dll]
	…/Xamarin.Android.Common.targets(1502,3): Type 'Android.Runtime.JavaObject, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065' will register some of its Java override methods dynamically. This may adversely affect runtime performance. See preceding warnings for names of dynamically registered methods.
	…/Xamarin.Android.Common.targets(1502,3): [Arm64] Number of methods in the project that will be registered dynamically: 1
	…/Xamarin.Android.Common.targets(1502,3): [X86_64] Number of methods in the project that will be registered dynamically: 1

These warnings occur because the marshal method signature now differs
from the public API signature.  Consider:

	namespace Java.Lang;

	partial class Object {
	    static sbyte n_Equals_Ljava_lang_Object_ (IntPtr jnienv, IntPtr native__this, IntPtr native_obj) => …

	    [Register ("equals", "(Ljava/lang/Object;)Z", "GetEquals_Ljava_lang_Object_Handler")]
	    public virtual unsafe bool Equals (Java.Lang.Object? obj) => …
	}

`MarshalMethodsClassifier` uses the 3rd `connector` parameter to
`RegisterAttribute` to determine the name of the marshal method --
string processing `GetEquals_Ljava_lang_Object_Handler` to obtain
`n_Equals_Ljava_lang_Object_` -- and then checks that the parameter
and return types match between the two methods.

Now that we're emitting blittable types, they *don't* match, thus the
warning.

Fix this by treating our blittable types as equivalent to their
native types in `MarshalMethodsClassifier`.

Additionally, `Mono.Android` has "built-in delegates" that marshal
`bool` types, e.g. `_JniMarshal_PP_Z`.  Because `generator` no longer
creates these delegates -- it will now create `_JniMarshal_PP_B`
instead -- we need to add them manually.
@github-actions github-actions bot locked and limited conversation to collaborators Mar 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid non-blittable types in native callback methods
2 participants