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

[sangheon.kim]

Hi David,

On 07/20/2015 01:58 AM, David Holmes wrote:
> Hi Sangheon,
>
> On 18/07/2015 3:11 PM, sangheon.kim wrote:
>> 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.
>
> You have to fix the tests before this can be pushed else you won't get 
> the push through JPRT.
Okay I will fix these tests before pushing this change.
Or wait until 8112746 pushed.

BTW, may I ask the reason?
I thought tests which run during JPRT are used from the source code 
which I submitted.

Thanks,
Sangheon


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