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
>