RFR(xxs, jdk10): 8171508: os::jvm_path -XXaltjvm processing error after 8066474

Fri Apr 28 01:57:08 UTC 2017

Hi Thomas,

Did a bit of archaeology on this ...

On 27/04/2017 7:25 PM, Thomas Stüfe wrote:
> Hi David,
>
> You are right, this is more complicated.
>
> --
>
> Here is what I think was originally intended:
>
> Scenario a): I start java from jdk A and give it, via -XXaltjvm, a path
> pointing to a libjvm.so in jdk B. In this case, os::jvm_path() should
> detect that jdk B is a full JDK, and return the path to jdk B. So, all
> JDK libraries should be loaded from jdk B.
>
> Scenario b) I start java from jdk A and give it, via -XXaltjvm, a path
> pointing to a standalone libjvm.so. In this case, os::jvm_path() should
> see that there is no full valid JDK around the alternate libjvm.so, and
> uses a JDK from JAVA_HOME. Failing that (no JAVA_HOME set), it should
> use the path from the loaded alternative libjvm.so.
>
> --
>
> However, there are errors:
>
> Error 1: -XXaltjvm is currently not handled by os::jvm_path(): Arguments
> <http://ld8443:8080/source/s?defs=Arguments&project=openjdk-jdk10-hs>::sun_java_launcher_is_altjvm
> <http://ld8443:8080/source/s?defs=sun_java_launcher_is_altjvm&project=openjdk-jdk10-hs>()
> returns always false and we skip altjvm handling altogether. This is
> because -Dsun.java.launcher.is_altjvm=true is missing. The Launcher
> should set this property if XXaltjvm is set.

No. The property, despite its name, is nothing to do with the java 
launcher. It was added by:

https://bugs.openjdk.java.net/browse/JDK-8027113

to "decouple the '-XXaltjvm=<path>' option from the gamma launcher". We 
set the property on the command-line via the build system.

> Side note: the gtestLauncher sets -Dsun.java.launcher.is_altjvm=true. So
> starting gtests is currently the only way to hit the altjvm path in
> os::jvm_path().

Right - and previously this code was only ever hit if 
Arguments::created_by_gamma_launcher() was true. So this os::jvm_path 
logic really has nothing at all to do with the -XXaltjvm option as 
processed by the java launcher - it was only for use by our custom 
launchers.

> Side note 2: Your CDS scenario just works because the path you add to
> -XXaltjvm is inside a full JDK (actually the primary JDK). This needs no
> altjvm handling at all in os::jvm_path() - it just returns the path to
> the primary jdk. (I wonder why you even need to specify XXaltjvm? Is
> this to distinguish between server and client VM?)

I was simply trying to provoke an error by specifying XXaltjvm and have 
os::jvm_path process it incorrectly and consequently have CDS dumping fail.

This truly is an awful mess. I suspect we have not actually relied on 
any of that jvm_path logic for a long time given it will never match 
with what it considers to be a regular JDK. I think we are simply using 
the JAVA_HOME logic (by gtest) and so can delete the rest of it completely.

Thanks,
David

