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

[coleen.phillimore at oracle.com]

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