RFR 8209087: Clean up runtime code that compares 'this' to NULL

[Harold David Seigel]

Hi Gerard,

Thanks for reviewing this.

Prior to my patch, contains() returned false if _all was NULL or if the 
name was not found in _all.  The old code looked like this:

    inline bool PerfDataManager::exists(const char* name) {
       return _all->contains(name);
    }

    bool PerfDataList::contains(const char* name) { return
    find_by_name(name) != NULL; }

    PerfData* PerfDataList::find_by_name(const char* name) {

       // if add_item hasn't been called the list won't be initialized
       if (this == NULL)
         return NULL;
                ...
    }

If _all is NULL then contains() gets NULL back from find_by_name() and 
then returns false.  So, even in the existing code, exists() will return 
false if _all is NULL.

 >> Is "_all” ever allowed to be “null” here?
Since _all gets initialized to NULL and gets set to NULL, I think it is 
good to check for it properly without worry if a compiler will optimize 
away "this == NULL".

>>Is line 64 returning “false”, the same as line 62 returning “false”?
No. Line 62 returns false because the name is not found in a non-null _all.
Line 64 returns false because _all is null.

Thanks, Harold

On 10/17/2018 1:41 PM, Gerard Ziemski wrote:
> hi Harold,
>
> Looks good. One question though:
>
> In http://cr.openjdk.java.net/~hseigel/bug_8209087/webrev/src/hotspot/share/runtime/perfData.inline.hpp.frames.html
>
>    60 inline bool PerfDataManager::exists(const char* name) {
>    61   if (_all != NULL) {
>    62     return _all->contains(name);
>    63   } else {
>    64     return false;
>    65   }
>    66 }
>
> the function “PerfDataManager::exists" can return “false” if “_all"== NULL, when before it would return “false” only if "_all->contains(name)” returned false (it assumed “_all”!= NULL). Is "_all” ever allowed to be “null” here? Is line 64 returning “false”, the same as line 62 returning “false”?
>
> Similar question for http://cr.openjdk.java.net/~hseigel/bug_8209087/webrev/src/hotspot/share/runtime/perfData.cpp.frames.html
>
>
> cheers
>
>> On Oct 17, 2018, at 8:05 AM, Harold David Seigel <harold.seigel at oracle.com> wrote:
>>
>> Hi,
>>
>> Please review this JDK-12 fix for JDK-8209087.  This fix removes comparisons of 'this' to 'NULL' from the three remaining methods containing this code.  Callers of these methods were looked at to see if their calling object might be NULL.  If so, then checks for NULL were added to these callers.
>>
>> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8209087/webrev/index.html
>>
>> JBS Bug:  https://bugs.openjdk.java.net/browse/JDK-8209087
>>
>> The fix was tested by running Mach5 tiers 1 and 2 tests and builds on Linux-x64, Windows, and Mac OS X, running tiers 3-5 tests on Linux-x64, and by running JCK-12 Lang and VM tests on Linux-x64.
>>
>> Thanks, Harold
>>