> --
>
> So I do some trials with -XXaltjvm=<path>  together with explicitely
> setting -Dsun.java.launcher.is_altjvm=true.
>
> Scenario a): jdk A = ./images, jdk B = images-2
>
> without JAVA_HOME set:  ./images/jdk/bin/java
> -Dsun.java.launcher.is_altjvm=true -XXaltjvm=./images-2/jdk/lib/server
>
> We load libjvm.so and libjava.so from images-2. Looks correct? Yes, but
> I stepped thru with a debugger, what actually happens is that the
> full-JDK-recognition at os_linux.cpp:2348 fails. Then it checks for
> JAVA_HOME, which it is not set. Then it just gives up and falls back to
> the path to the alternate libjvm.so at os_linux.cpp:2364.Which happens
> to be the correct path anyway for jdk B,
>
> Now lets set JAVA_HOME to some completely unrelated JDK. os::jvm_path()
> should ignore the value and still return path to jdk b, yes? But no:
>
> - .../output $ export
> JAVA_HOME=/sapmnt/depot/tools/gen/linuxx86_64/licenseware/jse/1.5.0
> - .../output $ ./images/jdk/bin/java -Dsun.java.launcher.is_altjvm=true
> -XXaltjvm=./images-2/images/jdk/lib/server
> Error occurred during initialization of VM
> Unable to load native library:
> /net/sapmnt.depot/tools/gen/linuxx86_64/licenseware/jse/1.5.0/jre/lib/libjava.so:
> cannot open shared object file: No such file or directory
>
> This is Error 2: os::jvm_path() does not recognize "images-2" to be a
> valid full JDK, so it falls back to JAVA_HOME, which in this case points
> to some unrelated older JDK. Here, in this case,  it fails to load the
> libjava.so. But it should never have attempted to load it.
>
> This is the bug my patch attempts to fix, admittedly you are right,
> there may be more to it (the "jre" for example).
>
> Scenario b) just works, if JAVA_HOME is set correctly. This is what the
> gtestLauncher does: It sets -Dsun.java.launcher.is_altjvm=true and sets
> JAVA_HOME to the value of the -jdk:<path> argument (see gtestMain.cpp).
>
> --
>
> So the first error is IMHO that when -XXaltjvm is passed it should set
> -Dsun.java.launcher.is_altjvm=true but it does not.
>
> The second error is that the half-baked full-jdk-detection in
> os::jvm_path() does not work. You are right, there are more errors than
> the 5-slashes-path-traversal. This makes Scenario a) misbehave if
> JAVA_HOME happens to be set to another JDK. You will either not load, as
> in my case, or load JDK libraries from another JDK.
>
> I honestly do not like this coding at all and would love to see this
> made simpler. This is very difficult to understand.
>
> Kind Regards, Thomas
>
>
> On Thu, Apr 27, 2017 at 1:31 AM, David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
>     Hi Thomas,
>
>     Backing up a few steps ...
>
>     On 26/04/2017 5:10 PM, Thomas Stüfe wrote:
>
>         Hi all,
>
>         may I please have a review for this tiny fix. 8066474 removed
>         the <arch>
>         directory from the images and since then -XXaltjvm was slightly
>         broken.
>         When handling XXaltjvm, os::jvm_path() examines the path of the
>         libjvm.so
>         to check if it is part of what it considers a standard JDK by
>         traversing a
>         number of slashes up the path and looking for "/jre/lib". That
>         number of
>         slashes was off since 8066474.
>
>
>     Hold on a moment! We don't have a jre subdirectory any more either!
>     The image for a jdk directly contains lib/server/libjvm.so (for
>     example).
>
>     Also jvm_path is used for dumping the shared archive using the
>     default location. If I do:
>
>     > ./images/jdk/bin/java -XXaltjvm=images/jdk/lib/server/ -Xshare:dump
>
>     it works perfectly correctly:
>
>      > find . -name classes.jsa
>     ./images/jdk/lib/server/classes.jsa
>
>     So where's the bug ??
>
>     David
>     -----
>
>
>
>         webrev:
>         http://cr.openjdk.java.net/~stuefe/webrevs/8171508-os_jvm_path_xxaltjvm_processing_error_after_8066474/jdk10-webrev.00/webrev/index.html
>         <http://cr.openjdk.java.net/~stuefe/webrevs/8171508-os_jvm_path_xxaltjvm_processing_error_after_8066474/jdk10-webrev.00/webrev/index.html>
>
>         Bug: https://bugs.openjdk.java.net/browse/JDK-8171508
>         <https://bugs.openjdk.java.net/browse/JDK-8171508>
>
>         Note that this only affects cases where the alternate libjvm.so
>         is part of
>         a full jdk, so it does not affect the gtestLauncher.
>
>         Thanks & Regards, Thomas
>
>