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

[Gerard Ziemski]

Thank you David for feedback. Please see my answers inline:

> -------- Forwarded Message --------
> Subject:	Re: Revision1: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments
> Date:	Tue, 26 May 2015 18:10:14 +1000
> From:	David Holmes<david.holmes at oracle.com>
> Organization:	Oracle Corporation
> To:	Gerard Ziemski<gerard.ziemski at oracle.com>,hotspot-dev at openjdk.java.net  Developers<hotspot-dev at openjdk.java.net>, Coleen Phillimore<coleen.phillimore at oracle.com>, Dmitry Dmitriev<dmitry.dmitriev at oracle.com>
> CC:	david.therkelsen at oracle.com, Thomas Stüfe<thomas.stuefe at gmail.com>, sangheon.kim<sangheon.kim at oracle.com>
>
> Hi Gerard,
>
> Progress review so I can send this out tonight - I still have to
> complete the review and double-check the responses to my previous comments.
>
> In globals.hpp, looking at all the "stack parameters" I expect to see a
> constraint function specified somewhere, but there isn't one. So now I'm
> a bit confused about how constraint functions are specified and used. If
> there has to be a relationship maintained between A, B and C, is the
> constraint function specified for all of them or none of them and simply
> executed as post argument processing step? Can you elaborate on when
> constraint functions may be used, and must be used, and how they are
> processed?
>
Constraints were not meant as a framework that imposes restrictions as to when and how to be used. It’s a helper framework that makes it easy for a developer to implement the kind of a constraint that a particular flag(s) demands. The decision as to what goes into it is left to the engineer responsible for a particular flag. The process of implementing constraints and ranges is still ongoing for many of the flags, and there are 3 subtasks tracking the issue. This webrev covers the introduction of the range/constraint framework and a subset of ranges/constraints implemented for those flags for which I was able to find existing ad hoc code or comments describing them.


> A few minor specific comments below:
>
> src/share/vm/c1/c1_globals.hpp
>
> Minor nit: Could you change:
>
>    range(1, NOT_LP64(K) LP64_ONLY(32*K))
>
> to
>
>    range(1, NOT_LP64(1*K) LP64_ONLY(32*K))
>
> or even:
>
>    range(1, NOT_LP64(1) LP64_ONLY(32) *K)
>
> I find the (K) by itself a little odd-looking.
>
Done.


> ---
>
> src/share/vm/runtime/globals.cpp
>
> +       if (withComments) {
> + #ifndef PRODUCT
> +         st->print("%s", _doc);
> + #endif
> +       }
>
> The ifdef should be around the whole if block (as it should for the
> existing code just before this change).

Done.


> ---
>
> src/share/vm/runtime/globals.hpp
>
> range(1, NOT_LP64(K) LP64_ONLY(M))
>
> 1*K and 1*M please.

Done.


> 1328   /* 8K is well beyond the reasonable HW cache line size, even with
> the   */\
>
> delete the end 'the'
>
> 1329   /* aggressive prefetching, while still leaving the room for
> segregating */\
>
> delete 'the’.

Done.


> Nit:
>
> 1981           range(ReferenceProcessor::DiscoveryPolicyMin,
>           \
> 1982           ReferenceProcessor::DiscoveryPolicyMax)
>           \
>
> 1982 should be indented so the Reference's line up.

Done.


> 2588
> range((intx)Arguments::get_min_number_of_compiler_threads(),      \
> 2589                 max_jint)
>           \
>
> Seems odd for a range to be expressed this way - seems more like a
> constraint. And get_min_number_of_compiler_threads doesn't really seem
> like an API for Arguments.
>
All ranges could be expressed as constraints. Ranges are just “trivial” constraint, which is most of them, and are supposed to be easy to use by the developer. get_min_number_of_compiler_threads() in this context of expressing a range is “constant”. I need that value; it used to be accessed from Arguments internally, so it did not need to be exposed before, but with the range/constraints factored out, I need to be able to get at the value.

I will be persenting webrev 2 shortly after I do build and some testing.


cheers