From 0254f2058a5f8fd8839b3a126bf942f7fc5ef98d Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Mon, 25 Jun 2018 17:45:52 -0400 Subject: [PATCH] [java-interop] Require dynamic JVM loading There is a (*yugely* important) "philosophical" problem with using `Java.Runtime.Environment.dll.config` to specify the location of the `jvm.dll` to load, as originated in commit caca88d6: It cannot possibly work *reliably* on any machine *other than* the build machine. This is the case because the `/configuration/dllmap[@dll='jvm.dll']/@target` value comes from the value of `$(JI_JVM_PATH)`, which is a `make` variable generated during `make prepare` (see also 4bd9297f). As such, the only way this can work on an end-users machine is if the end user has the same JDK version installed as the machine which generated `Java.Runtime.Environment.dll.config`, which is *highly* unlikely. (Not everyone on the Xamarin.Android team has the same JDK version!) The only sane course of action, then, is to *not* use `Java.Runtime.Environment.dll.config` to specify the location of the `jvm.dll` to load. Commit 34129b6d started implementing this; let's finish the effort, and require it everywhere. *A* reason that commit 34129b6d kept `Java.Runtime.Environment.dll.config` use was so that unit tests would continue to execute, as the `TestJVM` type uses `Java.Runtime.Environment.dll`, which uses `Java.Runtime.Environment.dll.config`. To support this scenario, update the `` task so that the generated `JdkInfo.mk` file *`export`*s the `$(JI_JVM_PATH)` variable. This allows unit tests executed from e.g. `make run-all-tests` to inherit the `$JI_JVM_PATH` environment variable, allowing them to "implicitly explicitly" specify the location of the JVM to use. With unit test support in place, we can rip out all use of `jvm.dll`, requiring that we instead use e.g. `java-interop!java_interop_jvm_load()` to load the JVM. Additionally, reduce the "public API surface" within `java-interop-jvm.h` by moving the declaration of `DylibJVM` into `java-interop-jvm.c`. The `DylibJVM` type doesn't need to be public, and making it private will allow us to more easily change it. --- Makefile | 2 - gendarme-ignore.txt | 1 - .../Java.Interop.BootstrapTasks/JdkInfo.cs | 8 +-- src/Java.Interop/Java.Interop/JniRuntime.cs | 26 +------ .../Java.Interop/JreRuntime.cs | 32 +++++---- .../Java.Runtime.Environment.dll.config.in | 1 - src/java-interop/java-interop-jvm.c | 69 ++++++++++++------- src/java-interop/java-interop-jvm.h | 16 ++--- tests/TestJVM/TestJVM.cs | 4 +- tools/jnimarshalmethod-gen/App.cs | 4 +- 10 files changed, 78 insertions(+), 85 deletions(-) diff --git a/Makefile b/Makefile index 755fcffff..f8f278811 100644 --- a/Makefile +++ b/Makefile @@ -178,10 +178,8 @@ $(JRE_DLL_CONFIG): src/Java.Runtime.Environment/Java.Runtime.Environment.csproj $(MSBUILD) $(MSBUILD_FLAGS) $< run-test-jnimarshal: bin/Test$(CONFIGURATION)/Java.Interop.Export-Tests.dll bin/Test$(CONFIGURATION)/$(JAVA_INTEROP_LIB) $(JRE_DLL_CONFIG) - mv -fv "$(JRE_DLL_CONFIG)" "$(JRE_DLL_CONFIG).bak" MONO_TRACE_LISTENER=Console.Out \ $(RUNTIME) bin/$(CONFIGURATION)/jnimarshalmethod-gen.exe -v --jvm "$(JI_JVM_PATH)" -L "$(JI_MONO_LIB_PATH)mono/4.5" -L "$(JI_MONO_LIB_PATH)mono/4.5/Facades" "$<" - mv -fv "$(JRE_DLL_CONFIG).bak" "$(JRE_DLL_CONFIG)" $(call RUN_TEST,$<) # $(call GEN_CORE_OUTPUT, outdir, suffix, extra) diff --git a/gendarme-ignore.txt b/gendarme-ignore.txt index 329ee357f..f4eff4309 100644 --- a/gendarme-ignore.txt +++ b/gendarme-ignore.txt @@ -565,7 +565,6 @@ M: System.IntPtr Java.Interop.NativeMethods::java_interop_jnienv_get_direct_buff M: System.Int64 Java.Interop.NativeMethods::java_interop_jnienv_get_direct_buffer_capacity(System.IntPtr,System.IntPtr) M: Java.Interop.JniObjectReferenceType Java.Interop.NativeMethods::java_interop_jnienv_get_object_ref_type(System.IntPtr,System.IntPtr) M: System.Int32 Java.Interop.NativeMethods::java_interop_jvm_list(System.IntPtr[],System.Int32,System.Int32&) -M: System.Int32 Java.Interop.NativeMethods::java_interop_jvm_load(System.String) R: Gendarme.Rules.Naming.UseCorrectSuffixRule diff --git a/src/Java.Interop.BootstrapTasks/Java.Interop.BootstrapTasks/JdkInfo.cs b/src/Java.Interop.BootstrapTasks/Java.Interop.BootstrapTasks/JdkInfo.cs index ac915d925..fac073bba 100644 --- a/src/Java.Interop.BootstrapTasks/Java.Interop.BootstrapTasks/JdkInfo.cs +++ b/src/Java.Interop.BootstrapTasks/Java.Interop.BootstrapTasks/JdkInfo.cs @@ -147,10 +147,10 @@ void WritePropertyFile (string jarPath, string javacPath, string jdkJvmPath, IEn void WriteMakeFragmentFile (string jarPath, string javacPath, string jdkJvmPath, IEnumerable includes) { using (var o = new StreamWriter (MakeFragmentFile.ItemSpec)) { - o.WriteLine ($"JI_JAR_PATH := {jarPath}"); - o.WriteLine ($"JI_JAVAC_PATH := {javacPath}"); - o.WriteLine ($"JI_JDK_INCLUDE_PATHS := {string.Join (" ", includes)}"); - o.WriteLine ($"JI_JVM_PATH := {jdkJvmPath}"); + o.WriteLine ($"export JI_JAR_PATH := {jarPath}"); + o.WriteLine ($"export JI_JAVAC_PATH := {javacPath}"); + o.WriteLine ($"export JI_JDK_INCLUDE_PATHS := {string.Join (" ", includes)}"); + o.WriteLine ($"export JI_JVM_PATH := {jdkJvmPath}"); } } diff --git a/src/Java.Interop/Java.Interop/JniRuntime.cs b/src/Java.Interop/Java.Interop/JniRuntime.cs index fcb120457..2776f1e58 100644 --- a/src/Java.Interop/Java.Interop/JniRuntime.cs +++ b/src/Java.Interop/Java.Interop/JniRuntime.cs @@ -60,7 +60,7 @@ public partial class CreationOptions { public JniObjectReferenceManager ObjectReferenceManager {get; set;} public JniTypeManager TypeManager {get; set;} - public string JvmDllPath {get; set;} + public string JvmLibraryPath {get; set;} public CreationOptions () { @@ -77,17 +77,10 @@ interface ISetRuntime { } partial class NativeMethods { - const string JvmLibrary = "jvm.dll"; const string JavaInteropLibrary = "java-interop"; - [DllImport (JvmLibrary)] - internal static extern int JNI_GetCreatedJavaVMs ([Out] IntPtr[] handles, int bufLen, out int nVMs); - [DllImport (JavaInteropLibrary)] internal static extern int java_interop_jvm_list ([Out] IntPtr[] handles, int bufLen, out int nVMs); - - [DllImport (JavaInteropLibrary, CharSet=CharSet.Ansi)] - internal static extern int java_interop_jvm_load (string path); } public partial class JniRuntime : IDisposable @@ -96,8 +89,6 @@ public partial class JniRuntime : IDisposable const int JNI_EDETACHED = -2; const int JNI_EVERSION = -3; - static public bool JvmDllLoaded { get; private set; } - static ConcurrentDictionary Runtimes = new ConcurrentDictionary (); public static IEnumerable GetRegisteredRuntimes () @@ -115,10 +106,7 @@ public static JniRuntime GetRegisteredRuntime (IntPtr invocationPointer) internal static int GetCreatedJavaVMs (IntPtr[] handles, int bufLen, out int nVMs) { - if (JvmDllLoaded) - return NativeMethods.java_interop_jvm_list (handles, bufLen, out nVMs); - - return NativeMethods.JNI_GetCreatedJavaVMs (handles, bufLen, out nVMs); + return NativeMethods.java_interop_jvm_list (handles, bufLen, out nVMs); } public static IEnumerable GetAvailableInvocationPointers () @@ -134,16 +122,6 @@ public static IEnumerable GetAvailableInvocationPointers () return handles; } - protected static bool LoadJvmDll (string path) - { - if (JvmDllLoaded) - return true; - - JvmDllLoaded = NativeMethods.java_interop_jvm_load (path) != 0; - - return JvmDllLoaded; - } - static JniRuntime current; public static JniRuntime CurrentRuntime { get { diff --git a/src/Java.Runtime.Environment/Java.Interop/JreRuntime.cs b/src/Java.Runtime.Environment/Java.Interop/JreRuntime.cs index 09738da41..81d4ec24c 100644 --- a/src/Java.Runtime.Environment/Java.Interop/JreRuntime.cs +++ b/src/Java.Runtime.Environment/Java.Interop/JreRuntime.cs @@ -26,6 +26,8 @@ public class JreRuntimeOptions : JniRuntime.CreationOptions { internal List Options = new List (); + public string JvmLibraryPath {get; set;} + public JniVersion JniVersion {get; set;} public bool IgnoreUnrecognizedOptions {get; set;} @@ -73,21 +75,9 @@ public JreRuntime CreateJreVM () public class JreRuntime : JniRuntime { - const string JvmLibrary = "jvm.dll"; - const string JavaInteropLibrary = "java-interop"; - - [DllImport (JvmLibrary)] - static extern int JNI_CreateJavaVM (out IntPtr javavm, out IntPtr jnienv, ref JavaVMInitArgs args); - - [DllImport (JavaInteropLibrary)] - static extern int java_interop_jvm_create (out IntPtr javavm, out IntPtr jnienv, ref JavaVMInitArgs args); - static int CreateJavaVM (out IntPtr javavm, out IntPtr jnienv, ref JavaVMInitArgs args) { - if (JvmDllLoaded) - return java_interop_jvm_create (out javavm, out jnienv, ref args); - - return JNI_CreateJavaVM (out javavm, out jnienv, ref args); + return NativeMethods.java_interop_jvm_create (out javavm, out jnienv, ref args); } static unsafe JreRuntimeOptions CreateJreVM (JreRuntimeOptions builder) @@ -98,8 +88,12 @@ static unsafe JreRuntimeOptions CreateJreVM (JreRuntimeOptions builder) if (builder.InvocationPointer != IntPtr.Zero) return builder; - if (builder.JvmDllPath != null && !LoadJvmDll (builder.JvmDllPath)) - throw new Exception ($"Unable to load JVM library: {builder.JvmDllPath}"); + if (!string.IsNullOrEmpty (builder.JvmLibraryPath)) { + int r = NativeMethods.java_interop_jvm_load (builder.JvmLibraryPath); + if (r != 0) { + throw new Exception ($"Could not load JVM path `{builder.JvmLibraryPath}` ({r})!"); + } + } var args = new JavaVMInitArgs () { version = builder.JniVersion, @@ -160,5 +154,13 @@ protected override void Dispose (bool disposing) base.Dispose (disposing); } } + + partial class NativeMethods { + [DllImport (JavaInteropLib, CharSet=CharSet.Ansi)] + internal static extern int java_interop_jvm_load (string path); + + [DllImport (JavaInteropLib, CharSet=CharSet.Ansi)] + internal static extern int java_interop_jvm_create (out IntPtr javavm, out IntPtr jnienv, ref JavaVMInitArgs args); + } } diff --git a/src/Java.Runtime.Environment/Java.Runtime.Environment.dll.config.in b/src/Java.Runtime.Environment/Java.Runtime.Environment.dll.config.in index 3a7e85f8c..c1ebf627c 100644 --- a/src/Java.Runtime.Environment/Java.Runtime.Environment.dll.config.in +++ b/src/Java.Runtime.Environment/Java.Runtime.Environment.dll.config.in @@ -1,4 +1,3 @@ - @JAVA_RUNTIME_ENVIRONMENT_DLLMAP@ diff --git a/src/java-interop/java-interop-jvm.c b/src/java-interop/java-interop-jvm.c index 8d871f8b6..8fdd39db0 100644 --- a/src/java-interop/java-interop-jvm.c +++ b/src/java-interop/java-interop-jvm.c @@ -5,56 +5,73 @@ #include "java-interop-logger.h" #include "java-interop-util.h" -static struct DylibJVM *jvm = NULL; -int java_interop_jvm_load (const char *path) +typedef int (*java_interop_JNI_CreateJavaVM_fptr) (JavaVM **p_vm, void **p_env, void *vm_args); +typedef int (*java_interop_JNI_GetCreatedJavaVMs_fptr) (JavaVM **vmBuf, int bufLen, int *nVMs); + +struct DylibJVM { + void *dl_handle; + java_interop_JNI_CreateJavaVM_fptr JNI_CreateJavaVM; + java_interop_JNI_GetCreatedJavaVMs_fptr JNI_GetCreatedJavaVMs; +}; + +static struct DylibJVM *jvm; + +int +java_interop_jvm_load (const char *path) { + if (jvm != NULL) { + return JAVA_INTEROP_JVM_FAILED_ALREADY_LOADED; + } + jvm = calloc (1, sizeof (struct DylibJVM)); + if (!jvm) { + return JAVA_INTEROP_JVM_FAILED_OOM; + } jvm->dl_handle = dlopen (path, RTLD_LAZY); - - if (!jvm->dl_handle) - return 0; + if (!jvm->dl_handle) { + free (jvm); + jvm = NULL; + return JAVA_INTEROP_JVM_FAILED_NOT_LOADED; + } int symbols_missing = 0; -#define LOAD_SYMBOL(symbol) \ - jvm->symbol = dlsym (jvm->dl_handle, #symbol); \ - if (!jvm->symbol) { \ - log_error (LOG_DEFAULT, "Failed to load JVM symbol: %s", #symbol); \ - symbols_missing = 1; \ - } +#define LOAD_SYMBOL(symbol) do { \ + jvm->symbol = dlsym (jvm->dl_handle, #symbol); \ + if (!jvm->symbol) { \ + log_error (LOG_DEFAULT, "Failed to load JVM symbol: %s", #symbol); \ + symbols_missing = 1; \ + } \ + } while (0) + + LOAD_SYMBOL(JNI_CreateJavaVM); + LOAD_SYMBOL(JNI_GetCreatedJavaVMs); - LOAD_SYMBOL(JNI_CreateJavaVM) - LOAD_SYMBOL(JNI_GetCreatedJavaVMs) +#undef LOAD_SYMBOL if (symbols_missing) { - log_fatal (LOG_DEFAULT, "Failed to load some Mono symbols, aborting..."); - exit (FATAL_EXIT_JVM_MISSING_SYMBOLS); + free (jvm); + jvm = NULL; + return JAVA_INTEROP_JVM_FAILED_SYMBOL_MISSING; } - return 1; + return 0; } -static inline void -_assert_dl_handle () -{ - if (!jvm || !jvm->dl_handle) { - log_fatal (LOG_DEFAULT, "Missing JVM symbols!"); - exit (FATAL_EXIT_JVM_MISSING_SYMBOLS); - } -} +#define ji_return_val_if_fail(expr, val) do { if (!(expr)) return (val); } while (0) int java_interop_jvm_create (JavaVM **p_vm, void **p_env, void *vm_args) { - _assert_dl_handle (); + ji_return_val_if_fail (jvm != NULL, JAVA_INTEROP_JVM_FAILED_NOT_LOADED); return (*jvm->JNI_CreateJavaVM) (p_vm, p_env, vm_args); } int java_interop_jvm_list (JavaVM **vmBuf, int bufLen, int *nVMs) { - _assert_dl_handle (); + ji_return_val_if_fail (jvm != NULL, JAVA_INTEROP_JVM_FAILED_NOT_LOADED); return (*jvm->JNI_GetCreatedJavaVMs) (vmBuf, bufLen, nVMs); } diff --git a/src/java-interop/java-interop-jvm.h b/src/java-interop/java-interop-jvm.h index 14e903121..4bf3502da 100644 --- a/src/java-interop/java-interop-jvm.h +++ b/src/java-interop/java-interop-jvm.h @@ -5,18 +5,14 @@ typedef void JavaVM; -typedef int (*java_interop_JNI_CreateJavaVM_fptr) (JavaVM **p_vm, void **p_env, void *vm_args); -typedef int (*java_interop_JNI_GetCreatedJavaVMs_fptr) (JavaVM **vmBuf, int bufLen, int *nVMs); - -/* NOTE: structure members MUST NOT CHANGE ORDER. */ -struct DylibJVM { - void *dl_handle; - java_interop_JNI_CreateJavaVM_fptr JNI_CreateJavaVM; - java_interop_JNI_GetCreatedJavaVMs_fptr JNI_GetCreatedJavaVMs; -}; - JAVA_INTEROP_BEGIN_DECLS +#define JAVA_INTEROP_JVM_FAILED (-1000) +#define JAVA_INTEROP_JVM_FAILED_ALREADY_LOADED (JAVA_INTEROP_JVM_FAILED-1) +#define JAVA_INTEROP_JVM_FAILED_NOT_LOADED (JAVA_INTEROP_JVM_FAILED-2) +#define JAVA_INTEROP_JVM_FAILED_OOM (JAVA_INTEROP_JVM_FAILED-3) +#define JAVA_INTEROP_JVM_FAILED_SYMBOL_MISSING (JAVA_INTEROP_JVM_FAILED-4) + MONO_API int java_interop_jvm_load (const char *path); MONO_API int java_interop_jvm_create (JavaVM **p_vm, void **p_env, void *vm_args); MONO_API int java_interop_jvm_list (JavaVM **vmBuf, int bufLen, int *nVMs); diff --git a/tests/TestJVM/TestJVM.cs b/tests/TestJVM/TestJVM.cs index 11464fa11..2a22c518f 100644 --- a/tests/TestJVM/TestJVM.cs +++ b/tests/TestJVM/TestJVM.cs @@ -14,7 +14,9 @@ public class TestJVM : JreRuntime { static JreRuntimeOptions CreateBuilder (string[] jars) { var dir = Path.GetDirectoryName (typeof (TestJVM).Assembly.Location); - var builder = new JreRuntimeOptions (); + var builder = new JreRuntimeOptions () { + JvmLibraryPath = Environment.GetEnvironmentVariable ("JI_JVM_PATH"), + }; if (jars != null) { foreach (var jar in jars) builder.ClassPath.Add (Path.Combine (dir, jar)); diff --git a/tools/jnimarshalmethod-gen/App.cs b/tools/jnimarshalmethod-gen/App.cs index 2122b5636..82fffe398 100644 --- a/tools/jnimarshalmethod-gen/App.cs +++ b/tools/jnimarshalmethod-gen/App.cs @@ -160,7 +160,9 @@ void ProcessAssemblies (List assemblies) void CreateJavaVM (string jvmDllPath) { - var builder = new JreRuntimeOptions { JvmDllPath = jvmDllPath }; + var builder = new JreRuntimeOptions { + JvmLibraryPath = jvmDllPath, + }; try { builder.CreateJreVM ();