-
Notifications
You must be signed in to change notification settings - Fork 528
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
Marshal methods, Part 1 of ? #7004
Conversation
a37c7cc
to
e6cd07b
Compare
@@ -23,6 +23,10 @@ | |||
<RollForward>Major</RollForward> | |||
</PropertyGroup> | |||
|
|||
<PropertyGroup Condition=" '$(MSBuildRuntimeType)' == 'Core' "> |
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.
Is building with dotnet build
a requirement for this one? When you build inside Visual Studio, it will be Full
using .NET Framework MSBuild.
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.
Yep, marshal methods will be NET7+ only
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.
Well, but you can build .NET 7 projects in Visual Studio with .NET Framework MSBuild.
Does this feature require our build to be running under .NET 7?
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.
We can maybe shell out to dotnet
to make this work, if 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.
Native portion of the feature uses APIs that aren't available in Mono classic, so I wanted to make sure it's not enabled for classic by accident. However, this properly is here just until I finish all the parts of the feature, it will be
removed in the last PR. It's just easier this way for me to keep the feature disabled in main
while I can enable it globally in the branch I'm working in on the next PRs.
8d05292
to
e7eba0a
Compare
@grendello: please provide a description of the proposed architecture, how all the pieces fit together, and what the resulting output "looks like" conceptually. |
I can provide a bird's eye view of the architecture, I don't know exactly how the details are going to look. I'd rather leave writing docs till the last commit in the series. Unless you want that high altitude overview? |
e7eba0a
to
4bbda53
Compare
…987) Context: dotnet/android#7004 Context: fb94d59 fb94d59 added code to collect all `override` method descriptors in a list, so that the future marshal methods code generator can generate correct code for them. However, not all gathered descriptors contain the name of the managed type which declares the overridden methods, which is data that the native marshal method generator needs. Update when `JavaCallableWrapperGenerator.OverriddenMethodDescriptors` is populated so that the marshal method's declaring type is used.
f305db6
to
ac1433c
Compare
Source modified for my test app, won't work for anyone else
The code is tailored to my test app, so it won't work for anybody else yet
Disable all marshal methods code by default, we don't want it in `main` yet
ac1433c
to
799a72b
Compare
#endif | ||
|
||
namespace xamarin::android::internal { | ||
enum class MonoImageLoaderContext |
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 know large fractions of this code aren't even built in this PR, but why have this enum when only AssemblyLoadContext will be supported for marshal methods?
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.
It's a different kind of context, I'm not even sure I'm going to use this enum. You can ignore it for now :)
MonoError error; | ||
void *ret = mono_method_get_unmanaged_callers_only_ftnptr (method, &error); | ||
if (ret == nullptr || error.error_code != MONO_ERROR_NONE) { | ||
// TODO: make the error message friendlier somehow (class, method and assembly names) |
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.
Silly idea? store that "friendly string" as part of the struct. Alas, that'll make things bigger…
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.
Yeah, I thought about that, it might be the best idea
}, | ||
"classes.dex": { | ||
"Size": 348440 | ||
}, | ||
"lib/arm64-v8a/libmonodroid.so": { | ||
"Size": 484512 | ||
"Size": 510920 |
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.
Seems odd that libmonodroid.so
is ~26KB larger, yet lots of the new C++ code is not even compiled. Weird.
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.
A lot more code is inlined now. It's a bit surprising it generates that much of a difference, tbh.
[Xamarin.Android.Build.Tasks, monodroid] LLVM Marshal Methods Infra (#7004)
Context: https://github.com/xamarin/xamarin-android/wiki/Blueprint#java-type-registration
Introduce low-level "plumbing" for future JNI marshal method work.
A JNI marshal method is a [JNI-callable][0] function pointer provided
to [`JNIEnv::RegisterNatives()`][1]. Currently, JNI marshal methods
are provided via the interaction between `generator` and
`JNINativeWrapper.CreateDelegate()`:
* `generator` emits the "actual" JNI-callable method.
* `JNINativeWrapper.CreateDelegate()` uses System.Reflection.Emit
to *wrap* the `generator`-emitted for exception marshaling.
(Though see also 32cff438.)
JNI marshal methods are needed for all Java-to-C# transitions.
Consider the virtual `Activity.OnCreate()` method:
partial class Activity {
static Delegate? cb_onCreate_Landroid_os_Bundle_;
static Delegate GetOnCreate_Landroid_os_Bundle_Handler ()
{
if (cb_onCreate_Landroid_os_Bundle_ == null)
cb_onCreate_Landroid_os_Bundle_ = JNINativeWrapper.CreateDelegate ((_JniMarshal_PPL_V) n_OnCreate_Landroid_os_Bundle_);
return cb_onCreate_Landroid_os_Bundle_;
}
static void n_OnCreate_Landroid_os_Bundle_ (IntPtr jnienv, IntPtr native__this, IntPtr native_savedInstanceState)
{
var __this = global::Java.Lang.Object.GetObject<Android.App.Activity> (jnienv, native__this, JniHandleOwnership.DoNotTransfer)!;
var savedInstanceState = global::Java.Lang.Object.GetObject<Android.OS.Bundle> (native_savedInstanceState, JniHandleOwnership.DoNotTransfer);
__this.OnCreate (savedInstanceState);
}
// Metadata.xml XPath method reference: path="/api/package[@name='android.app']/class[@name='Activity']/method[@name='onCreate' and count(parameter)=1 and parameter[1][@type='android.os.Bundle']]"
[Register ("onCreate", "(Landroid/os/Bundle;)V", "GetOnCreate_Landroid_os_Bundle_Handler")]
protected virtual unsafe void OnCreate (Android.OS.Bundle? savedInstanceState) => …
}
`Activity.n_OnCreate_Landroid_os_Bundle_()` is the JNI marshal method,
responsible for marshaling parameters from JNI values into C# types,
forwarding the method invocation to `Activity.OnCreate()`, and (if
necessary) marshaling the return value back to JNI.
`Activity.GetOnCreate_Landroid_os_Bundle_Handler()` is part of the
type registration infrastructure, providing a `Delegate` instance to
`RegisterNativeMembers .RegisterNativeMembers()`, which is eventually
passed to `JNIEnv::RegisterNatives()`.
While this works, it's not incredibly performant: unless using one of
the optimized delegate types (32cff438 et. al),
System.Reflection.Emit is used to create a wrapper around the marshal
method, which is something we've wanted to avoid doing for years.
Thus, the idea: since we're *already* bundling a native toolchain and
using LLVM-IR to produce `libxamarin-app.so` (b21cbf94, 5271f3e1),
what if we emitted [Java Native Method Names][2] and *skipped* all
the done as part of `Runtime.register()` and
`JNIEnv.RegisterJniNatives()`?
Given:
class MyActivity : Activity {
protected override void OnCreate(Bundle? state) => …
}
During the build, `libxamarin-app.so` would contain the function:
JNIEXPORT void JNICALL
Java_crc…_MyActivity_n_1onCreate (JNIEnv *env, jobject self, jobject state);
During App runtime, the `Runtime.register()` invocation present in
[Java Callable Wrappers][3] would either be omitted or would be a
no-op, and Android/JNI would instead resolve `MyActivity.n_onCreate()`
as `Java_crc…_MyActivity_n_1onCreate()`.
Many of the specifics are still being investigated, and this feature
will be spread across various areas.
We call this effort "LLVM Marshal Methods".
First, prepare the way. Update `Xamarin.Android.Build.Tasks.dll`
and `src/monodroid` to introduce support for generating JNI marshal
methods into `libxamarin-app.so`. Most of the added code is
*disabled and hidden* behind `#if ENABLE_MARSHAL_METHODS`.
~~ TODO ~~
Other pieces, in no particular order:
* Update [`Java.Interop.Tools.JavaCallableWrappers`][4] so that
static constructors aren't needed when LLVM Marshal Methods
are used.
* Update [`generator`][5] so that *two* sets of marshal methods are
emitted: the current set e.g.
`Activity.n_OnCreate_Landroid_os_Bundle_()`, and an "overload"
set which has [`UnmanagedCallersOnlyAttribute`][6].
LLVM Marshal Methods will be able to directly call these
"unmanaged marshal methods" without the overhead of
`mono_runtime_invoke()`; see also f48b97cb.
* Finish the LLVM code generator so that LLVM Marshal Methods are
emitted into `libxamarin-app.so`.
* Update the linker so that much of the earlier marshal method
infrastructure is removed in Release apps. When
LLVM Marshal Methods are used, there is no need for
`Activity.cb_onCreate_Landroid_os_Bundle_`,
`Actvitiy.GetOnCreate_Landroid_os_Bundle_Handler()`, or the
`Activity.n_OnCreate_Landroid_os_Bundle_()` without
`[UnmanagedCallersOnly]`.
Meanwhile, we cannot remove the existing infrastructure, as the
current System.Reflection.Emit-oriented code is needed for faster app
builds, a desirable feature of Debug configuration builds.
LLVM Marshal Methods will be a Release configuration-only feature.
[0]: https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#native_method_arguments
[1]: https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#RegisterNatives
[2]: https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#resolving_native_method_names
[3]: https://github.com/xamarin/xamarin-android/wiki/Blueprint#java-type-registration
[4]: https://github.com/xamarin/java.interop/tree/main/src/Java.Interop.Tools.JavaCallableWrappers
[5]: https://github.com/xamarin/xamarin-android/wiki/Blueprint#generator
[6]: https://docs.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.unmanagedcallersonlyattribute?view=net-6.0 |
Context: dotnet#7091 We have a couple of BCL tests that seemingly started to fail after commit e1af958 landed. Unfortunately PR dotnet#7004 didn't run the BCL test stage, which may have caught this issue sooner. Update the triggers for the BCL tests to include changes to src/monodroid.
Fixes: dotnet#7085 Fixes: https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1554705 In a new Xamarin.Android project, if you go to your `OnCreate()` method and add a `throw new Exception("test")` VS Mac breaks on a `NullReferenceException` such as: [mono-rt] [ERROR] FATAL UNHANDLED EXCEPTION: System.NullReferenceException: Object reference not set to an instance of an object. at Android.Runtime.JNINativeWrapper._unhandled_exception (System.Exception e) [0x0000e] in /Users/runner/work/1/s/xamarin-android/src/Mono.Android/Android.Runtime/JNINativeWrapper.g.cs:12 at Android.Runtime.JNINativeWrapper.Wrap_JniMarshal_PPL_V (_JniMarshal_PPL_V callback, System.IntPtr jnienv, System.IntPtr klazz, System.IntPtr p0) [0x0001d] in /Users/runner/work/1/s/xamarin-android/src/Mono.Android/Android.Runtime/JNINativeWrapper.g.cs:111 at (wrapper native-to-managed) Android.Runtime.JNINativeWrapper.Wrap_JniMarshal_PPL_V(intptr,intptr,intptr) This does not happen in VS Windows, only VS Mac. After reviewing my changes in 32cff43, the System.Reflection.Emit code path does a null check for `mono_unhandled_exception_method`: bool filter = Debugger.IsAttached || !JNIEnv.PropagateExceptions; if (filter && JNIEnv.mono_unhandled_exception_method != null) { ig.BeginExceptFilterBlock (); ig.Emit (OpCodes.Call, JNIEnv.mono_unhandled_exception_method); ig.Emit (OpCodes.Ldc_I4_1); ig.BeginCatchBlock (null!); } else { ig.BeginCatchBlock (typeof (Exception)); } While the new "fast" code path, does not: static bool _unhandled_exception (Exception e) { if (Debugger.IsAttached || !JNIEnv.PropagateExceptions) { JNIEnv.mono_unhandled_exception (e); return false; } return true; } Adding `JNIEnv.mono_unhandled_exception?.Invoke(e)` appears to solve the problem when I build a local copy of `Mono.Android.dll` and test it inside VS Mac. I see my exception break properly on the line I threw the exception. `mono_unhandled_exception` being null appears to be something introduced in 2aff4e7. That commit's goal was to not lookup `mono_unhandled_exception_method` at startup, but wait until an exception is thrown. Unfortunately, `mono_unhandled_exception_method` is null at the time that the code S.R.Emitted, so we've had this behavior for a while! Since we are looking at reworking this entire system with "marshal methods" as in dotnet#7004, I think we should simply add the null check for now. We should probably investigate the sequence of events during startup & unhandled exceptions when the new system is in place.
Fixes: #7085 Fixes: https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1554705 In a new Xamarin.Android project, if you go to your `OnCreate()` method and add a `throw new Exception("test")`, Visual Studio for Mac breaks on a `NullReferenceException` such as: [mono-rt] [ERROR] FATAL UNHANDLED EXCEPTION: System.NullReferenceException: Object reference not set to an instance of an object. at Android.Runtime.JNINativeWrapper._unhandled_exception (System.Exception e) [0x0000e] in /Users/runner/work/1/s/xamarin-android/src/Mono.Android/Android.Runtime/JNINativeWrapper.g.cs:12 at Android.Runtime.JNINativeWrapper.Wrap_JniMarshal_PPL_V (_JniMarshal_PPL_V callback, System.IntPtr jnienv, System.IntPtr klazz, System.IntPtr p0) [0x0001d] in /Users/runner/work/1/s/xamarin-android/src/Mono.Android/Android.Runtime/JNINativeWrapper.g.cs:111 at (wrapper native-to-managed) Android.Runtime.JNINativeWrapper.Wrap_JniMarshal_PPL_V(intptr,intptr,intptr) This does not happen in VS Windows, only VS Mac. After reviewing my changes in 32cff43, the System.Reflection.Emit code path does a null check for `mono_unhandled_exception_method`: bool filter = Debugger.IsAttached || !JNIEnv.PropagateExceptions; if (filter && JNIEnv.mono_unhandled_exception_method != null) { ig.BeginExceptFilterBlock (); ig.Emit (OpCodes.Call, JNIEnv.mono_unhandled_exception_method); ig.Emit (OpCodes.Ldc_I4_1); ig.BeginCatchBlock (null!); } else { ig.BeginCatchBlock (typeof (Exception)); } While the new "fast" code path, does not: static bool _unhandled_exception (Exception e) { if (Debugger.IsAttached || !JNIEnv.PropagateExceptions) { JNIEnv.mono_unhandled_exception (e); return false; } return true; } Adding `JNIEnv.mono_unhandled_exception?.Invoke(e)` appears to solve the problem when I build a local copy of `Mono.Android.dll` and test it inside VS Mac. I see my exception break properly on the line I threw the exception. `mono_unhandled_exception` being null appears to be something introduced in 2aff4e7. That commit's goal was to not lookup `mono_unhandled_exception_method` at startup, but wait until an exception is thrown. Unfortunately, `mono_unhandled_exception_method` is null at the time that the code S.R.Emitted, so we've had this behavior for a while! Since we are looking at reworking this entire system with "marshal methods" as in #7004, I think we should simply add the null check for now. We should probably investigate the sequence of events during startup & unhandled exceptions when the new system is in place.
Fixes: #7085 Fixes: https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1554705 In a new Xamarin.Android project, if you go to your `OnCreate()` method and add a `throw new Exception("test")`, Visual Studio for Mac breaks on a `NullReferenceException` such as: [mono-rt] [ERROR] FATAL UNHANDLED EXCEPTION: System.NullReferenceException: Object reference not set to an instance of an object. at Android.Runtime.JNINativeWrapper._unhandled_exception (System.Exception e) [0x0000e] in /Users/runner/work/1/s/xamarin-android/src/Mono.Android/Android.Runtime/JNINativeWrapper.g.cs:12 at Android.Runtime.JNINativeWrapper.Wrap_JniMarshal_PPL_V (_JniMarshal_PPL_V callback, System.IntPtr jnienv, System.IntPtr klazz, System.IntPtr p0) [0x0001d] in /Users/runner/work/1/s/xamarin-android/src/Mono.Android/Android.Runtime/JNINativeWrapper.g.cs:111 at (wrapper native-to-managed) Android.Runtime.JNINativeWrapper.Wrap_JniMarshal_PPL_V(intptr,intptr,intptr) This does not happen in VS Windows, only VS Mac. After reviewing my changes in 32cff43, the System.Reflection.Emit code path does a null check for `mono_unhandled_exception_method`: bool filter = Debugger.IsAttached || !JNIEnv.PropagateExceptions; if (filter && JNIEnv.mono_unhandled_exception_method != null) { ig.BeginExceptFilterBlock (); ig.Emit (OpCodes.Call, JNIEnv.mono_unhandled_exception_method); ig.Emit (OpCodes.Ldc_I4_1); ig.BeginCatchBlock (null!); } else { ig.BeginCatchBlock (typeof (Exception)); } While the new "fast" code path, does not: static bool _unhandled_exception (Exception e) { if (Debugger.IsAttached || !JNIEnv.PropagateExceptions) { JNIEnv.mono_unhandled_exception (e); return false; } return true; } Adding `JNIEnv.mono_unhandled_exception?.Invoke(e)` appears to solve the problem when I build a local copy of `Mono.Android.dll` and test it inside VS Mac. I see my exception break properly on the line I threw the exception. `mono_unhandled_exception` being null appears to be something introduced in 2aff4e7. That commit's goal was to not lookup `mono_unhandled_exception_method` at startup, but wait until an exception is thrown. Unfortunately, `mono_unhandled_exception_method` is null at the time that the code S.R.Emitted, so we've had this behavior for a while! Since we are looking at reworking this entire system with "marshal methods" as in #7004, I think we should simply add the null check for now. We should probably investigate the sequence of events during startup & unhandled exceptions when the new system is in place.
This PR brings the lowest level "plumbing" for the future marshal methods implementation.
The code is disabled because it's not ready to work for anyone else except me, but I'd
like to split the code up into chunks that are easier to review, as the entire thing
is likely to be very big.
The code just needs to compile and stay out of the way.
Marshal methods are a feature designed to replace the current mechanism of "registering"
managed types with the Java runtime (via JNI). The way this is currently done, is the
JI binding generator generates 3 managed methods per single overridable Java class member,
for instance
Android.App.Activity::OnCreate
:Another generator,
JavaCallableWrappersGenerator
, is used to output Java code whenever the applicationoverrides any of the Java members, e.g. for the same
OnCreate
above:From there, the
mono.android.Runtime.register
call is a function defined in the native Xamarin.Android runtimewhich takes the method descriptions and, when called by Java, passes them to the managed land which iterates
over the
__md_methods
array, processing each string to register (using JNI) the method with Java runtime.Marshal methods will replace the registration mechanism, thus removing the
.register
call and__md_methods
from the Java side, instead generating a set of native functions in the Xamarin.Android runtime with correct symbol
names that will allow the Java runtime to call the, e.g.,
n_OnCreate
above, inside our native code. This willremove the need for entire machinery that currently emits CIL code at the managed registration stage.
It will also allow us to remove the delegate backing field and the
GetOnCreate_Landroid_os_Bundle_Handler
method (andits counterparts for other overrides) from the generated C# code. The way this is going to work is that the
handler method (
n_OnCreate_Landroid_os_Bundle_
above) will be decorated with the[UnmanagedCallersOnly]
attributewhich will allow it to be invoked directly from the generated native function (the
n_OnCreate
implementation in theexample above).
The current plan of how the above modifications are going to work at build time is that the JI C# code generator
will output two versions of the native handler code (the
n_OnCreate_Landroid_os_Bundle_
above) where one of themwill have the
[UnmanagedCallersOnly]
attribute and the other will not. The function decorated with the attribute willhave a slightly different name (since it will otherwise have the same signature), e.g.
n_OnCreate_Landroid_os_Bundle_MM
.Then, when the application is built and marshal methods are enabled, the linker will remove the static backing field which
stores the delegate, as well as the
GetOnCreate_Landroid_os_Bundle_Handler
method, together with then_OnCreate_Landroid_os_Bundle_
method. If marshal methods are disabled, however, only then_OnCreate_Landroid_os_Bundle_MM
will be removed.