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

[Kim Barrett]

On Jun 3, 2015, at 5:29 PM, Derek White <derek.white at oracle.com> wrote:
> 
>> - 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.


Yes, this is what I was talking about, and I agree with Derek, except I would go further an make these default implementations be ShouldNotReachHere(), because we shouldn’t be reaching there...