Revision3: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments

David Holmes david.holmes at oracle.com
Wed Jun 17 03:56:40 UTC 2015


Hi Gerard,

On 17/06/2015 7:11 AM, Gerard Ziemski wrote:
> Coleen, Dmitry, David, Kim, Derek and Bengt,
>
> I have create an incremental webrev as requested:
> http://cr.openjdk.java.net/~gziemski/8059557_rev2_to_rev3
>
> As per suggestion from Kim and Coleen, I will not be spinning webrev4
> unless there is a blocker in the code that we haven't found yet.
>
> The plan right now is to check in rev3 and have
> https://bugs.openjdk.java.net/browse/JDK-8112746 track any follow up
> issues, which I already populated with comments from Coleen, Dmitry and
> Kim.
>
> Can I have a final sign off on this webrev rev3 from all of you please?

Other than the D->%d issue Dmitry spotted I have no further comments.

Thanks,
David

>
> cheers
>
> On 6/14/2015 9:31 PM, David Holmes wrote:
>> Hi Gerard,
>>
>> Any chance you could produce and incremental webrev please?
>>
>> Thanks,
>> David
>>
>> On 14/06/2015 12:32 AM, Gerard Ziemski wrote:
>>> hi all,
>>>
>>> Thank you for the reviews so far!
>>>
>>> Here is a revision 3 of the feature taking into account feedback from
>>> Kim Barrett, David Holmes, Derek White and Bengt Rutisson. I have
>>> also merged the changes to the latest jdk9.
>>>
>>> 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 mechanism 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_rev3
>>> 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
>>>
>>> Note: due to "awk" limit of 50 pats the Frames diff is not available
>>> for "src/share/vm/runtime/arguments.cpp” and
>>> "src/share/vm/runtime/globals.cpp”
>>>
>>>
>>> Followup issues:
>>>
>>> JDK-8085866: There are 99 uses of FLAG_SET_ERGO, should they check
>>> the return value?
>>> JDK-8085864: FLAG_SET_CMDLINE in TestGenCollectorPolicy() currently
>>> ignore the return values
>>> JDK-8081842: Find a better place for
>>> EMIT_{CONSTRAINTS,RANGES}_FOR_GLOBALS_EXT
>>> JDK-8081833: There is a large amount of code near-duplication among
>>> the various CommandLineFlagRange_<type> classes
>>> JDK-8081519: Split globals.hpp to factor out the Flag values needed
>>> by JDK-8059557
>>>
>>>
>>> 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/g1/g1_globals.cpp |   14 +-
>>>   src/share/vm/gc/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                          | 765
>>> ++++++----------
>>>   src/share/vm/runtime/arguments.hpp |   24 +-
>>>   src/share/vm/runtime/commandLineFlagConstraintList.cpp      | 286
>>> ++++++
>>>   src/share/vm/runtime/commandLineFlagConstraintList.hpp |   80 +
>>>   src/share/vm/runtime/commandLineFlagConstraintsCompiler.cpp |   44 +
>>>   src/share/vm/runtime/commandLineFlagConstraintsCompiler.hpp |   39 +
>>>   src/share/vm/runtime/commandLineFlagConstraintsGC.cpp       | 235
>>> +++++
>>>   src/share/vm/runtime/commandLineFlagConstraintsGC.hpp |   58 +
>>>   src/share/vm/runtime/commandLineFlagConstraintsRuntime.cpp |   65 +
>>>   src/share/vm/runtime/commandLineFlagConstraintsRuntime.hpp |   41 +
>>>   src/share/vm/runtime/commandLineFlagRangeList.cpp           | 367
>>> ++++++++
>>>   src/share/vm/runtime/commandLineFlagRangeList.hpp |   71 +
>>>   src/share/vm/runtime/globals.cpp                            | 721
>>> ++++++++++++---
>>>   src/share/vm/runtime/globals.hpp                            | 343
>>> ++++++-
>>>   src/share/vm/runtime/globals_extension.hpp                  | 105 +-
>>>   src/share/vm/runtime/init.cpp |    5 +-
>>>   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 |   12 +-
>>>   src/share/vm/services/diagnosticCommand.cpp |    3 +-
>>>   src/share/vm/services/management.cpp |    6 +-
>>>   src/share/vm/services/memoryService.cpp |    4 +-
>>>   src/share/vm/services/writeableFlags.cpp                    | 183 ++-
>>>   src/share/vm/services/writeableFlags.hpp |   60 +-
>>>   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 +-
>>>   49 files changed, 2851 insertions(+), 943 deletions(-)
>>>
>>
>


More information about the hotspot-dev mailing list