Stop using dlopen/dlsym to load Mono symbols #3223

grendello · 2019-06-14T09:47:16Z

Since the dawn of time Xamarin.Android (nee MonoAndroid) used dlopen/dlsym
to load Mono runtime symbols that were required for the managed applications to
work on Android. This was done due to the fact that there had been no way to
reliably load shared libraries referenced by the XA runtime in earlier versions
of Android. However, thanks to the recent switch to API level 16 as our lowest
supported level, we are now able to simply preload the shared libraries we
depend on and, thus, allow the system linker to resolve all the symbols when
loading libmonodroid.so. This, in turn, means we can now directly link
libmonosgen-2.0 on all target platforms (including all of the supported host
operating systems) and, what's also very important, include Mono headers
directly in our source. This makes sure we don't divert in definitions of
various macros and functions that we use accross our runtime and that we
continue to work with any version of Mono shipped in the Mono SDK archives.

This commit removes all traces of the DylibMono class and it also renames all
the C++ header files to have the .hh extension, to mirror the .cc source
file convention and cleary mark the C++ headers as such, as opposed to the
handful of C headers (.h) we have in our tree.

jonpryor · 2019-06-14T17:57:23Z

@garuma: Heads up on this PR: it changes libmono-android.debug.* so that instead of using dlopen() to load libmonosgen-2.0.*, it instead links against libmonosgen-2.0.*.

I can only assume that this will require changes on the Designer side.

garuma · 2019-06-14T18:36:35Z

My initial feeling would be that as long as the right libmonosgen-2.0 is linked (aka the desktop version) we should be fine. We'll see what the integration tests say

radekdoulik · 2019-06-18T08:29:15Z

The below error might be related to the fact that we don't load the libmonosgen anymore, so we might need to set the [DY]LD_LIBRARY_PATH for jnimarshalmethod-gen so that it loads libmonosgen dependency?

