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

[Dmitry Dmitriev]

Hi Gerard,

Thank you for fixing  the code. Please, look at my comments inline about 
CMSOldPLABMax and SurvivorAlignmentInBytes constraints:

On 26.05.2015 20:11, Gerard Ziemski wrote:
> 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).
I think that verify_min_value only verify parameter for minimal value, 
i.e. CMSOldPLABMax should not be less than 1. So, it seems that 
CMSOldPLABMax doesn't need constraint.

>
>
>>   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.
I oversight that error message is also incompelete. It should state that 
SurvivorAlignmentInBytes  must be greater or equal than. Currently it 
states only that SurvivorAlignmentInBytes must be greater.  Can you 
please fix that?

Thanks,
Dmitry


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