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

[David Holmes]

Thanks Coleen!

David

On 16/12/2017 1:21 AM, coleen.phillimore at oracle.com wrote:
> This looks good.
> Coleen
> 
> On 12/15/17 8:17 AM, Daniel D. Daugherty wrote:
>> On 12/14/17 9:29 PM, David Holmes wrote:
>>> 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/
>>
>> src/hotspot/share/runtime/arguments.cpp
>>     No comments.
>>
>> Thumbs up!
>>
>> Dan
>>
>>
>>>
>>> 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
>>>>
>>
>