Revision1: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments
Dmitry Dmitriev
dmitry.dmitriev at oracle.com
Sun May 24 20:52:29 UTC 2015
Hi Gerard,
I have found new several issues. Here my comments for 2 modules:
1) In src/share/vm/runtime/commandLineFlagConstraintsGC.cpp:
156 Flag::Error G1MaxNewSizePercentConstraintFunc(bool verbose, uintx* value) {
157 if ((CommandLineFlags::finishedInitializing() == true) && (*value < G1NewSizePercent)) {
158 if (verbose == true) {
159 jio_fprintf(defaultStream::error_stream(),
160 "G1MaxNewSizePercent (" UINTX_FORMAT ") must be less than or "
161 "equal to G1NewSizePercent (" UINTX_FORMAT ")\n",
162 *value, G1NewSizePercent);
163 }
Message on line 160 must state that G1MaxNewSizePercent must be greater
than G1NewSizePercent.
186 Flag::Error CMSOldPLABMaxConstraintFunc(bool verbose, size_t* value) {
187 if ((CommandLineFlags::finishedInitializing() == true) && (*value < CMSOldPLABMax)) {
188 if (verbose == true) {
189 jio_fprintf(defaultStream::error_stream(),
190 "CMSOldPLABMax (" SIZE_FORMAT ") must be greater than or "
191 "equal to CMSOldPLABMax (" SIZE_FORMAT ")\n",
192 *value, CMSOldPLABMax);
193 }
194 return Flag::VIOLATES_CONSTRAINT;
It seems that this function perform wrong check. It verifies value for
CMSOldPLABMax and compare it against CMSOldPLABMax. I think that it
should be compared against CMSOldPLABMin. In this case error message
should be corrected.
228 Flag::Error SurvivorAlignmentInBytesConstraintFunc(bool verbose, intx* value) {
229 if (CommandLineFlags::finishedInitializing() == true) {
230 if (*value != 0) {
231 if (!is_power_of_2(*value)) {
232 if (verbose == true) {
233 jio_fprintf(defaultStream::error_stream(),
234 "SurvivorAlignmentInBytes (" INTX_FORMAT ") must be power of 2\n",
235 *value);
236 }
237 return Flag::VIOLATES_CONSTRAINT;
238 }
239 if (SurvivorAlignmentInBytes < ObjectAlignmentInBytes) {
240 if (verbose == true) {
241 jio_fprintf(defaultStream::error_stream(),
242 "SurvivorAlignmentInBytes (" INTX_FORMAT ") must be greater "
243 "than ObjectAlignmentInBytes (" INTX_FORMAT ") \n",
244 *value, ObjectAlignmentInBytes);
245 }
246 return Flag::VIOLATES_CONSTRAINT;
247 }
On line 239 "*value" should be instead of "SurvivorAlignmentInBytes".
2) In src/share/vm/runtime/commandLineFlagConstraintsRuntime.cpp:
33 Flag::Error ObjectAlignmentInBytesConstraintFunc(bool verbose, intx* value) {
34 if (!is_power_of_2(*value)) {
35 if (verbose == true) {
36 jio_fprintf(defaultStream::error_stream(),
37 "ObjectAlignmentInBytes=%d must be power of 2\n",
38 (int)*value);
39 }
40 return Flag::VIOLATES_CONSTRAINT;
41 }
42 // In case page size is very small.
43 if ((int)*value >= os::vm_page_size()) {
44 if (verbose == true) {
45 jio_fprintf(defaultStream::error_stream(),
46 "ObjectAlignmentInBytes=%d must be less than page size %d\n",
47 (int)*value, os::vm_page_size());
48 }
I understand that ObjectAlignmentInBytesConstraintFunc have not huge
upper range and in this code not introduce problems, so it can be leaved
as is. I think that on lines 37-38 and 46 it is unnecessary to convert
"*value" to "int", because instead of "%d" format you can use
INTX_FORMAT. Also, on line 43 os::vm_page_size can be converted to wide
type(from int to intx) instead of converting *value to narrow type(on 64
bit systems from intx to int), i.e. use following compare statement
(*value >= (intx)os::vm_page_size()).
Regards,
Dmitry
On 21.05.2015 18:56, Gerard Ziemski wrote:
> hi all,
>
> Here is a revision 1 of the feature taking into account feedback from
> Coleen, Dmitry and David. I will be responding to each of the feedback
> emails shortly.
>
> 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. RUNTIME_FLAGS 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 size_t, 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/8059557_rev1/
> 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/globals_ppc.hpp | 2 +-
> src/cpu/sparc/vm/globals_sparc.hpp | 2 +-
> src/cpu/x86/vm/globals_x86.hpp | 2 +-
> src/cpu/zero/vm/globals_zero.hpp | 3 +-
> src/os/aix/vm/globals_aix.hpp | 2 +-
> src/os/bsd/vm/globals_bsd.hpp | 29 +-
> src/os/linux/vm/globals_linux.hpp | 9 +-
> src/os/solaris/vm/globals_solaris.hpp | 4 +-
> src/os/windows/vm/globals_windows.hpp | 5 +-
> src/share/vm/c1/c1_globals.cpp | 4 +-
> src/share/vm/c1/c1_globals.hpp | 17 +-
> src/share/vm/gc_implementation/g1/g1_globals.cpp | 16 +-
> src/share/vm/gc_implementation/g1/g1_globals.hpp | 38 ++-
> src/share/vm/opto/c2_globals.cpp | 12 +-
> src/share/vm/opto/c2_globals.hpp | 39 ++-
> 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 | 242 +++++++++++++++++++++
> src/share/vm/runtime/commandLineFlagConstraintList.hpp | 72 ++++++
> 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/globals_extension.hpp | 101 +++++++-
> src/share/vm/runtime/init.cpp | 6 +-
> src/share/vm/runtime/os.hpp | 17 +
> src/share/vm/runtime/os_ext.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 | 4 +-
> test/gc/g1/TestStringDeduplicationTools.java | 6 +-
> test/runtime/CompressedOops/CompressedClassSpaceSize.java | 4 +-
> test/runtime/CompressedOops/ObjectAlignment.java | 9 +-
> test/runtime/contended/Options.java | 10 +-
> 48 files changed, 2641 insertions(+), 878 deletions(-)
>
More information about the hotspot-dev
mailing list