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

[gerard ziemski]

Thank you for the review.


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

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.


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

Fixed.


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

The string changes are to keep the code consistent and the biggest value 
here is to provide a patterneasy to apply for new constraints with 
clearily identified parts of strings that can be easily parsed and 
modified in parts, ie:

print_error_if_needed(verbose, *- same for all flags*
   "Flag (" FORMAT ") must be", *- similar**for all flags with "Flag" 
and "FORMAT" that need to be filled in*
   "greater than on equal to Flag (" FORMAT ")\n"; *- **this may need to 
change substantially from flag to flag*
   "value, Flag);" *- similar for all**except for the actual "F**lag"*


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

Snagheon will take over this as discusses in other email.


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

Right, I wasn't sure whether to factor this method out or not. I decided 
to have it local because it's trivial, but I will factor it out into 
src/share/vm/runtime/commandLineFlagRangeList.hpp/.cpp though that will 
require all the constraint files to include the 
commandLineFlagRange.List.hpp


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

Fixed.

I will post a webrev shortly after testing.



cheers