Revision3: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments

[Kim Barrett]

On Jun 13, 2015, at 10:32 AM, Gerard Ziemski <gerard.ziemski at oracle.com> wrote:
> 
> Here is a revision 3 of the feature taking into account feedback from Kim Barrett, David Holmes, Derek White and Bengt Rutisson. I have also merged the changes to the latest jdk9.
> 
> […]
> 
> The code change builds and passes JPRT (-testset hotspot) and UTE (vm.quick.testlist) 
> 
> 
> References: 
> 
>          Webrev: 
> http://cr.openjdk.java.net/~gziemski/8059557_rev3
> 
>             JEP: 
> https://bugs.openjdk.java.net/browse/JDK-8059557
> 
> Compiler subtask: 
> https://bugs.openjdk.java.net/browse/JDK-8078554
> 
>      GC subtask: 
> https://bugs.openjdk.java.net/browse/JDK-8078555
> 
> Runtime subtask: 
> https://bugs.openjdk.java.net/browse/JDK-8078556
> 
> 
> Note: due to "awk" limit of 50 pats the Frames diff is not available for "src/share/vm/runtime/arguments.cpp” and "src/share/vm/runtime/globals.cpp”
> 
> 
> Followup issues:
> 
> JDK-8085866: There are 99 uses of FLAG_SET_ERGO, should they check the return value?
> JDK-8085864: FLAG_SET_CMDLINE in TestGenCollectorPolicy() currently ignore the return values
> JDK-8081842: Find a better place for EMIT_{CONSTRAINTS,RANGES}_FOR_GLOBALS_EXT
> JDK-8081833: There is a large amount of code near-duplication among the various CommandLineFlagRange_<type> classes
> JDK-8081519: Split globals.hpp to factor out the Flag values needed by JDK-8059557

Here are a few comments on Revision3.  I think all of these can (and
should) be handled via followup CRs, and that none of these should
prevent Revision3 from being pushed as is.

If another round of review for this change does prove to be needed (I
hope not), please provide an incremental webrev in the review package.

------------------------------------------------------------------------------ 
src/share/vm/gc/g1/g1_globals.hpp
 71   product(double, G1ConcMarkStepDurationMillis, 10.0,                       \
 72           "Target duration of individual concurrent marking steps "         \
 73           "in milliseconds.")                                               \
 74           range(1.0, (double)max_uintx)                                     \

The new upper bound here seems like a completely random number to me.

------------------------------------------------------------------------------
src/share/vm/opto/c2_globals.hpp
110   notproduct(intx, IndexSetWatch, 0,                                        \
111           "Trace all operations on this IndexSet (-1 means all, 0 none)")   \
112           range(-1, 0)                                                      \

I suspect the upper bound here should be uint_max.

------------------------------------------------------------------------------
src/share/vm/runtime/globals.hpp 
375   static const char* flag_error_str(Flag::Error error) {

All other functions for the Flag are declared in the .hpp and defined
in the .cpp file.  I don't see any reason for this function to be
different.

------------------------------------------------------------------------------ 
src/share/vm/runtime/globals.hpp   
1563   product(size_t, HeapSizePerGCThread, ScaleForWordSize(64*M),              \
1564           "Size of heap (bytes) per GC thread used in calculating the "     \
1565           "number of GC threads")                                           \
1566           range((uintx)os::vm_page_size(), max_uintx)                       \

Type is size_t, but range values are using uintx.

------------------------------------------------------------------------------ 
src/share/vm/runtime/globals.hpp  
1846   product(size_t, MarkStackSizeMax, NOT_LP64(4*M) LP64_ONLY(512*M),         \
1847           "Maximum size of marking stack")                                  \
1848           range(1, (max_jint - 1))                                          \

That's a very odd looking maximum.

------------------------------------------------------------------------------ 
src/share/vm/services/classLoadingService.cpp
35 #include "utilities/defaultStream.hpp"

I think utilities/ostream.hpp might be sufficient here.

------------------------------------------------------------------------------ 
src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
100 #define UseConcMarkSweepGCWorkaroundIfNeeded(initial, max) { \
101   if ((initial == 7) && (max == 6)) { \
102     return Flag::SUCCESS; \
103   } \
104 }

The do/while(0) macro idiom is the usual way to encapsulate
statements.  It took me a couple of tries to parse this, where it
would have been immediately obvious with the do/while(0) idiom.

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

In the various apply_constraint_and_check_range_TYPE functions, the
present pattern seems to be to find and check the range (if it
exists), then find and apply the constraint (if it exists), and then
combine the results.

I think it might be better to not apply the constraint if there is a
range check failure.  That way, constraint functions can be written to
assume that the values have been range checked, and only need to worry
about whatever additional constraints might exist, without the need to
potentially deal with out of range values.

Whichever behavior is used should be documented as part of the
interface to this facility, since it affects how one writes constraint
functions.  It might also, in some cases, affect whether it is useful
to provide a range spec in conjunction with a constraint function.

Such a change might also eliminate the need for the get_status_error
helper.

[Obviously this is pretty high priority if handled as a followup
change, since it has a somewhat significant impact on how one writes
constraint functions.]

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