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

[Zoltán Majó]

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