Revision2: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments
David Holmes
david.holmes at oracle.com
Fri May 29 04:54:06 UTC 2015
Missed the tests:
test/runtime/CompressedOops/ObjectAlignment.java
testObjectAlignment(-1)
! .shouldContain("must be power of 2")
! .shouldContain("outside the allowed range")
I'm missing how it can fail for both reasons - won't the range error
stop the constraint check from even being applied?
---
test/runtime/contended/Options.java
output.shouldContain("ContendedPaddingWidth");
! output.shouldContain("outside the allowed range");
output.shouldContain("must be a multiple of 8");
Again with the new scheme the range check should prevent the constraint
check, so I don't see how the error message can report both errors ??
David
-----
On 29/05/2015 2:39 PM, David Holmes wrote:
> Hi Gerard,
>
> Meta-comment: I had expected to see constraint functions subsume range
> checks - ie that the range check would be folded into the constraints
> function so that all the constraints are clearly seen in one place. Not
> sure splitting things across two checks aids in understandability. This
> isn't a blocker but I'd be interested to hear other views on this.
>
>
> A general comment, note that in cases like:
>
> "intx %s="INTX_FORMAT" is outside the allowed range [ "INTX_FORMAT"
> ... "INTX_FORMAT" ]\n",
>
> we now need to add spaces between the macros like INTX_FRORMAT and the
> double-quotes - see 8081202. Not sure who will be pushing first but it's
> an easy fix to make.
>
> A few minor specific comments:
>
> src/share/vm/runtime/commandLineFlagConstraintsCompiler.hpp
>
> Has the comment:
>
> 32 * Here we have runtime arguments constraints functions,
>
> - should say 'compiler'
>
> ---
>
> src/share/vm/runtime/commandLineFlagConstraintsCompiler.cpp
>
> Not obvious all the includes are needed ie java.hpp and os.hpp
>
> ---
>
> src/share/vm/runtime/commandLineFlagConstraintsGC.hpp
>
> Has the comment:
>
> 32 * Here we have runtime arguments constraints functions,
>
> - should say 'GC'
>
> I'm a little surprised the #if INCLUDE_ALL_GCS only covers the G1
> options. I guess we don't as aggressively exclude the code for the other
> GC's.
>
> ---
> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
>
> Not obvious all the includes are needed ie java.hpp and os.hpp, and
> c1/c2 globals?
>
> ---
>
> Nothing else jumped out at me, but I didn't verify the non-runtime
> constraints.
>
> Thanks,
> David
>
> On 28/05/2015 7:28 AM, 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. 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_rev2
>> 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 | 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/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 | 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(-)
>>
More information about the hotspot-dev
mailing list