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

[Derek White]

Hi Gerard,

Thanks for the explanations. Just quoting still-open issues below:

On 6/2/15 4:09 PM, Gerard Ziemski wrote:
> Thank you Derek very much for your feedback. Please see my answers inline:
>
>> Line 2036, 2041:
>>    - Set status to false instead of returning directly?
> I’m sorry, which file do those lines refer to?
In arguments.cpp, in bool Arguments::check_vm_args_consistency(). My 
point is that the pre-existing control flow was to set  "status = false" 
and continue checking flags, not to bail out when the first failure 
occurred.

>> Line 2827 (existing); Change "K" -> "1*K" for consistency.
> Line 2827 in gobals.hpp or is it a different file?

Never mind. I should have been talking about arguments.cpp, but I must 
have miss-typed the line #, and now I can't find it. There are actually 
lots of naked "K"s around, so even though I'm not a fan, I can't 
complain about consistency.

> - Shouldn't the default implementations for 
> CommandLineFlagConstraint::apply_XXX() return failure, not success?
> I looked at it from the point of view of the more likely scenario, which is assume success, unless a fault detected.
>
> If we did do this, though, then instead of:
>
> Flag::Error MinHeapFreeRatioConstraintFunc(bool verbose, uintx* value) {
>    if ((CommandLineFlags::finishedInitializing()) && (*value > MaxHeapFreeRatio)) {
>      if (verbose == true) {
>        jio_fprintf(defaultStream::error_stream(),
>                    "MinHeapFreeRatio (" UINTX_FORMAT ") must be less than or "
>                    "equal to MaxHeapFreeRatio (" UINTX_FORMAT ")\n",
>                    *value, MaxHeapFreeRatio);
>      }
>      return Flag::VIOLATES_CONSTRAINT;
>    } else {
>      return Flag::SUCCESS;
>    }
> }
>
> we’d have:
>
> Flag::Error MinHeapFreeRatioConstraintFunc(bool verbose, uintx* value) {
>    Flag::Error error = Flag::VIOLATES_CONSTRAINT;
>    if ((CommandLineFlags::finishedInitializing()) && (*value > MaxHeapFreeRatio)) {
>      if (verbose == true) {
>        jio_fprintf(defaultStream::error_stream(),
>                    "MinHeapFreeRatio (" UINTX_FORMAT ") must be less than or "
>                    "equal to MaxHeapFreeRatio (" UINTX_FORMAT ")\n",
>                    *value, MaxHeapFreeRatio);
>      }
>    } else {
>      error = Flag::SUCCESS;
>    }
>    return error;
> }
>
> which is not as nice, in my opinion.
I'm not sure we're talking about the same thing.

Looking at commandLineFlagConstraintList.hpp:

   55   virtual Flag::Error apply_bool(bool* value, bool verbose = true) { return Flag::SUCCESS; };

I had the question "What happens if someone calls a version of the apply 
function (apply_FOO) that doesn't match the type of the constraint?"

For example, look at CommandLineFlagConstraint::apply_bool(). What if 
the flag is really an "int", and it has a constraint? So the constraint 
is of type CommandLineFlagConstraint_intx. If apply_bool() is ever 
called on a CommandLineFlagConstraint_intx it will return Flag::SUCCESS. 
This is wrong - it violates the implicit constraint that the type of the 
value matches the type of the flag.

It looks like the callers ensure that constraint's type matches the 
value's type, so the default apply_bool() is never called. I'm not sure 
why you'd have to re-write MinHeapFreeRatioConstraint() (as describe 
above) no matter what the default value of apply_bool() is.

But if some later code forgets to match the types correctly, I think 
we're better off calling shouldNotReachHere() in the default methods.

I think Kim had a similar question about default check_FOO() methods in 
commandLineFlagRangeList.hpp.

   47   virtual Flag::Error check_intx(intx value, bool verbose = true) { return Flag::SUCCESS; }
   48   virtual Flag::Error check_uintx(uintx value, bool verbose = true) { return Flag::SUCCESS; }
   49   virtual Flag::Error check_uint64_t(uint64_t value, bool verbose = true) { return Flag::SUCCESS; }
   50   virtual Flag::Error check_size_t(size_t value, bool verbose = true) { return Flag::SUCCESS; }
   51   virtual Flag::Error check_double(double value, bool verbose = true) { return Flag::SUCCESS; }

Thanks for reading this far!

  - Derek

Notes:

    apply_bool() is called in two places.

    1) apply_constraint_and_check_range_bool()
         - Do the callers do a type check?
             - Yes. On type mismatch:
                - CommandLineFlags::boolAtPut(const char* name, size_t
    len, bool* value, Flag::Flags origin) returns  Flag::WRONG_FORMAT.
                - CommandLineFlagsEx::boolAtPut(CommandLineFlagWithType
    flag, bool value, Flag::Flags origin) will fail a "guarantee". Not
    sure if that's best idea.

    2) CommandLineFlags::check_all_ranges_and_constraints()
            - Caller basically does a type case, so apply_bool() is
    never called with wrong type flag.

   55   virtual Flag::Error apply_bool(bool* value, bool verbose = true) { return Flag::SUCCESS; };
   56   virtual Flag::Error apply_intx(intx* value, bool verbose = true) { return Flag::SUCCESS; };
   57   virtual Flag::Error apply_uintx(uintx* value, bool verbose = true) { return Flag::SUCCESS; };
   58   virtual Flag::Error apply_uint64_t(uint64_t* value, bool verbose = true) { return Flag::SUCCESS; };
   59   virtual Flag::Error apply_size_t(size_t* value, bool verbose = true) { return Flag::SUCCESS; };
   60   virtual Flag::Error apply_double(double* value, bool verbose = true) { return Flag::SUCCESS;

These are never called (because the callers ensure that constraint's 
type matches the "value" type), so it shouldn't matter if they return 
Flag::SUCCESS or Flag::INVALID_FLAG.