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

[Lois Foltan]

+1
Lois

On 8/9/2016 2:24 PM, Coleen Phillimore wrote:
>
> 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
>