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

[Coleen Phillimore]

On 7/21/16 1:39 PM, harold seigel wrote:
> Hi Coleen,
>
> Thank you for reviewing my changes!
>
> Please see comments inline.
>
>
> On 7/20/2016 5:47 PM, Coleen Phillimore wrote:
>>
>> http://cr.openjdk.java.net/~hseigel/bug_8136930.hs/src/share/vm/runtime/arguments.cpp.udiff.html 
>>
>>
>> To check if is_internal_module_property, the numbered ones go to 
>> is_matching_numbered_property, but do you also want to disallow:
>>
>> jdk.module.addreads.foo   or jdk.module.addreads (with no number?)
> We don't ignore properties like these because they do not have special 
> meaning to the jdk.  I didn't want to ignore properties just because 
> they are very similar to ones that do get ignored.

Don't we own the jdk.module namespace and from these patterns it's clear 
that it's an error, although we won't flag it, and somebody might get 
results that are unexpected.

I think you should flag these.

>>
>> Why aren't addmods and limitmods allowed be specified more than once 
>> on the command line and handled like the other ones?
> Please see Alan's response.  Should I open an RFE for allowing 
> multiple --add-modules options?

Yes, and you can discuss it there.

>>
>> + // options. For example: use "--add-modules java.sql", not 
>> "-Djdk.module.addmods=java.sql"
>>
>> Does this comment need an equal sign?
> Yes, done.
>>
>> http://cr.openjdk.java.net/~hseigel/bug_8136930.hs/test/compiler/unsafe/UnsafeGetConstantField.java.udiff.html 
>>
>>
>> Can you fix the indentation?
> Done.
>>
>> We should add an option -XX:+UseUnsafe to be a shortcut for this:
>>
>> --add-exports=java.base/jdk.internal.misc=ALL-UNNAMED
> Please see Alan's response.

I was looking at all the tests and was wondering if there's a way for 
some other option to imply these -add-exports options, so we wouldn't 
need so many.  But the patterns aren't all the same in all the tests, 
except the ALL-UNNAMED bit.

I've Reviewed this now.

Thanks,
Coleen


>>
>>
>>
>> The rest looks good.   This implementation looks very clean.
> Thanks! Harold
>>
>> Thanks,
>> Coleen
>>
>>
>> On 7/17/16 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/
>>>
>>> The changes were tested with the JCK lang and VM tests, the JTreg 
>>> hotspot tests, and the RBT hotspot nightlies.
>>>
>>> Thanks, Harold
>>
>