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

Wed Jun 3 19:26:14 UTC 2015

Thank you Kim, for more feedback. Please my comments in-line:

> Subject:	Re: Revision2: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments
> Date:	Tue, 2 Jun 2015 18:52:40 -0400
> From:	Kim Barrett<kim.barrett at oracle.com>
> To:	Gerard Ziemski<gerard.ziemski at oracle.com>
> CC:	hotspot-dev at openjdk.java.net  Developers<hotspot-dev at openjdk.java.net>, David Holmes<david.holmes at oracle.com>, Coleen Phillimore<coleen.phillimore at oracle.com>, Dmitry Dmitriev<dmitry.dmitriev at oracle.com>, Alexander Harlap<alexander.harlap at oracle.com>
>
> On May 27, 2015, at 5:28 PM, Gerard Ziemski<gerard.ziemski at oracle.com>
>   wrote:
>> hi all,
>>
>> Here is a revision 2 of the feature taking into account feedback from Dmitry, David, Kim and Alexander.
>> ...
> Here's another collection of comments.  I haven't looked at the actual
> ranges and constraints yet.  I might leave off that, since others seem
> to be looking there.  Let me know if you think another set of eyes is
> needed there.
>
Given the breath of issues your review brings up, having you go over those would be very helpful - just please wait until rev 3.

> ------------------------------------------------------------------------------
> src/share/vm/runtime/globals.cpp
> 1067 //#define PRINT_FLAGS_SORTED_BY_TYPE_THEN_NAMES
> and following
>
> The PRINT_FLAGS_SORTED_BYTE_TYPE_THEN_NAMES is never defined, so the
> associated #else clause is (presently) dead code.  If it were live
> code then I'd have several complaints about it.  But since it appears
> to be dead, I'll limit my complaint to it existing at all.

I will keep it simple and just remove it.

> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagRangeList.cpp
>
> There is a large amount of code near-duplication among the various
> CommandLineFlagRange_<type> classes.  I think this could be
> substantially reduced via some fairly straightforward usage of macros
> and templates.  This can be dealt with in a follow-on cleanup - please
> file a CR.
>
> Probably similarly for the constraint classes.

Filed as JDK-8081833. Could you please add detail there?

> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagConstraintList.cpp
> src/share/vm/runtime/commandLineFlagRangeList.cpp
>
> The macros EMIT_CONSTRAINT_COMMERCIAL_FLAG and
> EMIT_RANGE_COMMERCIAL_FLAG are defined in these files.  This is
> parhaps the wrong place for them, since they are unused in open code.
>
> If moving the commercial flag handlers, it might be desirable to
> define a helper macro for use by all the EMIT_RANGE_xxx_FLAG (and
> similarly for EMIT_COMMERCIAL_xxx_FLAG).

Those macros are not revealing anything - do we really need to move them?

If we decide we absolutely must move them and hide behind a level of extra macros, would you kindly elaborate (in private if it needs to be) how to do so?

> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagConstraintList.cpp
> src/share/vm/runtime/commandLineFlagRangeList.cpp
>
> The classes CommandLineFlag{Constraint,Range}List are derived from
> CHeapObj<mtInternal>, but appear to never be allocated, and only have
> static members.  I think they should instead be derived from
> AllStatic.

Fixed.

> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagRangeList.cpp
>
> The various emit_range_xxx functions are defined with global scope.
> I*think*  they should be defined at file scope.
>
> Similarly for the constraint list functions.

If I make them at file scope then the compiler complains that some methods are not used, so I would have to remove those corresponding methods whose flag type is not currently used yet in our macro tables. But then in a future, when we add constraint or a range to a flag, whose type has not been used yet, we will have to implement those methods, which might be confusing to whoever it is that happens to be doing that.

I think we would be better off by leaving things here implemented as is and prepared for any new ranges/constraints flags.

> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagRangeList.cpp
> src/share/vm/runtime/commandLineFlagRangeList.cpp
>
> These files directly include runtime/os_ext.hpp, but that file is not
> intended to be directly included, but must be included from the
> *middle*  of runtime/os.hpp.  I think the only reason this include
> isn't blowing up is that there is some earlier include of
> runtime/os.hpp, so that the direct include of os_ext.hpp is suppressed
> by its include guard.  It's not obvious what earlier file might be
> providing that include of os.hpp, and I wonder if perhaps it's coming
> from the precompiled headers.

Changed it to “runtime/os.hpp” and all seems well.

