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

[Kim Barrett]

On Aug 3, 2015, at 12:11 PM, gerard ziemski <gerard.ziemski at oracle.com> wrote:
> 
> On 07/31/2015 08:15 PM, Kim Barrett wrote:
>> On Jul 31, 2015, at 2:04 PM, gerard ziemski <gerard.ziemski at oracle.com>
>>> 
>>> 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_rev2/
>> Not a full review, for reasons that will be discussed below.  I
>> decided to stop and see if I can convince you to do something about
>> the first couple of items before I go any further.
>> 
>> ------------------------------------------------------------------------------ 
>> I really wish this had been broken up into separate issues with
>> associated changesets, rather than one large combined changeset.
>> 
>> I realize that breaking it up will be more work for you, but it would
>> make a substantial difference in the quality of my review.
>> 
> 
> Yes, I thought about that, and the more work for myself is not the big concern here - it's more work for everyone. I am not a commitrer, so I would need a sponsor for very single change, and lastly each change requires vigorous testing, which takes a long while and uses JPRT/RBT resources. I would prefer if we made one last effort here and clear up this backlog if that's OK. 

Sponsoring a reviewed and well tested change is usually not a big deal
for the sponsor; I'm happy to do it for you, especially now that we're
both normally operating in the same repository.  I understand the
testing overhead, though different kinds of changes might require
different levels of testing.

That said, I went ahead and made my way through the existing webrev,
with comments below.  Your explanation of the rationale behind the
string formatting changes was helpful; still difficult to review, but
at least now I know why I'm working so hard :-)

------------------------------------------------------------------------------  
> - Passing values by value, not pointer, to constraint functions. It
> was thought originally that me way want to “fix” the values, but it
> was decided to be out of scope for the JEP. (requested by Coleen)

I keep waffling about this, so I'll go ahead an mention it, even
though I don't yet have a strong opinion either way.

For the writer of a constraint function, not needing to deal with a
pointer is clearly better.

However, I actually kind of thought it was a feature that the values
were passed by pointer reference rather than by value (though the
pointer should be const-qualified if no "fixup" capability is
intended).  That prevents certain kinds of bugs due to implicit
conversions; the types involved here provide pretty much all
conversions between pairs of types.  I'm also concerned about bugs
resulting from reversal of the value and verbose arguments, especially
since their order is not consistent throughout the internal APIs.

There are two different pathways to these constraint functions, one of
which is still using (and needs to use) pointers.  This change
dereferences these pointers, rather than taking the address of values
on the other path.  So which approach is used is a wash from the
caller's perspective.

------------------------------------------------------------------------------   
src/share/vm/runtime/commandLineFlagConstraintsGC.cpp

Pre-existing issue:
It seems like there is a lot of very similar parameterized
boiler-plate code here, and that using macros or templates could
significantly reduce the amount of source code.  Of course, it would
be important to accomplish such a shortening without involving
infrastructure that is itself overly complex, but I don't think
anything particularly tricky should be needed here.

This can be left for a later cleanup, or even ignored if nobody else
is bothered by it.

------------------------------------------------------------------------------   
src/share/vm/runtime/commandLineFlagConstraintsGC.cpp 
  66 static Flag::Error MaxPLABSizeBounds(const char* name, bool verbose, size_t value) {
...
  71                           "greater than ergonomic PLAB minimum size (" SIZE_FORMAT ")\n",
  72                           name, value, PLAB::min_size());

In addition to the copy-paste error already mentioned for line 71, on
line 72 PLAB::min_size() should be PLAB::max_size().

------------------------------------------------------------------------------  
src/share/vm/runtime/commandLineFlagConstraintsGC.cpp 
  79 static Flag::Error MinMaxPLABSizeBounds(const char* name, bool verbose, size_t value) {
  80   if (MinPLABSizeBounds(name, verbose, value) == Flag::SUCCESS) {
  81     return MaxPLABSizeBounds(name, verbose, value);
  82   }
  83   return Flag::VIOLATES_CONSTRAINT;
  84 }

I think it would be more maintainable to capture the result of
MinPLABSizeBounds in a "status" variable. If status is SUCCESS, set
status to result of MaxPLABSizeBounds.  Return status.  This
eliminates assumptions about possible MinPLABSizeBounds results.

But see later comment about these helper functions.

------------------------------------------------------------------------------  
src/share/vm/runtime/commandLineFlagConstraintsGC.cpp 
  53 static Flag::Error MinPLABSizeBounds(const char* name, bool verbose, size_t value) {
  66 static Flag::Error MaxPLABSizeBounds(const char* name, bool verbose, size_t value) {
  79 static Flag::Error MinMaxPLABSizeBounds(const char* name, bool verbose, size_t value) {
  86 Flag::Error YoungPLABSizeConstraintFunc(bool verbose, size_t value) {

Pre-existing issue:
While I'm all for using helper functions to break things up into
bite-sized chunks, in this case I think the helper functions are
actually making the code longer and more complicated.  A helper for
the two calls to print_error_if_needed might be useful if this
approach were taken.

Please take this as a suggestion to look at, rather than a required
change.

------------------------------------------------------------------------------  
src/share/vm/runtime/commandLineFlagConstraintsGC.cpp 
  53 static Flag::Error MinPLABSizeBounds(const char* name, bool verbose, size_t value) {
  54 #if INCLUDE_ALL_GCS
  55   if ((UseConcMarkSweepGC || UseG1GC) && (value < PLAB::min_size())) {
  56     print_error_if_needed(verbose,
  57                           "%s (" SIZE_FORMAT ") must be "
  58                           "greater than ergonomic PLAB minimum size (" SIZE_FORMAT ")\n",

Pre-existing defect:
Message says "greater than" but test is "greater or equal".

The obvious fix to the previously mentioned problems in
MaxPLABSizeBounds would have a similar problem.

------------------------------------------------------------------------------   
src/share/vm/runtime/commandLineFlagConstraintsRuntime.cpp
src/share/vm/runtime/commandLineFlagRangeList.cpp

Compiler and GC constraint functions consistently use error messages
of the form:

    <optionname> (<optionvalue>) ...

In this file the error messages are of the form

    <optionname>=<optionvalue>

The range checking functions also use the second form.

These should all be consistent.

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