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

Gerard Ziemski gerard.ziemski at oracle.com
Mon Jun 1 21:24:40 UTC 2015


hi Bengt,

Thank you for taking the time to provide your feedback. My answers are 
provided in-line:

On 5/29/2015 1:35 AM, Bengt Rutisson wrote:
>
> Hi Gerard,
>
> Not a full review, but just a couple of questions.
>
> The new constraint methods added in commandLineFlagConstraintsGC.cpp 
> are all just implementations of the existing constraints for the GC 
> flags, right? Basically these checks are just moved from arguments.cpp 
> to the constraint methods, or have any new ones been added?

The constraints were moved, as well as many ranges, though about 60% of 
ranges are new and were implemented based on comments and/or their names 
(ie. percentage in name implies 0..100)


>
> All methods in commandLineFlagConstraintsGC.cpp and 
> commandLineFlagConstraintsCompiler.cpp start with this check:
>
> if ((CommandLineFlags::finishedInitializing() == true)
>
> Only the runtime flags, checked in 
> commandLineFlagConstraintsRuntime.cpp, are checked even when 
> initialization has not been completed. Why is it important to be able 
> to check the constraints even before we have finished initializing? 
> Wouldn't it be simpler to just call the constraint methods once after 
> we've reached CommandLineFlags::finishedInitializing()? Then all the 
> constraint methods wouldn't have to remember to check for that.

The ideal flow for the flags is "define --> check --> use", however, 
that flow does not represent all the flags. There are flags that are set 
and then used to set other flags and parts of VM before the final check 
and therefore require validation earlier, hence the need for the 
"finishedInitiliazing". Personally, I don't like the added complexity 
myself either, but I think this is the best we can do for now without 
making lots of other changes to the code.


>
> (BTW, it is error prone to use == for boolean values. Better to just 
> use "if (CommandLineFlags::finishedInitializing())" .)
>

Fixed.

I will be posting up a new webrev shortly.


cheers


More information about the hotspot-dev mailing list