RFR 8130459(M): Add additional validation after heap creation

[sangheon.kim]

On 07/16/2015 09:49 PM, David Holmes wrote:
> On 17/07/2015 2:26 PM, sangheon.kim wrote:
>> Hi David,
>>
>> Thank you for reviewing this.
>> I added my answers below.
>>
>> On 07/16/2015 08:48 PM, David Holmes wrote:
>>> Hi Sangheon,
>>>
>>> On 17/07/2015 7:52 AM, sangheon.kim wrote:
>>>> Hi Coleen,
>>>>
>>>> Thank you for reviewing this.
>>>>
>>>> On 07/16/2015 01:05 PM, Coleen Phillimore wrote:
>>>>>
>>>>> http://cr.openjdk.java.net/~sangheki/8130459/webrev.00/src/share/vm/runtime/commandLineFlagConstraintList.cpp.udiff.html 
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> This
>>>>>
>>>>> +CommandLineFlagConstraint::ConstraintType
>>>>> CommandLineFlagConstraint::change_to_enum(const char* type) {
>>>>>
>>>>> function will return the enum if there is junk at the end of the
>>>>> string.  Is that expected?
>>>> No.
>>>> Now I'm thinking like below:
>>>> if (strncmp(type, afterParse_str, 10) == 0 && type[10] == '\0') {
>>>
>>> That seems fine to me.
>>>
>>> Overall this set of changes looks good to me, though I have a couple
>>> of nits:
>>>
>>> 1. It would be nice if the constraint type could default to AnyTime,
>>> rather than having to be explicitly declared - though I understand if
>>> the macros make that impractical.
>> I thought to have explicit declaration would be better.
>> And also I didn't want to adding more complexity into our macro. :)
>>
>>>
>>> 2. Why do you use AfterParse when it is really "after ergo" ? I can
>>> imagine you might need to distinguish the two in the future.
>> I didn't think of that.
>> Do you prefer 'AfterErgo'?
>
> That works for me.
Okay.

>
>>>
>>> 3. check_ranges_and_constraints_of_after_parse seems to have three 
>>> jobs:
>>> - a) possibly print ranges and constraints
>>> - b) check ranges
>>> - c) check constraints
>>> Given you are introducing multiple constraint checking phases I think
>>> it would be better to factor out the constraint part separately,
>> I agree to separating constraints part.
>>
>>> then you would not need check_constraints_of_after_parse or
>>> check_constraints_of_after_memory_init, as you could simply call eg.:
>>>
>>> check_constraints(CommandLineFlagConstraint::AfterMemoryInit);
>> I also thought about this approach but I thought accessing only via
>> CommandLineFlags class would be better rather than including
>> commandLineFlagConstraintList.hpp file.
>> But I don't have strong opinion on this.
>
> You could have the wrappers on CommandlineFlags class to avoid the 
> additional includes - though I'm not sure I'd be that worried about 
> it. But you could still have those wrappers just call 
> check_constraints and have check_constraints set the validating type.
I meant constraint type enum is at 'commandLineFlagConstraintList.hpp 
and currently 'globals.hpp' doesn't include headers except 'debug.hpp' 
and variant of 'globals_xxx.hpp'.
So I didn't want to add 'commandLineFlagConstraintList.hpp on that.
And also I have to add 'commandLineFlagConstraintList.hpp' at thread.cpp 
and universe.cpp to use as a parameter.

How about like below?
- bool check_ranges();
- bool check_constraints_of_after_ergo();
- bool check_constraints_of_after_memory_init();

>
> So when do Anytime constraints get checked? I seem to be missing that 
> part ??
Anytime is checked during argument processing.
e.g. checking double..
Arguments::parse_argument -> set_fp_numeric_flag -> 
CommandLineFlags::doubleAtPut -> apply_constraint_and_check_range_double.

>
>>>
>>> and have it set _validating_type.
>>>
>>> Also "check constraints of after" is not grammatically well formed.
>> These names mean 'check constraint of "AnyTime", "AfterParse",
>> "AfterMemoryInit"'. :)
>
> Ah I see I was parsing it wrong. Okay.
To avoid header file matter, these names became a little longer.

Thanks,
Sangheon


>
> Thanks,
> David
>
>>
>> David, I will prepare updated webrev after your answer.
>>
>> Thanks,
>> Sangheon
>>
>>
>>>
>>> Thanks,
>>> David
>>>
>>>> Or do you have any recommendation?
>>>> Let me send a revised webrev after hearing your opinion.
>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~sangheki/8130459/webrev.00/src/share/vm/runtime/commandLineFlagConstraintList.hpp.udiff.html 
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Generally the nonstatic data members of a class are written before 
>>>>> the
>>>>> functions (at the top of the class declaration).  You may have to put
>>>>> these after the enum declaration, but not at the bottom.
>>>> Okay, I will fix it.
>>>>
>>>>>
>>>>> I like this change.  I think it looks good.
>>>> Thank you!
>>>>
>>>> Sangheon
>>>>
>>>>>
>>>>> Coleen
>>>>>
>>>>>
>>>>> On 7/15/15 3:33 AM, sangheon.kim wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Please review this change of adding additional validation on
>>>>>> command-line flag checking framework.
>>>>>>
>>>>>> Recently we introduced Command-line flag checking framework
>>>>>> (JDK-8059557 <https://bugs.openjdk.java.net/browse/JDK-8059557>, JEP
>>>>>> 245: Validate JVM Command-Line Flag Arguments ) for ranges and
>>>>>> constraints.
>>>>>> And most cases this works great.
>>>>>> Unfortunately there are some flags which can be checked after heap
>>>>>> creation and these flags are hard to be checked under current
>>>>>> framework.
>>>>>> In this regard, I suggest to have an additional constraint checking.
>>>>>>
>>>>>> This suggestion adds one more constraint checking stage after memory
>>>>>> initialization.
>>>>>> And if we need more validations we can extend it.
>>>>>>
>>>>>> There are 2 big changes.
>>>>>>
>>>>>> 1. Flag declaration.
>>>>>> 'constraint(FUNC)' -> 'constraint(FUNC,TYPE)'
>>>>>> This webrev has 3 types, Anytime, AfterParse and AfterMemoryInit.
>>>>>> - Anytime: current constraint functions which don't use
>>>>>> CommandLineFlags::finishedInitializing().
>>>>>> - AfterParse: constraint functions which use
>>>>>> CommandLineFlags::finishedInitializing().
>>>>>> - AfterMemoryInit: I added only YoungPLABSize as an example.
>>>>>>   * All GC related flags will be covered by JDK-8078555
>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8078555>: GC: implement
>>>>>> ranges (optionally constraints) for those flags that have them
>>>>>> missing.
>>>>>>
>>>>>> 2. Previous constraint functions have to use
>>>>>> 'CommandLineFlags::finishedInitializing()' if the flag needs to be
>>>>>> checked after Argument::parse().
>>>>>> However, with this suggestion there's no need to call the function.
>>>>>> Instead framework will decide when the constraint function is needed
>>>>>> to be called.
>>>>>> And previously constraint functions are called twice while this
>>>>>> change will call each constraint function only one time.
>>>>>>
>>>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8130459
>>>>>> webrev: http://cr.openjdk.java.net/~sangheki/8130459/webrev.00/
>>>>>> Testing: JPRT, UTE(vm.quick-pcl) and tests at
>>>>>> test/runtime/CommandLine.
>>>>>>
>>>>>> Thanks,
>>>>>> Sangheon
>>>>>
>>>>
>>