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

Tue Jul 21 22:11:58 UTC 2015

Hi Coleen,

Thank you for reviewing this.

Sangheon

On 07/21/2015 02:13 PM, Coleen Phillimore wrote:
>
> This looks good.  I reviewed version .03.
> Thank you for adding this extra support to command line verification.
>
> Coleen
>
> On 7/20/15 3:48 PM, sangheon.kim wrote:
>> Hi Dmitry,
>>
>> On 07/20/2015 12:05 PM, Dmitry Dmitriev wrote:
>>> Sangheon, please see my comment inline.
>>>
>>> On 20.07.2015 20:24, sangheon.kim wrote:
>>>> Hi Dmitry,
>>>>
>>>> Thanks for reviewing this.
>>>> I added inline answer.
>>>>
>>>> On 07/20/2015 10:09 AM, Dmitry Dmitriev wrote:
>>>>> Hello Sangheon,
>>>>>
>>>>> I think that you can rid from 
>>>>> CommandLineFlagConstraint::change_to_enum function by using 
>>>>> EMIT_CONSTRAINT_CHECK macro in following form:
>>>>> #define EMIT_CONSTRAINT_CHECK(func, 
>>>>> type)                               , func, 
>>>>> CommandLineFlagConstraint::type
>>>> The idea of changing to enum seems nice.
>>>> But with this approach we can't detect typos or wrong types.
>>> This approach allow to detect typos, because code in this case will 
>>> not compile. For example if we specify "AtPurse" instead of 
>>> "AtParse", then after preprocessor it will result to 
>>> "CommandLineFlagConstraint::AtPurse" and this value will not be 
>>> found in ConstraintType enum. Or you mean something different?
>> You are right! Sorry, I didn't check yours correctly.
>> As 'type' belongs to 'CommandLineFlagConstraint' enum, your 
>> suggestion will work correctly including detection of typos.
>>
>> Here's webrev.03 including your suggestion of 'removing 
>> change_to_enum()'.
>> http://cr.openjdk.java.net/~sangheki/8130459/webrev.03/
>>
>> Thanks,
>> Sangheon
>>
>>
>>>
>>> Thanks,
>>> Dmitry
>>>
>>>>
>>>> Thanks,
>>>> Sangheon
>>>>
>>>>
>>>>>
>>>>> And changing emit_constraing_##type functions accordingly. E.g. to
>>>>> void emit_constraint_bool(const char* name, 
>>>>> CommandLineFlagConstraintFunc_bool func, 
>>>>> CommandLineFlagConstraint::ConstraintType type) {
>>>>>   CommandLineFlagConstraintList::add(new 
>>>>> CommandLineFlagConstraint_bool(name, func, type));
>>>>> }
>>>>>
>>>>> But probably I miss something and not take something into account, 
>>>>> so fell free to correct me. Thanks!
>>>>>
>>>>> Dmitry
>>>>>
>>>>> On 18.07.2015 8:11, 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.
>>>>>>
>>>>>> 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
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>