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

[Coleen Phillimore]

Harold's changes look fine, although exactly what should be done for 
https://bugs.openjdk.java.net/browse/JDK-8162412 will need to be a 
follow-on issue.

Coleen


On 8/8/16 11:39 PM, Mandy Chung wrote:
> This is the full hotspot webrev containing all of Harold's incremental patches:
>    http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8136930/gnu-options/webrev-hotspot.03/
>
> I will be pushing the hotspot change for Harold together with the CLI work for jdk, langtools, and other repos once the code review is completed.
>
> FYI.  Changes for jdk, langtools and other repos are posted [1].
> Mandy
>
> [1] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2016-August/009025.html
>
>
>> On Aug 8, 2016, at 2:04 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
>>
>>
>> http://cr.openjdk.java.net/~hseigel/bug_8136930.hs3/src/share/vm/prims/jvmtiEnv.cpp.udiff.html
>>
>> *!_char **tmp_value = *property_ptr+readable_count++_;*
>>
>>
>> This looks really painful to me.  Can you add some parentheses?  Or put readable_count++ on the next statement.
>>
>> Is this an incremental webrev?  Can you send a full one?
>> thanks,
>> Coleen
>>
>>
>> On 8/8/16 1:25 PM, harold seigel wrote:
>>> Hi,
>>>
>>> Please review the latest version of this change.  It is similar to the below change except it changes uses of -mp to -p.
>>>
>>> http://cr.openjdk.java.net/~hseigel/bug_8136930.hs3/
>>>
>>> Thanks! Harold
>>>
>>>
>>> On 8/4/2016 8:46 AM, harold seigel wrote:
>>>> Hi,
>>>>
>>>> Please review this update for this fix.  This webrev only shows the changes since the last webrev.  These changes include:
>>>>
>>>> 1. Fix forJDK-8162415
>>>>     <https://bugs.openjdk.java.net/browse/JDK-8162415> - the JVM now
>>>>     prints the following message when ignoring a property and
>>>>     PrintWarnings is enabled:
>>>>     warning: Ignoring system property options whose names start with
>>>>     '-Djdk.module'.  They are reserved for internal use.
>>>>
>>>> 2. Fix for JDK-8162412
>>>>     <https://bugs.openjdk.java.net/browse/JDK-8162412>
>>>> 3. Fixes a problem where JVMTI was failing two JCK vm/jvmti tests
>>>>
>>>> 4. Incorporates review comments from Alan, Coleen, Dan, and Lois
>>>> 5. Fixes JTReg tests that failed due to the new option syntax.
>>>>
>>>> Revised webrev: http://cr.openjdk.java.net/~hseigel/bug_8136930.hs2/
>>>>
>>>> Thanks, Harold
>>>>
>>>> On 8/2/2016 9:25 AM, harold seigel wrote:
>>>>> 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