RFR 8209087: Clean up runtime code that compares 'this' to NULL (original) (raw)

Gerard Ziemski gerard.ziemski at oracle.com
Thu Oct 18 13:32:20 UTC 2018

hi Harold,

Thank you for the explanation, looks good.

On Oct 17, 2018, at 1:56 PM, Harold David Seigel <harold.seigel at oracle.com> wrote:

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 findbyname(name) != NULL; } PerfData* PerfDataList::findbyname(const char* name) { // if additem 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 findbyname() 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/bug8209087/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/bug8209087/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/bug8209087/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

More information about the hotspot-runtime-dev mailing list