Revision1: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments
David Holmes
david.holmes at oracle.com
Tue May 26 08:10:14 UTC 2015
Hi Gerard,
Progress review so I can send this out tonight - I still have to
complete the review and double-check the responses to my previous comments.
In globals.hpp, looking at all the "stack parameters" I expect to see a
constraint function specified somewhere, but there isn't one. So now I'm
a bit confused about how constraint functions are specified and used. If
there has to be a relationship maintained between A, B and C, is the
constraint function specified for all of them or none of them and simply
executed as post argument processing step? Can you elaborate on when
constraint functions may be used, and must be used, and how they are
processed?
A few minor specific comments below:
src/share/vm/c1/c1_globals.hpp
Minor nit: Could you change:
range(1, NOT_LP64(K) LP64_ONLY(32*K))
to
range(1, NOT_LP64(1*K) LP64_ONLY(32*K))
or even:
range(1, NOT_LP64(1) LP64_ONLY(32) *K)
I find the (K) by itself a little odd-looking.
---
src/share/vm/runtime/globals.cpp
+ if (withComments) {
+ #ifndef PRODUCT
+ st->print("%s", _doc);
+ #endif
+ }
The ifdef should be around the whole if block (as it should for the
existing code just before this change).
---
src/share/vm/runtime/globals.hpp
range(1, NOT_LP64(K) LP64_ONLY(M))
1*K and 1*M please.
1328 /* 8K is well beyond the reasonable HW cache line size, even with
the */\
delete the end 'the'
1329 /* aggressive prefetching, while still leaving the room for
segregating */\
delete 'the'.
Nit:
1981 range(ReferenceProcessor::DiscoveryPolicyMin,
\
1982 ReferenceProcessor::DiscoveryPolicyMax)
\
1982 should be indented so the Reference's line up.
2588
range((intx)Arguments::get_min_number_of_compiler_threads(), \
2589 max_jint)
\
Seems odd for a range to be expressed this way - seems more like a
constraint. And get_min_number_of_compiler_threads doesn't really seem
like an API for Arguments.
That's all for now.
Thanks,
David
-----
On 22/05/2015 1:56 AM, 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