Re: RFR(L) 8136930: Simplify use of module-system options by custom launchers

[harold seigel]

Hi Lois,

Thanks for the review.  Please see comments in-line.
Harold


On 8/1/2016 8:40 PM, Lois Foltan wrote:

On 7/17/2016 7:05 PM, harold seigel wrote:
Hi,

Please review these Hotspot VM only changes to process the seven 
module-specific options that have been renamed to have gnu-like 
names.  JDK changes for this bug will be reviewed separately.

Descriptions of these options are here 
<http://openjdk.java.net/jeps/293>. For these six options, 
--module-path, --upgrade-module-path, --add-modules, --limit-modules, 
--add-reads, and --add-exports, the JVM just sets a system property.  
For the --patch-module option, the JVM sets a system property and 
then processes the option in the same way as when it was named -Xpatch.

Additionally, the JVM now checks properties specified on the command 
line.  If a property matches one of the properties used by one of the 
above options then the JVM ignores the property. This forces users to 
use the explicit option when wanting to do things like add a module 
or a package export.

The RFR contains two new tests.  Also, many existing tests were 
changed to use the new option names.

JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8136930

Webrev: http://cr.openjdk.java.net/~hseigel/bug_8136930.hs/

Hi Harold,

Overall looks good.  A couple of comments:

src/share/vm/prims/jvmtiEnv.cpp
- line #3428 - The if statement is incorrect.  There are internal 
properties, like jdk.boot.class.path.append, whose value if non-null 
should be returned.
This code will be reworked in the next version of these changes because 
of multiple issues.

src/share/vm/runtime/arguments.cpp
- Arguments::append_to_addmods_property was added before the VM 
starting to process --add-modules.  So with this fix, it seems like it 
could be simply changed to:

    bool Arguments::append_to_addmods_property(const char* module_name) {
      PropertyList_unique_add(&_system_properties,
    Arguments::get_property("jdk.module.addmods"),
                                                   module_name,
    AppendProperty, UnwriteableProperty, InternalProperty);
    }

Please consider making this change since currently it contains a lot 
of duplicated code that is now unnecessary.
The one difference is that append_to_addmods_property() returns a status 
but PropertyList_unique_add() does not.  I'll look into this a bit further.

- line #3171, should the comment be "--add-modules=java.sql" instead 
of "--add-modules java.sql"?
yes.

The changes suggested by you, Coleen, and Dan will be in the next 
version of this webrev.

Thanks, Harold

Thanks,
Lois














The changes were tested with the JCK lang and VM tests, the JTreg 
hotspot tests, and the RBT hotspot nightlies.

Thanks, Harold