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

Fri Apr 28 22:33:26 UTC 2017

On 29/04/2017 1:18 AM, coleen.phillimore at oracle.com wrote:
> On 4/27/17 1:07 PM, Vladimir Kozlov wrote:
>> I think we just need to remove -XXaltjvm.
>
> +1
>
> I used to use this all the time.   Now, never.

I'll spell this out again just to be very sure there is no confusion - 
including possibly mine :)

The code under discussion does not come into play when passing -XXaltjvm 
to the java launcher. So that use of -XXaltjvm is completely independent 
and out of scope for this fix, so if it needs removing that is a 
separate issue.

The code under discussion previously was only activated when using the 
gamma-launcher, and that was later changed to activate if the (badly 
named) -Dsun.java.launcher.is_altjvm=true is set, which is then used by 
the gtestlauncher.

However the gtestlauncher always sets JAVA_HOME so the code looking for 
jre/lib<arch>/... can fail and not cause any problems. And that code was 
not updated when we got rid of the <arch> subdirectory, nor when we got 
rid of the <jre> subdirectory; and on OSX never worked in the first 
place as it never had an <arch> directory!

So I think we can simply delete all the code code that tries to look for 
jre/lib/... and only use the JAVA_HOME logic.

Thanks,
David

> Coleen
>
>>
>> Originally we use it for testing locally built JVM (when we had
>> ability to build Hotspot JVM separate). And for binary search of
>> changes which may cause a failure by selecting different JVM for the
>> same JDK. OR even different JDK releases (Hotspot Express).
>>
>> But since JDK 8 it become almost impossible since JVM code is tightly
>> connected to JDK code.
>>
>> Regards,
>> Vladimir
>>
>> On 4/27/17 5:21 AM, David Holmes wrote:
>>> Hi Thomas,
>>>
>>> Thanks for the detailed investigation. I am very confused by all this.
>>> I'm wondering whether we should just kill off -XXaltjvm altogether?
>>> Otherwise I'm not at all sure what is the "right" thing to do as I'm not
>>> clear what it is intended to do.
>>>
>>> David
>>>
>>> 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.
>>>>
>>>> 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().
>>>>
>>>> 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?)
>>>>
>>>> --
>>>>
>>>> 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
>>>>
>>>>
>