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

[Vladimir Kozlov]

Hi, Zoltan

We usually try to avoid assignments in 'if' statements (to avoid 
confusion between '=' and '==' which bug prone). Please, separate it:

+           if ((match = scan_flag_and_value(option, line, bytes_read,

Otherwise it  looks good.

Thanks,
Vladimir

On 8/27/14 12:27 AM, Zoltán Majó wrote:
> Hi Vladimir,
>
>
> thank you for the feedback.
>
> On 08/25/2014 08:50 PM, Vladimir Kozlov wrote:
>> You don't need changes in globals.* files now.
>
> I forgot about those files, sorry. I reverted them 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);
>
> OK, I changed the methods.
>
>> Why you dup flag: strdup(flag)?
>> TypedMethodOptionMatcher() does it already: os::strdup_check_oom(opt).
>
> You're right, it's unnecessary and I've removed it.
>
>> 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);
>
> You're right, but I removed that code as you suggested not to specialize
> that constructor (please see below).
>
>> Please, use underscore for '_option' field's name too, even if it was
>> not before.
>
> OK.
>
>> 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.
>
> You're right. I removed all specializations of that constructor.
>
>> Why you need specialization for CompilerOracle::option_value()?
>
> I changed has_option_value() to a template method and so we have only
> one declaration in compilerOracle.hpp.
>
> But we need to explicitly instantiate the method for all OptionTypes
> supported, because the method relies on some data/functionality that is
> available only from within CompilerOracle and thus it is not possible
> to  instantiate it in other compilation units. The explicit
> instantiations are in compilerOracle.cpp.
>
>> 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.
>
> I changed it.
>
> Here is the updated webrev:
> http://cr.openjdk.java.net/~zmajo/8055286/webrev.02/
>
> I ran JPRT again.
>
> Thank you and best regards,
>
>
> Zoltan
>
>> 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
>>>>>
>>>
>