Review Request JDK-8136930 Examine implications for custom launchers, equivalent of java -X options in particular

[harold seigel]

Hi,

Please review this updated webrev of the VM changes for JDK-8136930:

    http://cr.openjdk.java.net/~hseigel/bug_8136930.02/

The updated webrev contains improvements suggested by David, Mandy, and 
Lois.

Thanks, Harold

On 6/5/2016 8:50 PM, David Holmes wrote:
> Hi Mandy,
>
> On 4/06/2016 4:47 PM, Mandy Chung wrote:
>> Webrev:
>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8136930/webrev.00/
>>
>> -modulepath, -addmods, -limitmods, -XaddExports, -XaddReads, -Xpatch 
>> are java launcher options in the current implementation.  Custom 
>> launchers will have to use -D to set some system properties to 
>> configure module system.  Different ways to configure module system 
>> is confusing and not friendly for environments using both java 
>> launcher and custom launchers.
>>
>> This patch pushes the handling of the module options into the VM.  
>> That will avoid the confusion between launcher and VM options and 
>> avoids needing to use system properties.  All launcher 
>> implementations can configure the module system via JNI Invocation 
>> API setting these options in a unified way.  The options and syntax 
>> remain the same as specified in JEP 261.
>>
>> For the non-repeating options, like the other VM options, the last 
>> one wins.  The current implementation communicates the options to the 
>> module system through system properties, as a private interface, and 
>> these system properties will be removed once they are read during the 
>> module system initialization. These system properties are reserved as 
>> private interface and they will be ignored if they are set via -D in 
>> the command line. Harold implements the hotspot change and can 
>> explain further details.
>>
>> This patch will impact existing tests and scripts that set the system 
>> properties for example to break encapsulation in the command line 
>> e.g. -Djdk.launcher.addexports.<N>.  They will need to be updated to 
>> replace the use of -D with the appropriate module option e.g. 
>> -XaddExports.  Since they are new options in JDK 9, use 
>> -XX:+IgnoreUnrecognizedVMOptions if they need to be ignored by 
>> earlier releases.
>
> Taking an initial look at the VM changes ...
>
> I may be missing the full context but you seem to be checking only 
> that some prefix matches the expected property format, not that the 
> entire value is valid eg. if passed jdk.module.patch.1junk it will match
>
> You use NEW_C_HEAP_ARRAY in places but that will abort the VM on 
> failure, whereas you return error codes for other failure modes. For 
> consistency use NEW_C_HEAP_ARRAY_RETURN_NULL and return an error on 
> NULL. (This seems a pre-existing flaw.)
>
> There seem to be far too many string literals for the various 
> jdk.module.* forms and lots of hard-coded string lengths. It would be 
> nice if that could be cleaned up using #defines, or string constant 
> variables, so that all the potential property names can be located in 
> one place.
>
> Are there any tests in hotspot/test/* that will require changes for this?
>
> Thanks,
> David
>
>> Mandy
>>
>>