RFR (S) round 0 for 8141068 Refactor -XXFlags= code in preparation for removal

[Daniel D. Daugherty]

On 11/11/15 9:43 AM, Coleen Phillimore wrote:
>
>
> On 11/11/15 11:36 AM, Daniel D. Daugherty wrote:
>> This needs a little bit of clarification:
>>
>> 3976 jint Arguments::match_special_option_and_act(const 
>> JavaVMInitArgs* args,
>> 3977                                              char ** 
>> vm_options_file,
>> 3978 ScopedVMInitArgs* args_out) {
>>
>> The 'vm_options_file' parameter is currently used for two things:
>>
>> 1) when the parameter is non-NULL, that indicates the caller
>>    supports '-XX:VMOptionsFile'; currently only the cmd line
>>    option bucket supports '-XX:VMOptionsFile'
>
> The args_out != NULL parameter indicates the same thing.

Yes. That's the check that used to be done. However, Ron
previously consolidated all of the checks to use the same
variable (vm_options_file). It seemed to confuse folks that
one check was done with 'vm_options_file' and the other check
was done with 'args_out'.


>> 2) makes sure that only one -XX:VMOptionsFile=foo option is
>>    specified on the cmd line
>
> You can do that with a local variable vm_options_file in 
> match_special_option_and_act.
>
> So it can be trivially removed.   But it's not a big deal.

Yes, that's possible. And then we would be back in the situation
where one check is done via 'args_out' and the other check is
done via 'vm_options_file'.

The other reason for having 'vm_options_file' have a more global
scope was that we didn't know whether the restriction for having
only one '-XX:VMOptionsFile' would apply across all buckets when
support was added for the env vars. Again, that's something that
will be dealt with JDK-8135198.

Dan


>
> Coleen
>>
>> Restriction #1 is implemented by:
>>
>> 3994       if (vm_options_file != NULL) {
>> <snip>
>> 4022       } else {
>> 4023         jio_fprintf(defaultStream::error_stream(),
>> 4024                     "VM options file is only supported on the 
>> command line\n");
>> 4025         return JNI_EINVAL;
>> 4026       }
>>
>> Restriction #2 is implemented by:
>>
>> 3994       if (vm_options_file != NULL) {
>> 3995         // The caller accepts -XX:VMOptionsFile
>> 3996         if (*vm_options_file != NULL) {
>> 3997           jio_fprintf(defaultStream::error_stream(),
>> 3998                       "The VM Options file can only be specified 
>> once and "
>> 3999                       "only on the command line.\n");
>> 4000           return JNI_EINVAL;
>> 4001         }
>>
>> So the 'vm_options_file' parameter cannot be removed at this
>> time without changing the current set of restrictions on the
>> '-XX:VMOptionsFile' option. Ron has another bug looking at
>> lifting those restrictions:
>>
>> JDK-8135198 Specifying a vm options file should not be limited
>>             to only once on the command line
>> https://bugs.openjdk.java.net/browse/JDK-8135198
>>
>> JDK-8135198 will require a CCC request in order to change the
>> semantics that were defined in the original work (JDK-8061999).
>>
>> Dan
>>
>>
>> On 11/10/15 2:02 PM, Ron Durbin wrote:
>>> Coleen
>>>
>>> This is to formally respond to your email.  Dan and I agreed the 
>>> change you are requesting should be done, but not
>>> in this change set.  Keeping this change set to only XX:Flags 
>>> changes. A future change set that could clean up
>>> the options file processing should address your issue.
>>>
>>> Ron
>>>
>>>
>>>> -----Original Message-----
>>>> From: Coleen Phillimore
>>>> Sent: Monday, November 09, 2015 2:03 PM
>>>> To: hotspot-runtime-dev at openjdk.java.net
>>>> Subject: Re: RFR (S) round 0 for 8141068 Refactor -XXFlags= code in 
>>>> preparation for removal
>>>>
>>>>
>>>> Ron,
>>>>
>>>> This looks really good!  Although I don't see why you need to pass
>>>> vm_options_file argument into match_special_options_and_act now since
>>>> it's not used afterward, so you only have to pass one NULL in the 
>>>> cases
>>>> where vm_options_file is not allowed.
>>>>
>>>> thanks,
>>>> Coleen
>>>>
>>>> On 11/6/15 2:05 PM, Ron Durbin wrote:
>>>>> This bug fix consists of a refactoring/cleanup of the -XX:Flags code.
>>>>>
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~rdurbin/8141068_OCR0_JDK9_webrev
>>>>>
>>>>>
>>>>> Bug report:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8141068
>>>>>
>>>>> This bug fix has been tested on:
>>>>>      OS:
>>>>>        Solaris, MAC, Windows, Linux
>>>>>      Tests:
>>>>>        Manual unit tests
>>>>>        JPRT with -testset hotspot(including the SQE current test 
>>>>> coverage for this feature.)
>>
>