RFR 8130459(M): Add additional validation after heap creation
Coleen Phillimore
coleen.phillimore at oracle.com
Tue Jul 21 21:13:33 UTC 2015
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
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-dev
mailing list