[2019-06-18T07:15:46.118Z]   System.DllNotFoundException: lib/host-Darwin/libmono-android.debug.dylib
[2019-06-18T07:15:46.118Z]     at (wrapper managed-to-native) Java.Interop.NativeMethods.java_interop_jvm_load(string)
[2019-06-18T07:15:46.118Z]     at Java.Interop.JreRuntime.CreateJreVM (Java.Interop.JreRuntimeOptions builder) [0x0002f] in /Users/builder/jenkins/workspace/xamarin-android-pr-pipeline-release/xamarin-android/external/Java.Interop/src/Java.Runtime.Environment/Java.Interop/JreRuntime.cs:89 
[2019-06-18T07:15:46.118Z]     at Java.Interop.JreRuntime..ctor (Java.Interop.JreRuntimeOptions builder) [0x00000] in /Users/builder/jenkins/workspace/xamarin-android-pr-pipeline-release/xamarin-android/external/Java.Interop/src/Java.Runtime.Environment/Java.Interop/JreRuntime.cs:130 
[2019-06-18T07:15:46.118Z]     at Java.Interop.JreRuntimeOptions.CreateJreVM () [0x00000] in /Users/builder/jenkins/workspace/xamarin-android-pr-pipeline-release/xamarin-android/external/Java.Interop/src/Java.Runtime.Environment/Java.Interop/JreRuntime.cs:69 
[2019-06-18T07:15:46.118Z]     at Xamarin.Android.Tools.JniMarshalMethodGenerator.App.CreateJavaVM (System.String jvmDllPath) [0x0000d] in /Users/builder/jenkins/workspace/xamarin-android-pr-pipeline-release/xamarin-android/external/Java.Interop/tools/jnimarshalmethod-gen/App.cs:202 
[2019-06-18T07:15:46.118Z]```

Context: dotnet/android#3223 Context: https://jenkins.mono-project.com/job/xamarin-android-pr-pipeline-debug/619/ Commit a30523e breaks the xamarin-android build, as it introduces new XML attributes to `class-parse` output which `Xamarin.Android.Tools.ApiXmlAdjuster` doesn't appreciate: Unhandled Exception: System.Exception: …/xamarin-android/bin/BuildDebug/api/api-10.xml.class-parse (16,7): Element 'class' has an unexpected attribute: 'source-file-name'. Expected attributes are: source-file-name at Xamarin.Android.Tools.ApiXmlAdjuster.XmlUtil.CheckExtraneousAttributes (System.String elementName, System.Xml.XmlReader reader, System.String[] expected) at Xamarin.Android.Tools.ApiXmlAdjuster.JavaApiLoaderExtensions.LoadTypeAttributes (Xamarin.Android.Tools.ApiXmlAdjuster.JavaType type, System.Xml.XmlReader reader, System.String[] otherAllowedAttributes) at Xamarin.Android.Tools.ApiXmlAdjuster.JavaApiLoaderExtensions.Load (Xamarin.Android.Tools.ApiXmlAdjuster.JavaClass kls, System.Xml.XmlReader reader) Update `Xamarin.Android.Tools.ApiXmlAdjuster` so that the new attributes introduced in a30523e are considered acceptable. Additionally, fix the error message so that it *actually* lists the "expected attributes", instead of repeating the *un*-expected attribute.

Changes: dotnet/java-interop@be58159...60e85b0 Context: dotnet/java-interop#459 Updates `generator` so that all bound Java interfaces also implement `IJavaPeerable` in addition to `IJavaObject`, for eventual future C#8 Default Interface Member support. [generator] Remove extraneous slash when creating `.projitems`. [generator] Always use `XAPeerMembers` for `XAJavaInterop1` Drop dependency on DylibMono when building for Xamarin.Android (#3223) [jnienv-gen] fix p/invoke usage for .NET framework Add `jnimarshalmethod-gen.exe -r ASSEMBLY` option. Improve support for binding package-private interfaces. Parse `EnclosingMethod`, `SourceFile` attribute blobs. Emit events for `addListener(Listener,Handler)` pattern. Fix `jnimarshalmethod-gen.exe`-related build error introduced by having bound interfaces implement `IJavaPeerable`: Instance property 'PeerReference' is not defined for type 'Android.Widget.IListAdapter' Parameter name: propertyName System.ArgumentException: Instance property 'PeerReference' is not defined for type 'Android.Widget.IListAdapter' Parameter name: propertyName at System.Linq.Expressions.Expression.Property (System.Linq.Expressions.Expression expression, System.String propertyName) at Java.Interop.JavaPeerableValueMarshaler.CreateIntermediaryExpressionFromManagedExpression (Java.Interop.Expressions.JniValueMarshalerContext context, System.Linq.Expressions.ParameterExpression sourceValue) at Java.Interop.JavaPeerableValueMarshaler.CreateReturnValueFromManagedExpression (Java.Interop.Expressions.JniValueMarshalerContext context, System.Linq.Expressions.ParameterExpression sourceValue) Added `external/Java.Interop/build-tools/jnienv-gen.csproj` to `Xamarin.Android.sln` so that it builds properly.

jonpryor · 2019-08-23T19:05:31Z

src/monodroid/CMakeLists.txt

+    link_directories("${XA_LIB_TOP_DIR}/${HOST_BUILD_NAME}")
+    include_directories("${DEFAULT_BIN_DIR}/include/${HOST_BUILD_NAME}")
+    include_directories("${DEFAULT_BIN_DIR}/include/${HOST_BUILD_NAME}/eglib")
+    include_directories("../../bin/${CONFIGURATION}/include/${HOST_BUILD_NAME}")


Should header files be extracted into $(topdir)/bin/Debug/include. Or should bin/BuildDebug be used? Though if the header files are part of the “mono bundle”, they’ll need to be in the former and not the latter...

I'd prefer to keep them where the Mono archive puts them.

jonpryor · 2019-08-23T19:06:34Z

src/monodroid/jni/debug.cc

-#include "debug.h"
-#include "util.h"
-#include "globals.h"
+#include "debug.hh"


Why rename some files to be .hh, but not others such as monodroid.h? Why rename at all?

The .hh files contain C++ classes/syntax. This is for the benefit of text editors which use the extension to determine what type of file is being edited (it is also the case with some LSP servers). It follows the convention of naming C++ files with the .cc or .cpp convention. Without renaming the files, some editors/servers would treat the files as C code thus greying out the C++ code (where #ifdef __cplusplus is used, for instance) and/or show syntax errors.

jonpryor · 2019-08-23T19:07:37Z

src/monodroid/jni/mkbundle-api.h

@@ -10,7 +10,7 @@ typedef struct BundleMonoAPI
 	void (*mono_register_bundled_assemblies) (const MonoBundledAssembly **assemblies);
 	void (*mono_register_config_for_assembly) (const char* assembly_name, const char* config_xml);
 	void (*mono_jit_set_aot_mode) (int mode);
-	void (*mono_aot_register_module) (void* aot_info);
+	void (*mono_aot_register_module) (void** aot_info);


Er...what? Did we always have the wrong declaration? Did Mono change the declaration?

It looks like we had it wrong from the beginning. The signature of the function in Mono is:

void mono_aot_register_module (gpointer *aot_info);

It was added in this commit to the C code and in this commit to jit.h using void* instead of gpointer.

jonpryor · 2019-08-23T19:12:44Z

src/monodroid/jni/monodroid-glue.cc

@@ -2268,16 +2249,16 @@ extern "C" void monodroid_dylib_mono_free (DylibMono *mono_imports)

  it should also accept libmono_path = nullptr parameter
 */
-extern "C" int monodroid_dylib_mono_init (DylibMono *mono_imports, const char *libmono_path)
+extern "C" int monodroid_dylib_mono_init (void *mono_imports, const char *libmono_path)


Who/what is invoking this function that we need to keep it around?

The only caller I found is in Java.Interop's java-interop-gc-bridge-mono.cc. It can probably be removed from that source, but there's no knowing if anyone else out there calls the function...

lambdageek · 2019-08-23T19:42:31Z

src/java-runtime/java/mono/android/MonoPackageManager.java

@@ -55,6 +55,7 @@ public static void LoadApplication (Context context, ApplicationInfo runtimePack
 				// needed in the latest Android versions but is required in at least
 				// API 16 and since there's no inherent negative effect of doing it,
 				// we can do it unconditionally.
+				System.loadLibrary("monosgen-2.0");


It would be cool if you could try to load Mono from externalDir first. That way @brendanzagaeski and I can still drop in a hacked up libmonosgen-2.0 if we want to triage some Mono regression.

We can't do it unconditionally. The code has to work for all apps and access to externalDir needs an Android permission to be granted (I think that holds true even for debug mode apps).

Since the dawn of time `Xamarin.Android` (nee `MonoAndroid`) used `dlopen/dlsym` to load Mono runtime symbols that were required for the managed applications to work on Android. This was done due to the fact that there had been no way to reliably load shared libraries referenced by the XA runtime in earlier versions of Android. However, thanks to the recent switch to API level 16 as our lowest supported level, we are now able to simply preload the shared libraries we depend on and, thus, allow the system linker to resolve all the symbols when loading `libmonodroid.so`. This, in turn, means we can now directly link `libmonosgen-2.0` on all target platforms (including all of the supported host operating systems) and, what's also very important, include Mono headers directly in our source. This makes sure we don't divert in definitions of various macros and functions that we use accross our runtime and that we continue to work with any version of Mono shipped in the Mono SDK archives. This commit removes all traces of the `DylibMono` class and it also renames all the C++ header files to have the `.hh` extension, to mirror the `.cc` source file convention and cleary mark the C++ headers as such, as opposed to the handful of C headers (`.h`) we have in our tree.

For a very long time Xamarin.Android (nee `Mono for Android`) used **dlopen**(3)/**dlsym**(3) to load Mono runtime symbols that were required for the managed applications to work on Android. The more "conventional" solution would have been to change `libmonodroid.so` to have a link-time dependency on `libmonosgen-2.0.so`, instead of `dlopen(".../libmonosgen-2.0.so")` and corresponding `dlsym()` usage, but Android complicates matters by not reliably [loading dependencies from the same path][0]. However, thanks to the recent switch to API level 16 as our lowest supported API level, we are now able to simply preload the shared libraries we depend on and, thus, allow the system linker to resolve all the symbols when loading `libmonodroid.so` ("convention"). This, in turn, means we can now directly link against `libmonosgen-2.0` on all target platforms (including all of the supported host operating systems) and, what's also very important, include Mono headers directly in our source. This ensures we don't differ in definitions of various macros and functions that we use across our runtime and that we continue to work with any version of Mono shipped in the Mono SDK archives. Remove all traces of the `DylibMono` class and also rename all the C++ header files to have the `.hh` extension, to mirror the `.cc` source file convention and clearly mark the C++ headers as such, as opposed to the handful of C headers (`.h`) we have in our tree. [0]: https://stackoverflow.com/questions/11058898/loading-shared-libs-that-depend-on-other-shared-libs

grendello added the do-not-merge PR should not be merged. label Jun 14, 2019

grendello requested review from radekdoulik and jonpryor June 14, 2019 09:53

grendello force-pushed the drop-dylibmono branch from 1c52d19 to 0b8fedf Compare June 14, 2019 17:18

jonpryor requested a review from garuma June 14, 2019 17:53

grendello force-pushed the drop-dylibmono branch 5 times, most recently from 75f1ba4 to c97af07 Compare June 18, 2019 06:55

grendello force-pushed the drop-dylibmono branch 6 times, most recently from 2bfd22c to 29517aa Compare June 25, 2019 09:21

grendello added the full-mono-integration-build For PRs; run a full build (~6-10h for mono bumps), not the faster PR subset (~2h for mono bumps) label Jun 25, 2019

grendello force-pushed the drop-dylibmono branch from 29517aa to a2714fa Compare June 26, 2019 20:23

grendello force-pushed the drop-dylibmono branch 2 times, most recently from aea4dd5 to dd208ea Compare July 19, 2019 10:05

grendello marked this pull request as ready for review July 19, 2019 14:47

grendello requested a review from jonathanpeppers as a code owner July 19, 2019 14:47

grendello removed the do-not-merge PR should not be merged. label Jul 19, 2019

grendello force-pushed the drop-dylibmono branch from dd208ea to aeae253 Compare July 24, 2019 14:40

grendello force-pushed the drop-dylibmono branch 3 times, most recently from d00ccaa to a03df04 Compare August 14, 2019 06:07

jonpryor mentioned this pull request Aug 16, 2019

[ApiXmlAdjuster] Add additional expected attributes dotnet/java-interop#476

Merged

grendello force-pushed the drop-dylibmono branch 3 times, most recently from b43b28c to d580c54 Compare August 19, 2019 15:06

grendello force-pushed the drop-dylibmono branch 2 times, most recently from e2f48e8 to 177a264 Compare August 22, 2019 15:56

jonpryor reviewed Aug 23, 2019

View reviewed changes

lambdageek reviewed Aug 23, 2019

View reviewed changes

grendello force-pushed the drop-dylibmono branch from 177a264 to 9cd7d8d Compare August 26, 2019 09:20

grendello added the do-not-merge PR should not be merged. label Aug 27, 2019

grendello force-pushed the drop-dylibmono branch from 9cd7d8d to b97a957 Compare August 28, 2019 21:02

grendello requested a review from dellis1972 as a code owner August 28, 2019 21:02

grendello force-pushed the drop-dylibmono branch 6 times, most recently from 5be7be2 to 6567425 Compare August 30, 2019 07:05

grendello removed the do-not-merge PR should not be merged. label Aug 30, 2019

grendello force-pushed the drop-dylibmono branch from 6567425 to dcc1207 Compare August 30, 2019 15:19

jonpryor merged commit 6be4bdb into dotnet:master Aug 30, 2019

grendello deleted the drop-dylibmono branch August 30, 2019 20:38

github-actions bot locked and limited conversation to collaborators Jan 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop using dlopen/dlsym to load Mono symbols #3223

Stop using dlopen/dlsym to load Mono symbols #3223

grendello commented Jun 14, 2019 •

edited

Loading

jonpryor commented Jun 14, 2019

garuma commented Jun 14, 2019

radekdoulik commented Jun 18, 2019

jonpryor Aug 23, 2019

grendello Aug 26, 2019

jonpryor Aug 23, 2019

grendello Aug 26, 2019

jonpryor Aug 23, 2019

grendello Aug 26, 2019

jonpryor Aug 23, 2019

grendello Aug 26, 2019

lambdageek Aug 23, 2019

grendello Aug 26, 2019

Stop using dlopen/dlsym to load Mono symbols #3223

Stop using dlopen/dlsym to load Mono symbols #3223

Conversation

grendello commented Jun 14, 2019 • edited Loading

jonpryor commented Jun 14, 2019

garuma commented Jun 14, 2019

radekdoulik commented Jun 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grendello commented Jun 14, 2019 •

edited

Loading