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

[Zoltán Majó]

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
>>>>>>
>>>>
>>