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

[Kim Barrett]

On Jul 20, 2015, at 3:48 PM, sangheon.kim <sangheon.kim at oracle.com> wrote:
> 
> Here's webrev.03 including your suggestion of 'removing change_to_enum()'.
> http://cr.openjdk.java.net/~sangheki/8130459/webrev.03/

------------------------------------------------------------------------------

I think I'm ok with the new functionality, though I'm having some
trouble following parts of it.  I think that is in part because some
things are not placed where I would expect them to be.  Here's a
suggestion that I think might help:

check_ranges should be a public static member function of
CommandLineFlagRangeList, and its definition should be with that
class.  CommandLineFlags::check_ranges should forward to it, or remove
that forwarding function and call
CommandLineFlagRangeList::check_ranges directly.

check_constraints should be a public static member function of
CommandLineFlagConstraintList, and its definition should be with that
class.  It should set _validating_type to the argument (after
verifying the argument is greater than current, or perhaps even the
next value after current), and set_validating_type should be
eliminated.

CommandLineFlags::check_constraints_of_after_xxx functions should call
CommandLineFlagConstraintList::check_constraints.  Alternatively,
eliminate those functions and instead call
CommandLineFlagConstraintList::check_constraints directly or via a
CommandLineFlags::check_constraints forwarding function.  [Be
consistent with check_ranges about forwarding function or not.]

Eliminating the check_constraints_of_after_xxx functions would also
involve moving the call to post_after_ergo_constraint_check, but I
think that's misplaced inside
CommandLineFlags::check_constraints_of_after_ergo.

Eliminating those check_constraints_of_after_xxx functions would have
the additional benefit of removing any concerns about their names; I
think another reviewer (David?) commented on them, and they strike me
as rather strange.

------------------------------------------------------------------------------  

Why do we need check_ranges?  Aren't ranges always checked when a
value is assigned?  I'm probably forgetting something, but I can't
right now find or recall a reason for needing this late check.  Oh,
maybe it's to validate the initial values for options that didn't get
set via command line or ergonomics?  If so, perhaps check_ranges
should be run much earlier?  But maybe some options don't have a value
at all until after apply_ergo is called?

This should be a separate discussion / RFE.

------------------------------------------------------------------------------   
The name post_after_ergo_constraint_check seems really odd.  I
understand how it comes about, but still...

------------------------------------------------------------------------------   
src/share/vm/runtime/commandLineFlagConstraintList.cpp
 223 #define INITIAL_CONTRAINTS_SIZE 16
...
 230   _constraints = new (ResourceObj::C_HEAP, mtInternal) GrowableArray<CommandLineFlagConstraint*>(INITIAL_CONTRAINTS_SIZE, true);

Pre-existing typo: INITIAL_CONTRAINTS_SIZE => INITIAL_CONSTRAINTS_SIZE

------------------------------------------------------------------------------ 
src/share/vm/runtime/commandLineFlagConstraintList.hpp
  71   const char* name() { return _name; }
  72   ConstraintType type() { return _validate_type; }

type() should be const.  Pre-existing: name() should be const.

------------------------------------------------------------------------------
src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
  50                   "ergornomic PLAB minimum size (" SIZE_FORMAT ")\n",
...
  65                   "ergornomic PLAB maximum size (" SIZE_FORMAT ")\n",

"ergornomic" => "ergonomic"

------------------------------------------------------------------------------ 
src/share/vm/runtime/globals.cpp  
1248     CommandLineFlagRange* range = CommandLineFlagRangeList::at(i);
1249     const char* name = range->name();
1250     Flag* flag = Flag::find_flag(name, strlen(name), true, true);
1251     if (flag != NULL) {

[Pre-existing code, though moved as part of this change.]

I don't understand the need for the "if (flat != NULL) {" wrapper.
Range objects are only created and registered with flags, so it
shouldn't be possible for that test to ever fail.  Note that the
constraint processing doesn't have such a test.

[Doing anything about this should be a separate RFE, and this question
is really directed more toward Gerard rather than Sangheon.]

------------------------------------------------------------------------------ 
src/share/vm/runtime/globals.cpp 
1213 bool CommandLineFlags::check_ranges() {
1214 //#define PRINT_RANGES_SIZES
1215 #ifdef PRINT_RANGES_SIZES
...
1274 static bool check_constraints(CommandLineFlagConstraint::ConstraintType constraint_type) {
1275 //#define PRINT_CONSTRAINTS_SIZES
1276 #ifdef PRINT_CONSTRAINTS_SIZES

[More or less pre-existing code, though moved and updated as part of
this change.]

Do we really need this development cruft cluttering the code?

[Doing anything about this should be a separate RFE, and this question
is really directed more toward Gerard rather than Sangheon.]

------------------------------------------------------------------------------