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

[sangheon.kim]

Hi David and Coleen,

Okay, I totally agree to your opinions.
And here is webrev v02.
http://cr.openjdk.java.net/~sangheki/8130459/webrev.02/

- String compare to avoid junk at the end of the string at 
CommandLineFlagConstraint::change_to_enum. (Coleen)
- Moved nonstatic data members to locate before the functions. (Coleen)
- Renamed 'Anytime => AtParse' and ' AfterParse => AfterErgo'. (David 
and Coleen)
- Separated 'CommandLineFlags::check_all_ranges_and_constraints' to 
'check_ranges', 'check_constraints_of_after_ergo'. (David)
   : This will change the behavior.
   If we check ranges and constraints separately and stop when one of 
them fails(at thread.cpp), 2 tests( 
test/runtime/CompressedOops/ObjectAlignment.java, 
test/runtime/contended/Options.java ) will fail. And these will be 
covered by JDK-8112746 
<https://bugs.openjdk.java.net/browse/JDK-8112746> (Followup to 
JDK-8059557).
   I just added patched test cases temporarily to pass JPRT.

Tested: JPRT, UTE(vm.quick-pcl), test/runtime/CommandLine/*

Thanks,
Sangheon


On 07/17/2015 05:23 AM, Coleen Phillimore wrote:
>
> I like David's new name suggestions as well.  Anytime => AtParse and 
> AfterParse => AfterErgo look better to me.
> That was the only thing I was unsure about in my review of this code 
> and didn't have a better suggestion.  The rest looks good to me.  I 
> prefer _not_ having default argument of AtParse and not adding that 
> complication to the macros.  It seems that the constraint writer 
> should specify phase when to check the argument.
>
> Thanks,
> Coleen
>
> On 7/17/15 2:42 AM, David Holmes wrote:
>> 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
>>>>>>>>
>>>>>>>
>>>>>
>>>
>