RFR (M): 8112746: Followup to JDK-8059557 (JEP 245: Validate JVM Command-Line Flag Arguments)
gerard ziemski
gerard.ziemski at oracle.com
Thu Aug 13 16:13:06 UTC 2015
Thank you for the review. Comments in-line:
On 08/11/2015 01:57 PM, Kim Barrett wrote:
> On Aug 10, 2015, at 1:13 PM, gerard ziemski <gerard.ziemski at oracle.com> wrote:
>>
>> Here is a rev2 to rev3 diff that Kim requested: http://cr.openjdk.java.net/~gziemski/8112746_rev2_to_rev3/
>>
>> On 08/07/2015 11:36 AM, gerard ziemski wrote:
>>> References:
>>>
>>> JEP 245: https://bugs.openjdk.java.net/browse/JDK-8122937
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8112746
>>> webrev: http://cr.openjdk.java.net/~gziemski/8112746_rev3/
>
> Thanks for the incremental webrev.
>
> Generally looks good. I have a few changes to suggest, but I don't
> see any show-stoppers. I would be ok with deferring some or all of
> these suggestions, so that we can be done with this change set and
> unblock Sangheon's dependent work.
>
> *** Could be fixed now or later:
>
> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagRangeList.hpp
> src/share/vm/runtime/commandLineFlagRangeList.cpp
> class CommandLineError ...
>
> I think this isn't the right place for this class. I think a better
> place would be globals.[ch]pp, following class CommandLineFlags.
>
> I think such a change would eliminate the need for new #include of
> "runtime/commandLineFlagRangeList.hpp" in
> commandLineFlagConstraints{Compiler,GC,Runtime}.cpp, introduced in
> this latest revision.
>
> And of course, the definition of the print function should be moved to a
> corresponding location in globals.cpp.
Even better - I would propose that we factor out the flag manipulation
code out of globals.hpp/.cpp and leave those files to deal only with
macros, and move the rest of the code into its own
CommandLineFlags.hpp/.cpp file.
Filed JDK-8133564 (runtime) to track this and the other issues from below.
>
> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
> 58 static Flag::Error MaxPLABSizeBounds(const char* name, size_t value, bool verbose) {
> 59 #if INCLUDE_ALL_GCS
> 60 if ((UseConcMarkSweepGC || UseG1GC) && (value > PLAB::max_size())) {
> 61 CommandLineError::print(verbose,
> 62 "%s (" SIZE_FORMAT ") must be "
> 63 "less than ergonomic PLAB maximum size (" SIZE_FORMAT ")\n",
> 64 name, value, PLAB::min_size());
>
> "less than" => "less than or equal to" to be consistent with test.
I filed "GC" version of the follow-up issue as JDK-8133565 (gc), for the
GC team to own this.
>
> ------------------------------------------------------------------------------
> commandLineFlagConstraints{Compiler,GC,Runtime}.cpp
>
> With the refactoring for CommandLineError, I think these files no
> longer need to #include "utilities/defaultStream.hpp".
Will add to JDK-8133564 (runtime).
>
> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagConstraintList.cpp
> 307 bool CommandLineFlagConstraintList::check_constraints(CommandLineFlagConstraint::ConstraintType type) {
> ...
> 327 } else if (flag->is_int()) {
> 328 int value = flag->get_int();
> 329 if (constraint->apply_int(value, true) != Flag::SUCCESS) status = false;
> 330 } else if (flag->is_uint()) {
> 331 uint value = flag->get_uint();
> 332 if (constraint->apply_uint(value, true) != Flag::SUCCESS) status = false;
> ...
>
> [Latest version added missing cases.]
>
> Suggest adding a final else clause to the flag type dispatch that is a
> ShouldNotReachHere or some such.
>
> Similary in CommandLineFlagRangeList::check_ranges().
Will add to JDK-8133564 (runtime).
>
> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
> 71 static Flag::Error MinMaxPLABSizeBounds(const char* name, size_t value, bool verbose) {
> 72 if (MinPLABSizeBounds(name, value, verbose) == Flag::SUCCESS) {
> 73 return MaxPLABSizeBounds(name, value, verbose);
> 74 }
> 75 return Flag::VIOLATES_CONSTRAINT;
> 76 }
>
> I'd prefer this returned the result of MinPLABSizeBounds when that
> isn't SUCCESS, rather than discarding that result and returning
> VIOLATES_CONSTRAINT. I think there isn't presently a difference, but
> every time I look at this I need to verify that belief.
Will add to JDK-8133565 (gc).
>
> ------------------------------------------------------------------------------
>
> *** Should be fixed later:
>
> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagConstraintList.cpp
> 307 bool CommandLineFlagConstraintList::check_constraints(CommandLineFlagConstraint::ConstraintType type) {
> ...
> 319 Flag* flag = Flag::find_flag(name, strlen(name), true, true);
> 320 // We must check for NULL here as lp64_product flags on 32 bit architecture
> 321 // can generate constraint check (despite that they are declared as constants),
> 322 // but they will not be returned by Flag::find_flag()
> 323 if (flag != NULL) {
>
> This comment suggests to me that there is a bug in the underlying
> constraint registration infrastructure; why are we registering
> constraints for options that don't exist?
>
> Similary in CommandLineFlagRangeList::check_ranges().
>
> I think this should be addressed as a followup RFE, rather than as
> further work on this change set. It's a pre-existing problem, not
> something introduced by this change set.
>
Will add to JDK-8133564 (runtime).
> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
> 218 Flag::Error CMSPrecleanNumeratorConstraintFunc(uintx value, bool verbose) {
> 219 if (value > (CMSPrecleanDenominator - 1)) {
> 220 CommandLineError::print(verbose,
> 221 "CMSPrecleanNumerator (" UINTX_FORMAT ") must be "
> 222 "less than or equal to CMSPrecleanDenominator - 1 (" UINTX_FORMAT ")\n",
> 223 value, CMSPrecleanDenominator - 1);
>
> [pre-existing]
>
> Wouldn't this be simpler with (value >= CMSPrecleanDenominator) and
> just "less than" (or "strictly less than" for consistency with wording
> elsewhere; same wording should be used in all such places, but I don't
> care which).
>
> This can be handled as a separate (small) cleanup; maybe give it to
> Sangheon to deal with among the other GC-related option processing
> improvements.
Will add to JDK-8133565 (gc).
Coleen, Kim, would one of you kindly sponsor this code for checking-in?
I will prepare an hg diff that passes "hg jcheck". Thank you.
cheers
More information about the hotspot-dev
mailing list