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

[harold seigel]

Thanks Lois!

Harold


On 8/9/2016 4:12 PM, Lois Foltan wrote:
> +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
>>
>