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

[Kim Barrett]

On Jul 31, 2015, at 2:04 PM, gerard ziemski <gerard.ziemski at oracle.com> wrote:
> 
> Please review this webrev 2 of the follow-up fixes including:
> 
> - Adding DBL_MAX (requested by Kim Barrett)
> 
> - 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)
> 
> - Add print_error_as_needed() utility function to range/constraint code to improve code robustness (requested by Coleen)
> 
> - Remove debug code (requested by Kim Barrett)
> 
> - Implement Flag::flag_error_str in .cpp file (requested by Kim Barrett)
> 
> - Only check constraint if range passes (requested by Kim Barrett)
> 
> - Merged with Sangheon Kim fix for "Add additional validation after heap creation” (JDK-8130459)
> 
> 
> This webrev passes "JPRT -testset hotspot” and “RBT vm.quick testlist”
> 
> 
> 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_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.

------------------------------------------------------------------------------ 
src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
  71                           "greater than ergonomic PLAB minimum size (" SIZE_FORMAT ")\n",

Copy-paste bug: this should be "less than ... maximum".

The formatting changes in the strings seem (mostly?) unnecessary, and
make the review much more difficult.

[Later]  After trying and failing to review these changes (my eyes
keep crossing) I'd really prefer the string formatting changes were
reverted, unless there's some real change hiding in there that I'm
failing to see.

------------------------------------------------------------------------------  
src/share/vm/gc/g1/g1_globals.hpp
  75           range(1.0, DBL_MAX)                                               \

I vaguely recall an issue being raised about this causing problems
with the output when dumping the option values and their ranges.
Something about the printed value of DBL_MAX being excessively long.

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

Duplicated print_error_if_needed.

------------------------------------------------------------------------------
src/share/vm/runtime/globals.cpp
 518 const char* Flag::flag_error_str(Flag::Error error) {
 519   switch (error) {
...
 528     default: return "NULL";

I think that default is a "should not reach here" situation?

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