>   Which reminds me, has this changeset
> been tested without precompiled headers?
>
Yes, I have explicitly tested it out by building without precompiled headers (on Mac OS X)

> That include of os_ext.hpp is to obtain the definitions of the macros
> EMIT_{CONSTRAINTS,RANGES}_FOR_GLOBALS_EXT.  I think those macro
> definitions probably ought to be moved to a different place, though I
> don't have a suggested landing place.

Filed JDK-8081842 to follow up on this.

> ------------------------------------------------------------------------------
> src/share/vm/services/writeableFlags.cpp
>
> 34 #define TEMP_BUF_SIZE 256
> ...
> 61 static void print_flag_error_message_if_needed(Flag::Error error, const char* name, FormatBuffer<80>& err_msg) {
>
> The ultimate destination of the error message is err_msg, which is
> explicitly limited to 80 characters.  It seems kind of pointless to
> make TEMP_BUF_SIZE larger than the err_msg size.

I need large enough buffer to hold the raw range string, which I then “compress” by removing white spaces, so that it fits into the 80 character error buffer.

> ------------------------------------------------------------------------------
> src/share/vm/runtime/globals.cpp
>
> In the various CommandLineFlags::<type>AtPut(const char* name, ...)
> functions, the call to the associated
> apply_constraint_and_check_range_<type> helper function uses
>    (CommandLineFlags::finishedInitializing() == false)
> to compute the verbose argument in that helper function call.  Better
> would be simply
>    !CommandLineFlags::finishedInitializing()
>
Done.

> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagRangeList.hpp
> 42 public:
> 43  const char* _name;
>
> _name should be private.  Any external accesses should be via the
> existing public name() member function.
>
> Similarly for CommandLineFlagConstraint.

Fixed.

> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagRangeList.hpp
> 44  CommandLineFlagRange(const char* name) { _name=name; }
>
> This will result in a dangling pointer if the name argument can be
> deallocated.  I suspect the intent is that this is only called with a
> string literal, and that all callers do so, but that's hard to ensure.
>
> Similarly for CommandLineFlagConstraint.
>
> I'm not sure there's anything to do here, other than audit and add
> comments.

The name arguments for the constructors come from macro tables and are string literals.

> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagRangeList.hpp
> 47   virtual Flag::Error check_intx(intx value, bool verbose = true) { return Flag::SUCCESS; }
> ... and other similar functions ...
> ... and similar functions in CommandLineFlagConstraint ...
>
> I'm dubious about default implementations returning success.  I think
> the default should be a failure indication, possibly even with
> ShouldNotReachHere(), since it indicates an attempt to apply a
> constraint for the wrong type of value.  If the constraint were being
> applied to the correct type of value, the default implementation would
> be overridden by the derived class of the constraint object.
>
> Similarly for CommandLineFlagConstraint.

I’m just mimicking the pre-existing behavior, which was with no checking everything was passing. The way I see it, we pass unless we prove we violate constraint or range.

> ------------------------------------------------------------------------------
>
> If I'm understanding correctly, there should now never be a call to
> FLAG_SET_{CMDLINE,DEFAULT,ERGO,other?} that doesn't have its result
> checked.  But most (by a substantial majority) appear to be unchecked.
> I'm guessing that's a followup task?  I don't see any mention of
> FLAG_SET_xxx checking in the review summary; it's a different task
> from determining appropriate ranges for flags that haven't had that
> done yet.

Actually, most FLAG_SET_CMDLINE uses check the return values, except for a few cases where a flag is deprecated, or some used in some unit tests, that I left up to the team responsible.

I assumed that the uses of FLAG_SET_DEFAULT and FLAG_SET_ERGO know what that are doing, but if it’s a wrong assumption, then we can add error check there too later.

This is an initial effort of introducing the new mechanism in. There are already subtasks covering pending tasks such as implementing those flags that don’t have ranges yet, so this checkin doesn’t represent the final effort.

> ------------------------------------------------------------------------------
> src/share/vm/runtime/globals.cpp
> 331  if (printRanges == false) {
>
> I'd prefer "if (!printRanges) {".
>
> Similarly for
>
> 380  } else if ((is_bool() == false) && (is_ccstr() == false)) {
> 381
> 381    if (printRanges == true) {
>
> Searching for " == true" and " == false" will probably find more.
>
I must say that personally I prefer to see the value of the comparison shown explicitly, but I understand that in cases of “true” and “false”, it may lead to bugs.

Fixed.

Thank you Kim for excellent feedback and I will be posting Revision 3 soon.

cheers