RFR (M): 8112746: Followup to JDK-8059557 (JEP 245: Validate JVM Command-Line Flag Arguments)

[Kim Barrett]

On Aug 10, 2015, at 1:13 PM, gerard ziemski <gerard.ziemski at oracle.com> wrote:
> 
> Here is a rev2 to rev3 diff that Kim requested: http://cr.openjdk.java.net/~gziemski/8112746_rev2_to_rev3/
> 
> On 08/07/2015 11:36 AM, gerard ziemski wrote:
>> References:
>> 
>>    JEP 245: https://bugs.openjdk.java.net/browse/JDK-8122937
>>        bug: https://bugs.openjdk.java.net/browse/JDK-8112746
>>     webrev: http://cr.openjdk.java.net/~gziemski/8112746_rev3/

Thanks for the incremental webrev.

Generally looks good.  I have a few changes to suggest, but I don't
see any show-stoppers.  I would be ok with deferring some or all of
these suggestions, so that we can be done with this change set and
unblock Sangheon's dependent work.

*** Could be fixed now or later:

------------------------------------------------------------------------------  
src/share/vm/runtime/commandLineFlagRangeList.hpp
src/share/vm/runtime/commandLineFlagRangeList.cpp
  class CommandLineError ...

I think this isn't the right place for this class.  I think a better
place would be globals.[ch]pp, following class CommandLineFlags.

I think such a change would eliminate the need for new #include of
"runtime/commandLineFlagRangeList.hpp" in
commandLineFlagConstraints{Compiler,GC,Runtime}.cpp, introduced in
this latest revision.

And of course, the definition of the print function should be moved to a
corresponding location in globals.cpp.

------------------------------------------------------------------------------
src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
  58 static Flag::Error MaxPLABSizeBounds(const char* name, size_t value, bool verbose) {
  59 #if INCLUDE_ALL_GCS
  60   if ((UseConcMarkSweepGC || UseG1GC) && (value > PLAB::max_size())) {
  61     CommandLineError::print(verbose,
  62                             "%s (" SIZE_FORMAT ") must be "
  63                             "less than ergonomic PLAB maximum size (" SIZE_FORMAT ")\n",
  64                             name, value, PLAB::min_size());

"less than" => "less than or equal to" to be consistent with test.

------------------------------------------------------------------------------  
commandLineFlagConstraints{Compiler,GC,Runtime}.cpp

With the refactoring for CommandLineError, I think these files no
longer need to #include "utilities/defaultStream.hpp".

------------------------------------------------------------------------------
src/share/vm/runtime/commandLineFlagConstraintList.cpp
 307 bool CommandLineFlagConstraintList::check_constraints(CommandLineFlagConstraint::ConstraintType type) {
...
 327       } else if (flag->is_int()) {
 328         int value = flag->get_int();
 329         if (constraint->apply_int(value, true) != Flag::SUCCESS) status = false;
 330       } else if (flag->is_uint()) {
 331         uint value = flag->get_uint();
 332         if (constraint->apply_uint(value, true) != Flag::SUCCESS) status = false;
...

[Latest version added missing cases.]

Suggest adding a final else clause to the flag type dispatch that is a
ShouldNotReachHere or some such.

Similary in CommandLineFlagRangeList::check_ranges(). 

------------------------------------------------------------------------------  
src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
  71 static Flag::Error MinMaxPLABSizeBounds(const char* name, size_t value, bool verbose) {
  72   if (MinPLABSizeBounds(name, value, verbose) == Flag::SUCCESS) {
  73     return MaxPLABSizeBounds(name, value, verbose);
  74   }
  75   return Flag::VIOLATES_CONSTRAINT;
  76 }

I'd prefer this returned the result of MinPLABSizeBounds when that
isn't SUCCESS, rather than discarding that result and returning
VIOLATES_CONSTRAINT.  I think there isn't presently a difference, but
every time I look at this I need to verify that belief.

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

*** Should be fixed later:

------------------------------------------------------------------------------ 
src/share/vm/runtime/commandLineFlagConstraintList.cpp
 307 bool CommandLineFlagConstraintList::check_constraints(CommandLineFlagConstraint::ConstraintType type) {
...
 319     Flag* flag = Flag::find_flag(name, strlen(name), true, true);
 320     // We must check for NULL here as lp64_product flags on 32 bit architecture
 321     // can generate constraint check (despite that they are declared as constants),
 322     // but they will not be returned by Flag::find_flag()
 323     if (flag != NULL) {

This comment suggests to me that there is a bug in the underlying
constraint registration infrastructure; why are we registering
constraints for options that don't exist?

Similary in CommandLineFlagRangeList::check_ranges(). 

I think this should be addressed as a followup RFE, rather than as
further work on this change set.  It's a pre-existing problem, not
something introduced by this change set.

------------------------------------------------------------------------------  
src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
 218 Flag::Error CMSPrecleanNumeratorConstraintFunc(uintx value, bool verbose) {
 219   if (value > (CMSPrecleanDenominator - 1)) {
 220     CommandLineError::print(verbose,
 221                             "CMSPrecleanNumerator (" UINTX_FORMAT ") must be "
 222                             "less than or equal to CMSPrecleanDenominator - 1 (" UINTX_FORMAT ")\n",
 223                             value, CMSPrecleanDenominator - 1);

[pre-existing]

Wouldn't this be simpler with (value >= CMSPrecleanDenominator) and
just "less than" (or "strictly less than" for consistency with wording
elsewhere; same wording should be used in all such places, but I don't
care which).

This can be handled as a separate (small) cleanup; maybe give it to
Sangheon to deal with among the other GC-related option processing
improvements.

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