Skip to content

Conversation

@jonpryor
Copy link
Contributor

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 caca88d:

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 4bd9297). 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 34129b6 started implementing this; let's
finish the effort, and require it everywhere.

A reason that commit 34129b6 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 <JdkInfo/> 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_path() to load the JVM.

Rename java_interop_jvm_load() to java_interop_jvm_load_path(),
so that we can introduce a new java_interop_jvm_load_handle()
"overload". java_interop_jvm_load_handle() should be useful within
Xamarin.Android, as the process will already have the JVM functions
available within the process' address space, avoiding the need to
"find" and hardcode a path to a jvm.dll to dlopen().

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.

#define JAVA_INTEROP_JVM_FAILED_SYMBOL (-1002)

MONO_API int java_interop_jvm_load_path (const char *path);
MONO_API int java_interop_jvm_load_handle (void *handle);
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 need to have that split into 2 functions? How would be java_interop_jvm_load_handle used on Android?

I expected to just pass NULL as path on Android and use RTLD_DEFAULT instead of calling dlopen? Like

int
java_interop_jvm_load (const char *path)
 {
	void *h = path ? dlopen (path, RTLD_LAZY) : RTLD_DEFAULT;
	if (!h) {
		return JAVA_INTEROP_JVM_FAILED_LOAD;
	}
	...
}

Or do we have the handle available in monodroid?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking the same thing, but I guess it can't be avoided if the code is to work in all kinds of environments (remember that it needs to be used also on host machines - macOS, Linux, Windows). Perhaps it could be changed, however, so that you can pass a null pointer in handle and then the code uses RTLD_DEFAULT? Or java_interop_jvm_load_path returns the loaded library's handle which you then pass on?

Copy link
Member

@radekdoulik radekdoulik Jun 26, 2018

Choose a reason for hiding this comment

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

On hosts we use the path. I am just not sure whether there is scenario, where we need to pass the handle directly. Android was mentioned, but I don't see where to get it, other than using RTLD_DEFAULT special handle, which we can do by using NULL path.

Using void *h = dlopen (path, RTLD_LAZY); is probably better than using the RTLD_DEFAULT directly (as I suggested earlier), because it will add RTLD_LAZY to the RTLD_DEFAULT handle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason (insanity?) I thought that using dlsym(NULL, "symbol") would work, so java_interop_jvm_load_handle(NULL) would be a perfectly sensible thing to do.

I'm not sure what I was thinking.

That said, now that I read the dlsym(3) man page, my crazy thinking doesn't seem that crazy, in that java_interop_jvm_load_handle(RTLD_DEFAULT) seems like a perfectly sane thing to do on Android.

Thus, having the two overloads seems...okay? Though using java_interop_jvm_load_path(NULL) also seems like it should be okay...

...except on Windows, which IIRC doesn't have a decent equivalent to dlopen(NULL, ...). However, it likewise has no equivalent to dlsym(RTLD_DEFAULT, ...), so for "proper" Windows use we'd actually need to use java_interop_jvm_load_path("path/to/jvm.dll") anyway, which removes the argument for having java_interop_jvm_load_handle().

@grendello, @radekdoulik: In short, which native API makes most sense to you?

Copy link
Member

Choose a reason for hiding this comment

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

I think java_interop_jvm_load_path(NULL) is preferable as it would still use the RTLD_LAZY, so it will be better performance wise.

Copy link
Contributor

Choose a reason for hiding this comment

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

windows can use the OSS library we build in MXE, so you can use dlopen (we submodule it in XA). As for the native API, dlsym(RTLD_DEFAULT) appears to be the one that's more expressive and descriptive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@grendello: I don’t understand your comment about dlsym(RTLD_DEFAULT).

The question is what to do about java_interop_jvm_load_path() vs. java_interop_jvm_load_handle().

Supporting dlsym(RTLD_DEFAULT) could be done by providing both, as this PR does, or by special-casing java_interop_jvm_load_path(NULL) so that when path==NULL we do handle=RTLD_DEFAULT, or by assuming that dlsym(dlopen(NULL, RTLD_LAZY), "symbol") is equivalent to dlsym(RTLD_DEFAULT, "symbol").

Copy link
Contributor

Choose a reason for hiding this comment

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

ignore my comment, I misunderstood. I thought you were asking about libdl APIs to use. From the java_interop* ones, I think the one taking char* is the right one to use. It can be used with the dlfcn-win32 library on the Windows side without any problems.

@jonpryor
Copy link
Contributor Author

Side note: reported unit test failure was introduced by 0881acc; PR #340 attempts to fix it.

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 caca88d:

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 4bd9297).  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 34129b6 started implementing this; let's
finish the effort, and require it everywhere.

*A* reason that commit 34129b6 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 `<JdkInfo/>` 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.
@jonpryor jonpryor merged commit d1cce19 into dotnet:master Jun 27, 2018
jonpryor added a commit that referenced this pull request Aug 2, 2018
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 caca88d:

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 4bd9297).  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 34129b6 started implementing this; let's
finish the effort, and require it everywhere.

*A* reason that commit 34129b6 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 `<JdkInfo/>` 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.
@github-actions github-actions bot locked and limited conversation to collaborators Apr 14, 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.

3 participants