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

[harold seigel]

Hi Coleen,

I can move the readable_count++ to another line.

Harold


On 8/8/2016 5:04 PM, Coleen Phillimore 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
>>>>>
>>>>
>>>
>>
>