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

[monodroid] Build the Xamarin.Android runtime for net6 #5665

Merged
merged 3 commits into from
Mar 3, 2021

Conversation

grendello
Copy link
Contributor

@grendello grendello commented Feb 26, 2021

This commit makes changes to build the native Xamarin.Android runtime
against Mono VM shipped from the dotnet/runtime repository in
nuget packages which also contain header files necessary to embed
Mono in Xamarin.Android.

Xamarin.Android runtime is now built separately for the "legacy"
target, using Mono from the SDK archives, and for net6. This
change required introduction of new target directories in
$XA_INSTALL_ROOT/lib:

  • arm64-v8a-net6 armeabi-v7a-net6 x86_64-net6 x86-net6

These directories now contain only the shared libraries together
comprising the Xamarin.Android runtime:

  • libmono-android..so libxamarin-debug-app-helper.so
  • libxamarin-app.so

Mono runtime libraries are not copied from their respective nuget
packages. Compared to the "legacy" builds, we are missing the
libMonoPosixHelper.so library. The helper DSO will be eventually
placed in a Mono.Posix nuget, once we are able to separate
Mono.Posix files from the Mono sources and place them in a
separate repository.

xaprepare is modified to generate MSBuild and cmake fragments
responsible for building net6 runtime binaries.

Native runtime now doesn't export any p/invoke entry points,
instead it uses the new PINVOKE_OVERRIDE mechanism to handle the
p/invoke dispatch internally. This allows us to not only hide the
public symbols, but also remove the libxa-internal-api.so
library completely.

@grendello
Copy link
Contributor Author

I won't be working on more TODO items from the list in the OP - the PR is already big enough. If the PR is green, let's please commit it.

