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

[Zoltán Majó]

Thank you, Vladimir!

I'll ask an other reviewer then.

Best regards,


Zoltan

On 08/28/2014 05:45 PM, Vladimir Kozlov wrote:
> 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
>>>>>>>>
>>>>>>
>>>>
>>