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

[Daniel D. Daugherty]

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