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

[Vladimir Kozlov]

Looks good. Please, ask an other reviewer to look on it to get 2 reviews - changes are not small.

Thanks,
Vladimir


On 8/28/14 1:31 AM, Zoltán Majó wrote:
> Hi Vladimir,
>
>
> On 08/27/2014 07:14 PM, Vladimir Kozlov wrote:
>> 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,
>
> I changed it. Here is the new webrev:
>
> http://cr.openjdk.java.net/~zmajo/8055286/webrev.03/
>
> JPRT runs fine.
>
> Thank you!
>
> Best regards,
>
>
> Zoltan
>
>>
>> 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
>>>>>>>
>>>>>
>>>
>