Revision3: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments
Coleen Phillimore
coleen.phillimore at oracle.com
Tue Jun 23 01:16:57 UTC 2015
Hi Gerard,
I realized that I didn't send my code review to the public alias. None
of these comments prevented checking it in but hopefully you fixed the
first one at least, and the rest are RFEs.
Thanks,
Coleen
On 6/15/15 7:46 PM, Coleen Phillimore wrote:
>
> Hi Gerard,
>
> I have read through the code again and the code still looks good to
> me. I had a couple of questions, which you could either answer "no"
> or have in a follow-up RFE.
>
> http://cr.openjdk.java.net/~gziemski/8059557_rev3/src/share/vm/runtime/commandLineFlagRangeList.cpp.html
> 87 st->print("[ "INTX_FORMAT_W(-25)" ... "INTX_FORMAT_W(25)" ]", _min, _max);
> In some C++15 code the lack of space between INTX_FORMAT and the
> string is an error, can you fix these? I thought you had this comment
> already but either I have an old webrev or you missed this one.
>
> http://cr.openjdk.java.net/~gziemski/8059557_rev3/src/share/vm/runtime/commandLineFlagConstraintsCompiler.cpp.html
> 31 Flag::Error AliasLevelConstraintFunc(bool verbose, intx* value) {
> In the constraint functions, the value parameter isn't set by the
> function. Why is it a pointer rather than a value? Is this required
> for instantiating the macro for the constraint functions? If so,
> could you make the pointers const or would that also not instantiate
> correctly with the macro?
>
> http://cr.openjdk.java.net/~gziemski/8059557_rev3/src/share/vm/runtime/commandLineFlagConstraintsRuntime.cpp.html
> 33 if (verbose == true) {
> 34 jio_fprintf(defaultStream::error_stream(),
> 35 "ObjectAlignmentInBytes=" INTX_FORMAT " must be power of 2\n",
> 36 *value);
> 37 }
>
> It seems like a printing function like
> print_command_line_error(verbose, const char* fmt, ...) would be nice
> here since this code pattern exists in a lot of places and it'll catch
> someone from forgetting the if (verbose) check.
>
> For booleans, the style is if (verbose) rather than if (verbose ==
> true), which I think people have pointed out. If the thing in the
> conditional is a pointer, it should be explicitly checked for NULL:
> if (ptr == NULL) or ideally if (NULL == ptr).
>
> This code looks ready to go except some issues that you've filed RFEs
> for. We could look at this for months and still find items that are
> good to change, but as a significant piece of work and in my opinion
> the cleanups are small enough to do later. This follows our coding
> standards for consistency and quality, and you've done an excellent
> job answering people's comments and making sure that the tests run and
> there are no regressions.
>
> Thank you!
> Coleen
>
>
> On 6/13/15 10: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