Revision2: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments (original) (raw)
Derek White derek.white at oracle.com
Mon Jun 1 22:13:23 UTC 2015
- Previous message: Revision2: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments
- Next message: Revision2: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Hi Gerard,
Still going over arguments.cpp and globals.hpp...
Here are a few issues found so far:
g1_globals.hpp:
G1ConcMarkStepDurationMillis: The range is declared as range(1.0, (double)max_uintx). But max_uintx has nothing to do with max double. It may not even be precisely representable as a double.
G1NewSizePercent: The constraint is checks that that its is <= G1MaxNewSizePercent, but not that it's >= 0. I assume that the uintx type is checking that?
arguments.cpp:
- Line 1405 - where did this come from? if (SurvivorAlignmentInBytes == 0) { SurvivorAlignmentInBytes = ObjectAlignmentInBytes; }
globals.hpp:
The variables StringTableSize and SymbolTableSize used to have ranges
- minimumStringTableSize..(max_uintx / StringTable::bucket_size()
- minimumSymbolTableSize..(max_uintx / SymbolTable::bucket_size() Now they are:
range(minimumStringTableSize, 111defaultStringTableSize) - range(minimumSymbolTableSize, 111defaultSymbolTableSize) This is OK? I didn't have a chance to calculate these myself.
- MinMetaspaceFreeRatio: used to have range 0..99, now it has range
0..100 and constraint <= MaxMetaspaceFreeRatio. - Not sure how important. - Actually the GC, runtime, and compiler groups should check all of their "percent" variables to see if having tighter ranges makes sense.
Line 2036, 2041:
- Set status to false instead of returning directly?
Line 2827 (existing); Change "K" -> "1*K" for consistency.
commandLineFlagConstraintList.hpp
- Initialize CommandLineFlagConstraintList._constraints in constructor to avoid dealing with NULL case?
- Shouldn't the default implementations for CommandLineFlagConstraint::apply_XXX() return failure, not success?
commandLineFlagConstraintsGC.cpp
Is this necessary?: 37 #include "c1/c1_globals.hpp" 40 #include "opto/c2_globals.hpp"
BAD TYPO:
- CMSOldPLABMaxConstraintFunc() is comparing against itself, not CMSOldPLABMin.
The rest of what I've seen looks good. Still looking...
- Derek
On 5/27/15 5:28 PM, Gerard Ziemski wrote:
hi all,
Here is a revision 2 of the feature taking into account feedback from Dmitry, David, Kim and Alexander. One significant change in this rev is the addition of runtime/commandLineFlagConstraintsCompiler.hpp/.cpp with one simple constraint needed by Dmitry's test framework. We introduce a new mechanism that allows specification of a valid range per flag that is then used to automatically validate given flag's value every time it changes. Ranges values must be constant and can not change. Optionally, a constraint can also be specified and applied every time a flag value changes for those flags whose valid value can not be trivially checked by a simple min and max (ex. whether it's power of 2, or bigger or smaller than some other flag that can also change) I have chosen to modify the table macros (ex. RUNTIMEFLAGS in globals.hpp) instead of using a more sophisticated solution, such as C++ templates, because even though macros were unfriendly when initially developing, once a solution was arrived at, subsequent additions to the tables of new ranges, or constraint are trivial from developer's point of view. (The intial development unfriendliness of macros was mitigated by using a pre-processor, which for those using a modern IDE like Xcode, is easily available from a menu). Using macros also allowed for more minimal code changes. The presented solution is based on expansion of macros using variadic functions and can be readily seen in runtime/commandLineFlagConstraintList.cpp and runtime/commandLineFlagRangeList.cpp In commandLineFlagConstraintList.cpp or commandLineFlagRangesList.cpp, there is bunch of classes and methods that seems to beg for C++ template to be used. I have tried, but when the compiler tries to generate code for both uintx and sizet, which happen to have the same underlying type (on BSD), it fails to compile overridden methods with same type, but different name. If someone has a way of simplifying the new code via C++ templates, however, we can file a new enhancement request to address that. This webrev represents only the initial range checking framework and only 100 or so flags that were ported from an existing ad hoc range checking code to this new mechanism. There are about 250 remaining flags that still need their ranges determined and ported over to this new mechansim and they are tracked by individual subtasks. I had to modify several existing tests to change the error message that they expected when VM refuses to run, which was changed to provide uniform error messages. To help with testing and subtask efforts I have introduced a new runtime flag: PrintFlagsRanges: "Print VM flags and their ranges and exit VM" which in addition to the already existing flags: "PrintFlagsInitial" and "PrintFlagsFinal" allow for thorough examination of the flags values and their ranges. The code change builds and passes JPRT (-testset hotspot) and UTE (vm.quick.testlist)
References: Webrev: http://cr.openjdk.java.net/~gziemski/8059557rev2 note: due to "awk" limit of 50 pats the Frames diff is not available for "src/share/vm/runtime/arguments.cpp" 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 hgstat: src/cpu/ppc/vm/globalsppc.hpp | 2 +- src/cpu/sparc/vm/globalssparc.hpp | 2 +- src/cpu/x86/vm/globalsx86.hpp | 2 +- src/cpu/zero/vm/globalszero.hpp | 3 +- src/os/aix/vm/globalsaix.hpp | 2 +- src/os/bsd/vm/globalsbsd.hpp | 29 +- src/os/linux/vm/globalslinux.hpp | 9 +- src/os/solaris/vm/globalssolaris.hpp | 4 +- src/os/windows/vm/globalswindows.hpp | 5 +- src/share/vm/c1/c1globals.cpp | 4 +- src/share/vm/c1/c1globals.hpp | 17 +- src/share/vm/gcimplementation/g1/g1globals.cpp | 16 +- src/share/vm/gcimplementation/g1/g1globals.hpp | 38 +- src/share/vm/opto/c2globals.cpp | 12 +- src/share/vm/opto/c2globals.hpp | 40 +- src/share/vm/prims/whitebox.cpp | 12 +- src/share/vm/runtime/arguments.cpp | 753 ++++++++++++++---------------------- src/share/vm/runtime/arguments.hpp | 24 +- src/share/vm/runtime/commandLineFlagConstraintList.cpp | 243 +++++++++++ src/share/vm/runtime/commandLineFlagConstraintList.hpp | 73 +++ src/share/vm/runtime/commandLineFlagConstraintsCompiler.cpp | 46 ++ src/share/vm/runtime/commandLineFlagConstraintsCompiler.hpp | 39 + src/share/vm/runtime/commandLineFlagConstraintsGC.cpp | 251 ++++++++++++ src/share/vm/runtime/commandLineFlagConstraintsGC.hpp | 59 ++ src/share/vm/runtime/commandLineFlagConstraintsRuntime.cpp | 67 +++ src/share/vm/runtime/commandLineFlagConstraintsRuntime.hpp | 41 ++ src/share/vm/runtime/commandLineFlagRangeList.cpp | 304 ++++++++++++++ src/share/vm/runtime/commandLineFlagRangeList.hpp | 67 +++ src/share/vm/runtime/globals.cpp | 699 +++++++++++++++++++++++++++------ src/share/vm/runtime/globals.hpp | 310 ++++++++++++-- src/share/vm/runtime/globalsextension.hpp | 101 +++- src/share/vm/runtime/init.cpp | 6 +- src/share/vm/runtime/os.hpp | 17 + src/share/vm/runtime/osext.hpp | 7 +- src/share/vm/runtime/thread.cpp | 6 + src/share/vm/services/attachListener.cpp | 4 +- src/share/vm/services/classLoadingService.cpp | 6 +- src/share/vm/services/diagnosticCommand.cpp | 3 +- src/share/vm/services/management.cpp | 6 +- src/share/vm/services/memoryService.cpp | 2 +- src/share/vm/services/writeableFlags.cpp | 161 +++++-- src/share/vm/services/writeableFlags.hpp | 52 +- test/compiler/c2/7200264/Test7200264.sh | 5 +- test/compiler/startup/NumCompilerThreadsCheck.java | 2 +- test/gc/arguments/TestHeapFreeRatio.java | 23 +- test/gc/arguments/TestSurvivorAlignmentInBytesOption.java | 6 +- test/gc/g1/TestStringDeduplicationTools.java | 6 +- test/runtime/CompressedOops/CompressedClassSpaceSize.java | 4 +- test/runtime/CompressedOops/ObjectAlignment.java | 9 +- test/runtime/contended/Options.java | 10 +- 50 files changed, 2730 insertions(+), 879 deletions(-)
- Previous message: Revision2: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments
- Next message: Revision2: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]