Review Request: JDK-8186145: tools/launcher/modules/validate/ValidateModulesTest.java fails when launched with -XX:+EnableJVMCI

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Review Request: JDK-8186145: tools/launcher/modules/validate/ValidateModulesTest.java fails when launched with -XX:+EnableJVMCI

Mandy Chung
java --validate-modules requires only java.base to do the validation and
hence the current implementation creates a minimum boot layer with
java.base only.  It fails when running with -XX:+EnableJVMCI when VM
attempts to load JVMCI class which is not in the boot layer.  This patch
changes the minimal boot layer to resolve all system modules that should
get --validate-modules to work with some VM options that add some module
at runtime.

Webrev:
http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8186145/webrev.00/

Mandy
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: JDK-8186145: tools/launcher/modules/validate/ValidateModulesTest.java fails when launched with -XX:+EnableJVMCI

Alan Bateman
On 24/08/2017 19:05, mandy chung wrote:

> java --validate-modules requires only java.base to do the validation
> and hence the current implementation creates a minimum boot layer with
> java.base only.  It fails when running with -XX:+EnableJVMCI when VM
> attempts to load JVMCI class which is not in the boot layer.  This
> patch changes the minimal boot layer to resolve all system modules
> that should get --validate-modules to work with some VM options that
> add some module at runtime.
>
> Webrev:
> http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8186145/webrev.00/
Looks okay except L218 where the comment needs to be updated to say that
it starts without an application module path.

I also wonder if a test is possible, it could use "@requires vm.jvmci"
so that it only runs when that capability is present.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: JDK-8186145: tools/launcher/modules/validate/ValidateModulesTest.java fails when launched with -XX:+EnableJVMCI

Mandy Chung


On 8/24/17 11:21 AM, Alan Bateman wrote:

> On 24/08/2017 19:05, mandy chung wrote:
>> java --validate-modules requires only java.base to do the validation
>> and hence the current implementation creates a minimum boot layer
>> with java.base only.  It fails when running with -XX:+EnableJVMCI
>> when VM attempts to load JVMCI class which is not in the boot layer.
>> This patch changes the minimal boot layer to resolve all system
>> modules that should get --validate-modules to work with some VM
>> options that add some module at runtime.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8186145/webrev.00/
> Looks okay except L218 where the comment needs to be updated to say
> that it starts without an application module path.
>

Good catch.

> I also wonder if a test is possible, it could use "@requires vm.jvmci"
> so that it only runs when that capability is present.

That's a good idea.  I added a new test in hotspot/test/compiler/jvmci. 
Updated webrev:
http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8186145/webrev.01/index.html

I considered adding this in the jdk/test directory.  @requires vm.jvmci
tightly couples with VM and also VM testlibrary and I am hesitated
extending jdk/test/TEST.ROOT to set up for this test.  We can revisit
this if there is a strong need to move this new test in the jdk/test
directory.

Mandy
[1]
http://hg.openjdk.java.net/jdk10/jdk10/file/90cdfe56f154/test/jtreg-ext/requires/VMProps.java


Reply | Threaded
Open this post in threaded view
|

Re: Review Request: JDK-8186145: tools/launcher/modules/validate/ValidateModulesTest.java fails when launched with -XX:+EnableJVMCI

Alan Bateman
On 25/08/2017 00:36, mandy chung wrote:

> :
>
> That's a good idea.  I added a new test in
> hotspot/test/compiler/jvmci.  Updated webrev:
> http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8186145/webrev.01/index.html 
>
>
> I considered adding this in the jdk/test directory.  @requires
> vm.jvmci tightly couples with VM and also VM testlibrary and I am
> hesitated extending jdk/test/TEST.ROOT to set up for this test. We can
> revisit this if there is a strong need to move this new test in the
> jdk/test directory.
>
The updated webrev looks good.

-Alan