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

[Gerard Ziemski]

hi Derek,

I answered your questions below:


On 6/3/2015 4:29 PM, Derek White wrote:
> 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.

Excellent catch, fixed.


>>> 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.

Ah, now I see what we're talking about - thank you for clarifying.

Fixed with ShouldNotReachHere()

I will be posting a new webrev shortly, after I do testing.


cheers