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

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
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;
          }
        }
      }
@@ -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;
          }
        }
      }

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
Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
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

Reply | Threaded
Open this post in threaded view
|

Re: [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
>
Reply | Threaded
Open this post in threaded view
|

Re: [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
>>

Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore
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
>>>
>

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
In reply to this post by Daniel D. Daugherty
Thanks Dan!

David

On 15/12/2017 11:17 PM, 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
>>>
>
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
In reply to this post by coleen.phillimore
Thanks Coleen!

David

On 16/12/2017 1:21 AM, [hidden email] 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
>>>>
>>
>