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

[David Holmes]

On 17/07/2015 4:19 PM, sangheon.kim wrote:
> 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();

Sure

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

Okay - it's buried :) In that case can I suggest using AtParse instead 
of Anytime - as it isn't really any time, it will happen when parsing 
that argument. Not a big deal if others object.

Thanks,
David

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