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

[Gerard Ziemski]

Thank you Dmitry for the corrections.

I have fixed the string format issues you pointed out in your other email, and responded to the other issues inline here:


> On May 26, 2015, at 11:30 AM, Gerard Ziemski<gerard.ziemski at oracle.com>  wrote:
>
>
>
>
> -------- Forwarded Message --------
> Subject:	Re: Revision1: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments
> Date:	Sun, 24 May 2015 23:52:29 +0300
> From:	Dmitry Dmitriev<dmitry.dmitriev 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>, David Holmes<david.holmes at oracle.com>, Coleen Phillimore<coleen.phillimore 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,
>
> I have found new several issues. Here my comments for 2 modules:
>
> 1) In src/share/vm/runtime/commandLineFlagConstraintsGC.cpp:
>   156 Flag::Error G1MaxNewSizePercentConstraintFunc(bool verbose, uintx* value) {
>   157   if ((CommandLineFlags::finishedInitializing() == true) && (*value < G1NewSizePercent)) {
>   158     if (verbose == true) {
>   159       jio_fprintf(defaultStream::error_stream(),
>   160                   "G1MaxNewSizePercent (" UINTX_FORMAT ") must be less than or "
>   161                   "equal to G1NewSizePercent (" UINTX_FORMAT ")\n",
>   162                   *value, G1NewSizePercent);
>   163     }
>
> Message on line 160 must state that G1MaxNewSizePercent must be greater than G1NewSizePercent.

Done.


>   186 Flag::Error CMSOldPLABMaxConstraintFunc(bool verbose, size_t* value) {
>   187   if ((CommandLineFlags::finishedInitializing() == true) && (*value < CMSOldPLABMax)) {
>   188     if (verbose == true) {
>   189       jio_fprintf(defaultStream::error_stream(),
>   190                   "CMSOldPLABMax (" SIZE_FORMAT ") must be greater than or "
>   191                   "equal to CMSOldPLABMax (" SIZE_FORMAT ")\n",
>   192                   *value, CMSOldPLABMax);
>   193     }
>   194     return Flag::VIOLATES_CONSTRAINT;
>
> It seems that this function perform wrong check. It verifies value for CMSOldPLABMax and compare it against CMSOldPLABMax. I think that it should be compared against CMSOldPLABMin. In this case error message should be corrected.

This constraint implements the previous ad hoc code logic:

   status = status && verify_min_value(CMSOldPLABMax, 1, "CMSOldPLABMax");

I think the desire here was to limit CMSOldPLABMax to be between 1 and the default value (the current CMSOldPLABMax value).


>   228 Flag::Error SurvivorAlignmentInBytesConstraintFunc(bool verbose, intx* value) {
>   229   if (CommandLineFlags::finishedInitializing() == true) {
>   230     if (*value != 0) {
>   231       if (!is_power_of_2(*value)) {
>   232         if (verbose == true) {
>   233           jio_fprintf(defaultStream::error_stream(),
>   234                     "SurvivorAlignmentInBytes (" INTX_FORMAT ") must be power of 2\n",
>   235                     *value);
>   236         }
>   237         return Flag::VIOLATES_CONSTRAINT;
>   238       }
>   239       if (SurvivorAlignmentInBytes < ObjectAlignmentInBytes) {
>   240         if (verbose == true) {
>   241           jio_fprintf(defaultStream::error_stream(),
>   242                     "SurvivorAlignmentInBytes (" INTX_FORMAT ") must be greater "
>   243                     "than ObjectAlignmentInBytes (" INTX_FORMAT ") \n",
>   244                     *value, ObjectAlignmentInBytes);
>   245         }
>   246         return Flag::VIOLATES_CONSTRAINT;
>   247       }
>
> On line 239 "*value" should be instead of "SurvivorAlignmentInBytes”.

Done.


> 2) In src/share/vm/runtime/commandLineFlagConstraintsRuntime.cpp:
>    33 Flag::Error ObjectAlignmentInBytesConstraintFunc(bool verbose, intx* value) {
>    34   if (!is_power_of_2(*value)) {
>    35     if (verbose == true) {
>    36       jio_fprintf(defaultStream::error_stream(),
>    37                 "ObjectAlignmentInBytes=%d must be power of 2\n",
>    38                 (int)*value);
>    39     }
>    40     return Flag::VIOLATES_CONSTRAINT;
>    41   }
>    42   // In case page size is very small.
>    43   if ((int)*value >= os::vm_page_size()) {
>    44     if (verbose == true) {
>    45       jio_fprintf(defaultStream::error_stream(),
>    46                 "ObjectAlignmentInBytes=%d must be less than page size %d\n",
>    47                 (int)*value, os::vm_page_size());
>    48     }
>
> I understand that ObjectAlignmentInBytesConstraintFunc have not huge upper range and in this code not introduce problems, so it can be leaved as is. I think that on lines 37-38 and 46 it is unnecessary to convert "*value" to "int", because instead of "%d" format you can use INTX_FORMAT. Also, on line 43 os::vm_page_size can be converted to wide type(from int to intx) instead of converting *value to narrow type(on 64 bit systems from intx to int), i.e. use following compare statement (*value >= (intx)os::vm_page_size()).

I copied the old ad hoc code "as is”, but we can tighten it up a bit.

I will be presenting web rev 2 shortly.


cheers