4. nunit/nunitlite (https://github.com/nunit/nunitlite/)
4. google/bionic (https://android.googlesource.com/platform/bionic/)
5. nunit/nunitlite (https://github.com/nunit/nunitlite/)
6. tessil/robin-map (https://github.com/Tessil/robin-map)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems odd: the in-repro ThirdPartyNotices.txt only needs to contain stuff within this repo, e.g. files in src-ThirdParty. A git submodule doesn't need to be listed in this file; instead, it needs to be listed in the generated bin/$(Configuration)/lib/xamarin.android/ThirdPartyNotices.txt file.

@@ -0,0 +1,84 @@
/*
Copy link
Member

@jonpryor jonpryor Mar 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file and other files which were copied from Bionic should be in src-ThirdParty/ndk, or some other "meaningful" src-ThirdParty subdirectory.

@@ -39,6 +39,7 @@ JNIEXPORT void JNICALL Java_mono_android_Runtime_register
JNIEXPORT void JNICALL Java_mono_android_Runtime_notifyTimeZoneChanged
(JNIEnv *, jclass);

#if !defined (ANDROID)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this was a manual addition? This is usually a generated file, so if this is a manual addition that's…problematic? It might be better to wrap the #include "mono_android_Runtime.h" instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

…though it's only the Desktop-related members here. Ugh.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be a good opportunity to introduce the long-considered DesignerRuntime class, have the native methods declared there, and update the current Designer-centric Runtime methods to instead invoke the DesignerRuntime methods. This means we'd have a new mono_android_DesignerRuntime.h file, which can be appropriately "guarded".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely not material for this PR :)

true
>;

static constexpr pinvoke_library_map::size_type LIBRARY_MAP_INITIAL_BUCKET_COUNT = 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want an initial bucket size of 1? :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it avoids an expensive call to guard locks around a static variable inside one of the robin-map functions

>;

static constexpr pinvoke_library_map::size_type LIBRARY_MAP_INITIAL_BUCKET_COUNT = 1;
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convention (if not always followed): #endif/#elif/etc. should have a comment to mention what symbols they're related to:

#endif  /* defined (NET6) */

#include "config.include"
#endif
#include "machine.config.include"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need machine.config.xml?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it contains a bunch of settings regarding pub tokens, remoting configuration etc - not sure we need it, but it might be needed for some things around the runtime

return monodroid_dlopen (name, flags, err);
}

size_t len = strlen (name) + DSO_EXTENSION_SIZE; // includes the trailing \0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we check/how do we prevent. strlen(name) from being std::numeric_limits<size_t>::max, causing the addition to be an overflow?

Should this use one of our "checked addition" macros?

const char* property_keys[] { "PINVOKE_OVERRIDE" };
const char* property_values[] { ptr_str };

log_warn (LOG_DEFAULT, "Initializing .NET6 Mono VM (override callback at %p, passed as %s", pinvoke_override_cb, ptr_str);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want this printed in Release builds? Or should this be log_debug()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually log_debug, I'd rather keep it log_warn for now - until we are ready for final release

_monodroid_timezone_get_default_id ()
{
JNIEnv *env = osBridge.ensure_jnienv ();
jmethodID getDefault = env->GetStaticMethodID (monodroidRuntime.get_java_class_TimeZone (), "getDefault", "()Ljava/util/TimeZone;");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to look these jmethodIDs up every time, instead of caching them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we did that to speed up startup. Caching it at the later stage would probably require locks. We can implement that, but not in this PR.

jobject d = env->CallStaticObjectMethod (monodroidRuntime.get_java_class_TimeZone (), getDefault);
jstring id = reinterpret_cast<jstring> (env->CallObjectMethod (d, getID));
const char *mutf8 = env->GetStringUTFChars (id, nullptr);
char *def_id = strdup (mutf8);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should null-check mutf8?


#define PINVOKE_SYMBOL(_sym_) { #_sym_, reinterpret_cast<void*>(&_sym_) }

MonodroidRuntime::pinvoke_api_map MonodroidRuntime::xa_pinvoke_map = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this table need to be sorted? If so, that should be commented.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it doesn't need to be sorted. robin-map is an equivalent to std::unordered_map, I sorted it just for convenience.

@grendello
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@grendello
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@grendello grendello changed the title [monodroid] NET6 native runtime [monodroid] Build the Xamarin.Android runtime for net6 Mar 2, 2021
This commit makes changes to build the native Xamarin.Android runtime
against Mono VM shipped from the `dotnet/runtime` repository in nuget
packages which also contain header files necessary to embed Mono in
Xamarin.Android.

Xamarin.Android runtime is now built separately for the "legacy" target,
using Mono from the SDK archives, and for net6.  This change required
introduction of new target directories in `$XA_INSTALL_ROOT/lib`:

  * arm64-v8a-net6
  * armeabi-v7a-net6
  * x86_64-net6
  * x86-net6

These directories now contain only the shared libraries together
comprising the Xamarin.Android runtime:

  * libmono-android.*.so
  * libxamarin-debug-app-helper*.so
  * libxamarin-app.so

Mono runtime libraries are not copied from their respective nuget
packages.  Compared to the "legacy" builds, we are missing the
`libMonoPosixHelper.so` library.  The helper DSO will be eventually
placed in a `Mono.Posix` nuget, once we are able to separate
`Mono.Posix` files from the Mono sources and place them in a separate
repository.

`xaprepare` is modified to generate MSBuild and cmake fragments
responsible for building net6 runtime binaries.

Native runtime now doesn't export any p/invoke entry points, instead it
uses the new `PINVOKE_OVERRIDE` mechanism to handle the p/invoke
dispatch internally.  This allows us to not only hide the public
symbols, but also remove the `libxa-internal-api.so` library completely.
@jonpryor
Copy link
Member

jonpryor commented Mar 3, 2021

WIP commit message;

Context: https://github.com/dotnet/runtime/issues/48416
Context: https://github.com/dotnet/runtime/pull/44505
Context: https://github.com/xamarin/xamarin-android/issues/5537

Update the `src/monodroid` build system to build the native
Xamarin.Android runtime libraries (e.g. `libmono-android.debug.so`)
against the MonoVM shipped from the `dotnet/runtime` repository in
the `Microsoft.NETCore.App.Runtime.*` NuGet packages.  These NuGet
packages also contain the header files needed to access and use
MonoVM's embedding API.

We thus ~double the number of libraries built by `src/monodroid`,
with one set of builds against the "legacy" Mono, using Mono from the
SDK archives, and another set of builds against .NET 6 MonoVM.

This change requires the introduction of new target directories in
`$(XAInstallPrefix)xbuild/Xamarin/Android/lib` (e.g.
`bin/Debug/lib/xamarin.android/xbuild/Xamarin/Android/lib`):

  * `arm64-v8a-net6`
  * `armeabi-v7a-net6`
  * `x86_64-net6`
  * `x86-net6`

These directories now contain only the shared libraries together
comprising the Xamarin.Android native runtime libraries:

  * `libmono-android.*.so`
  * `libxamarin-debug-app-helper*.so`
  * `libxamarin-app.so`

Mono runtime libraries are *not* copied from their respective NuGet
packages; the .NET 6 NuGet packages are required to obtain them.
Compared to the "legacy" builds, we are missing the 
libMonoPosixHelper.so` library.  This helper lib will eventually be
placed in a `Mono.Posix` NuGet, once we are able to separate
`Mono.Posix` files from the Mono sources and place them in a
[separate repository][0].

`xaprepare` is modified to generate MSBuild and cmake fragments
responsible for building net6 runtime binaries.

The `libmono-android.*.so` native libraries no longer need to export
any entry points for P/Invoke purposes.  Instead, the new
`PINVOKE_OVERRIDE` [runtime property][1] is used to hook into the
P/Invoke lookup process.  This allows us to not only hide the public
symbols, but also to remove the `libxa-internal-api.so`
library (d583b7c2) completely.

[0]: https://github.com/mono/mono.posix
[1]: https://docs.microsoft.com/en-us/dotnet/core/tutorials/netcore-hosting#step-3---prepare-runtime-properties

@jonpryor jonpryor merged commit 0cd890b into dotnet:master Mar 3, 2021
@grendello grendello deleted the net6-native branch March 3, 2021 07:45
@github-actions github-actions bot locked and limited conversation to collaborators Jan 25, 2024
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.

2 participants