[9] RFR(S): 8055286: Extend CompileCommand=option to handle numeric parameters

[Vladimir Kozlov]

You don't need changes in globals.* files now.

New access methods should return boolean, this way it will be easier to use:

static bool has_option_value(methodHandle method, const char* option, intx& value);

Why you dup flag: strdup(flag)?
TypedMethodOptionMatcher() does it already: os::strdup_check_oom(opt).
Don't assign option(opt) which is replaced immediately:

   MethodMatcher(class_name, class_mode, method_name, method_mode, signature, next),
                 option(opt), _type(BoolType), _value(value) {
     option = os::strdup_check_oom(opt);

Please, use underscore for '_option' field's name too, even if it was not before.

 From what I see the only reason you have TypedMethodOptionMatcher constructor specialization is _type(UnknownType) 
setting. But why you are not using your new functions: get_type_for<T>()? You have type check before already: 
strcmp(options, "intx") etc, so only known types are used.

Why you need specialization for CompilerOracle::option_value()?

scan_flag_and_value() should return 'match' value as add_option_string() to be consistent. The success could be checked 
by (match != NULL) as in other cases.

On 8/25/14 10:56 AM, Zoltán Majó wrote:
> Hi Vladimir,
>
>
> thank you for the feedback.
>
> On 08/22/2014 07:11 PM, Vladimir Kozlov wrote:
>> Sometimes we specify flag which is not declared as global flag in globals.hpp, for example NoRTMLockEliding. Please,
>> don't remove such ability:
>
> I did not know that, thanks for pointing it out.
>
>>
>> +           Flag *declared_flag = Flag::find_flag(flag, strlen(flag));
>>
>> Flags specified in 'option' command do not affect global flags. We use the same name only for convenience.
>
> I removed checking if a flag is declared in globals.hpp.
>
> But I think it is important that the type of a flag specified by the user matches the type expected by the compiler
> (otherwise the compiler could read random values at runtime). The type of a flag is therefore recorded in
> TypedMethodOptionMatcher and it is checked at runtime (line 381). If the two types do not match for a method m(), the
> default value of the flag is used for that method.

There is check, strcmp(options, "intx") etc, which guarantees the type consistency.

>
>> Could you move new option code from parse_from_line() method to separate method()?
>
> Yes, I did that. Please see the scan_flag_and_value() function.

Good.

>
>> Also parts of that code also could be factored out, like codes which process 'intx' and 'uintx' values - they are very
>> similar.
>
> I tried that, but it is not straightforward, as on some platforms sscanf() does not support format strings created at
> runtime. Without a common sscanf() call, the two branches are too little similar, but I might be wrong on that.
>
> So I decided to leave the code processing intx and uintx values unchanged. I hope that factoring out functionality into
> the scan_flag_and_value() function made the code already more readable.

Looks good.

>
>> Use 'Klass' in second line too:
>> +       // (1) CompileCommand=option,Klass::method,flag
>> +       // (2) CompileCommand=option,KLASS::method,type,flag,value
>
> Changed that as well.
>
> Here is the updated webrev: http://cr.openjdk.java.net/~zmajo/8055286/webrev.01/
>
> I ran the JPRT tests again, all pass.

Thanks,
Vladimir

>
> Thank you and best regards
>
>
> Zoltan
>
>> Thanks,
>> Vladimir
>>
>> On 8/22/14 2:12 AM, Zoltán Majó wrote:
>>> Hi,
>>>
>>>
>>> please review the following patch.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8055286
>>>
>>> Problem: Currently, CompileCommand=option handles only flags of type bool. CompileCommand=option should be extended to
>>> handle numeric types as well.
>>>
>>> Solution: This patch adds support for processing flags of type intx and uintx (in addition flags of type bool). Support
>>> for flags of type ccstr and ccstrlist is not added by this patch; we can add support for those types when it is needed.
>>>
>>> Webrev: http://cr.openjdk.java.net/~zmajo/8055286/
>>>
>>> Testing: JPRT, manual testing
>>>
>>> Thank you and best regards,
>>>
>>>
>>> Zoltan
>>>
>