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

Gerard Ziemski gerard.ziemski at oracle.com
Wed Jun 17 15:03:06 UTC 2015 

Thank you Dmitry!

On 6/17/2015 6:14 AM, Dmitry Dmitriev wrote:
> Hi Gerard,
>
> Changes looks good!
>
> Regards,
> Dmitry
>
> On 17.06.2015 0:11, 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?
>>
>>
>> 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