Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments
David Holmes
david.holmes at oracle.com
Mon May 18 05:28:00 UTC 2015
Hi Gerard,
I did a lengthy but still preliminary walk through of this code. We need
a lot of eyes on this. :)
On 15/05/2015 5:57 AM, Gerard Ziemski wrote:
> hi all,
>
> 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)
That was a lot to digest, though many of the changes are syntactic
adjustments (check for success versus zero etc).
But overall this looks really good (macro magic notwithstanding :) ). I
like the way you handled the constraints, and it's nice to see the
constraint functions in with the related code rather than being
expressed in a checking function in arguments.cpp (though see comments
for cases where this does not occur).
A few comments:
For constructs like:
void emit_range_bool(const char* name) { (void)name; /* NOP */ }
I seem to recall that this was not sufficient to avoid "unused" warnings
with some compilers - which is presumably why you did it?
---
In src/share/vm/runtime/globals.hpp it says:
see "checkRanges" function in arguments.cpp
but there is no such function in arguments.cpp
---
src/share/vm/runtime/arguments.cpp
The set_object_alignment() functions seems to have some redundant
constraint assertions. As does verify_object_alignment() - seems to me
that everything in verify_object_alignment should either be in the
constraint function for ObjectAlignmentInBytes or one for
SurvivorAlignmentInBytes - though the combination of verification and
the actual setting of SurvivorAlignmentInBytes may be a problem in the
new architecture. If you can't get rid of verify_object_alignment() I'd
be tempted to not process it the new way at all, as splitting the
constraint checking just leads to confusion IMO.
The changes to test/runtime/contended/Options.java suggest to me that a
constraint is missing on ContendedPaddingWidth - that it is a multiple
of 8 (BytesPerLong). That is (still) checked in arguments.cpp (and given
it is still checked there is no need to have removed it from the test).
---
Some of the test changes, such as:
test/gc/g1/TestStringDeduplicationTools.java
seem to be losing some of what they test. Not only is the test checking
the value is detected as erroneous, but it also detects that the user is
told in what way it is erroneous. The updated test doesn't validate that
part of the argument processing logic
----
There is some inconsistency in the test changes, sometimes you use:
shouldContain("outside the allowed range")
and sometimes:
shouldContain("is outside the allowed range")
That's all for now.
Thanks,
David
-----
>
> References:
>
> Webrev: http://cr.openjdk.java.net/~gziemski/8059557_rev0/
> 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 | 4 +-
> 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 | 711
> +++++++++++++++++++++++++++------------------------------------
> 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 | 226
> ++++++++++++++++++++
> src/share/vm/runtime/commandLineFlagConstraintsGC.hpp | 57 +++++
> src/share/vm/runtime/commandLineFlagConstraintsRuntime.cpp | 50 ++++
> src/share/vm/runtime/commandLineFlagConstraintsRuntime.hpp | 39 +++
> src/share/vm/runtime/commandLineFlagRangeList.cpp | 304
> +++++++++++++++++++++++++++
> src/share/vm/runtime/commandLineFlagRangeList.hpp | 67 ++++++
> src/share/vm/runtime/globals.cpp | 681
> ++++++++++++++++++++++++++++++++++++++++++++++++------------
> src/share/vm/runtime/globals.hpp | 302
> +++++++++++++++++++++-----
> 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/g1/TestStringDeduplicationTools.java | 8 +-
> test/runtime/CompressedOops/CompressedClassSpaceSize.java | 4 +-
> test/runtime/CompressedOops/ObjectAlignment.java | 9 +-
> test/runtime/contended/Options.java | 12 +-
> 47 files changed, 2572 insertions(+), 835 deletions(-)
>
>
>
>
More information about the hotspot-dev
mailing list