-
Notifications
You must be signed in to change notification settings - Fork 536
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
[XABT] Avoid non-blittable types in native callback methods #9724
base: main
Are you sure you want to change the base?
Conversation
a23c154
to
38e8ca4
Compare
38e8ca4
to
bffa04c
Compare
bffa04c
to
58858cd
Compare
|
||
// Because these types are marshaled as different blittable types, | ||
// we need to accept them as equivalent | ||
static readonly Tuple<string, string>[] equivalent_types = new [] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer the use of "named ValueTuple<…>
":
static readonly (string Source, string Replacement)[] equivalent_types = new[]{
(Source: "System.Boolean", Replacement: "System.SByte"),
(Source: "System.Char", Replacement: "System.UInt16"),
};
Then TypeMatches()
can use .Source
instead of .Item1
, etc.
(Source: "System.Char", Replacement: "System.UInt16"), | ||
]; | ||
|
||
static bool TypeMatches (string type, string methodType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The need for this entire method confuses me.
From the PR message:
Changing callback methods to blittable types (bool -> sbyte and char -> ushort) causes warnings when the marshal method classifier 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')
but why? The invalid return type: expected
message is from here:
android/src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodsClassifier.cs
Lines 211 to 214 in dbda951
if (String.Compare (returnType, method.ReturnType.FullName, StringComparison.Ordinal) != 0) { | |
log.LogWarning ($"Method '{method.FullName}' doesn't match native callback signature (invalid return type: expected '{returnType}', found '{method.ReturnType.FullName}')"); | |
return false; | |
} |
returnType
is MapType(target.ReturnType)
, which uses the verbatimTypes
set, and thus should be System.SByte
(which is in verbatimTypes
), but apparently isn't?
Why doesn't the existing code work? Why is this method needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm understanding what this code does, it is trying to match bound methods to their native callback methods.
Thus, given this bound method:
[Register ("markSupported", "()Z", "GetMarkSupportedHandler")]
public virtual unsafe bool MarkSupported () { ... }
It is trying to find this callback with a matching signature:
static bool n_MarkSupported (IntPtr jnienv, IntPtr native__this) { ... }
However, because the native callback is now sbyte
instead of bool
the return types no longer match and thus the "new" callback is rejected because bool
!= sbyte
:
static sbyte n_MarkSupported (IntPtr jnienv, IntPtr native__this) { ... }
The method I added simply says that for this purpose bool
and sbyte
are the same.
Context: dotnet/java-interop#1296
Context: dotnet/java-interop#1027
Changing callback methods to blittable types (
bool
->sbyte
andchar
->ushort
) causes warnings when the marshal method classifier verifies that the marshal types match the native types:Fix this by treating our blittable types as equivalent to their native types in the marshal method classifier.
Additionally,
Mono.Android
has "built-in delegates" that marshalbool
types (eg:_JniMarshal_PP_Z
). Becausegenerator
no longer creates these delegates (it will now create_JniMarshal_PP_B
instead), we need to add them manually.