[JDK 11] RFR: (XS) 8193364: verify_special_jvm_flags should not cause an assertion failure when version is bumped

[David Holmes]

Hi Dan,

Thanks for looking at this.

I opted to add an explanatory comment block before the method instead.

http://cr.openjdk.java.net/~dholmes/8193364/webrev.v1/

Thanks,
David
-----

On 13/12/2017 11:51 PM, Daniel D. Daugherty wrote:
> On 12/13/17 12:03 AM, David Holmes wrote:
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8193364
>> webrev: http://cr.openjdk.java.net/~dholmes/8193364/webrev/
>>
>> verify_special_jvm_flags operates only in debug builds and sanity 
>> checks the entries in the deprecated/obsoleted/expired/aliased tables. 
>> In addition it detects whether we have not yet removed the global 
>> variables for flags that are obsolete/expired, and issues a warning. 
>> In the latter case though it also returns false and causes the 
>> assertion to fail. This make it impossible to bump the JDK version 
>> without dealing with every flag en-masse.
>>
>> We should issue the warnings but not fail the assert in these cases.
>>
>> --- old/src/hotspot/share/runtime/arguments.cpp    2017-12-12 
>> 23:59:14.039718085 -0500
>> +++ new/src/hotspot/share/runtime/arguments.cpp    2017-12-12 
>> 23:59:11.575576703 -0500
>> @@ -710,7 +710,6 @@
>>        if (!version_less_than(JDK_Version::current(), 
>> flag.obsolete_in)) {
>>          if (Flag::find_flag(flag.name) != NULL) {
>>            warning("Global variable for obsolete special flag entry 
>> \"%s\" should be removed", flag.name);
>> -          success = false;
> 
> You should add a comment in place of the removed to line.
> Otherwise, someone reading that function will think that
> setting 'success' to false was just missed. Perhaps:
> 
>            // Do not set success to 'false' in this case since
>            // we only want a warning here for ease of transition
>            // to a new release.
> 
>> }
>>        }
>>      }
>> @@ -720,7 +719,6 @@
>>        if (!version_less_than(JDK_Version::current(), flag.expired_in)) {
>>          if (Flag::find_flag(flag.name) != NULL) {
>>            warning("Global variable for expired flag entry \"%s\" 
>> should be removed", flag.name);
>> -          success = false;
> 
> And perhaps:
> 
>            // Do not set success to 'false'; see above note.
> 
> Dan
> 
> 
>> }
>>        }
>>      }
>>
>> This need was flagged when we went to JDK 10 (see JDK-8173421) but not 
>> actioned. We need it now before we bump the version for JDK 11.
>>
>> Thanks,
>> David
>