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

[David Holmes]

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.

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.

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, 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);

and have it set _validating_type.

Also "check constraints of after" is not grammatically well formed.

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