RFR 8202171: Some oopDesc functions compare this with NULL

[Kim Barrett]

> On Jul 16, 2018, at 3:24 PM, Harold David Seigel <harold.seigel at oracle.com> wrote:
> 
> Hi,
> 
> Please review this JDK-12 fix for bug JDK-8202171.  The fix changes a few functions in oop.cpp into static functions to avoid comparisons between 'this' and NULL.
> 
> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8202171/webrev/index.html
> 
> JBS Bug:  https://bugs.openjdk.java.net/browse/JDK-8202171
> 
> This fix was regression tested by running Mach5 tiers 1 and 2 tests and builds on Linux-X64, Windows, Solaris Sparc, and Mac OS X, running tiers 3-5 tests on Linux-x64, and by running JCK-11 Lang and VM tests on Linux-x64.
> 
> Thanks, Harold

This looks good.

However, there is additional work to be done (described below).  I
think that work can be handled by new CRs, if not already covered by
other existing CRs.

------------------------------------------------------------------------------
src/hotspot/share/oops/oop.cpp
  92 void oopDesc::verify() {
  93   verify_on(tty, this);
  94 }

The compiler could elide the NULL check in the call to verify_on from
here, since "this" cannot be NULL here.  (Inline the call to verify_on
from verify, and notices the argument to verify_on was "this", which
cannot be NULL.)

I think oopDesc::verify() needs similar treatment, e.g. making it a
static function taking an oop argument.  I don't know how many places
call oopDesc::verify().  If it's a lot, dealing with this could be
done as a followup.

I think there are some other functions with similar issues,
e.g. print, print_value, print_address, print_string,
print_value_string (not sure I listed all of them).

------------------------------------------------------------------------------
There are more than a dozen other comparisons of "this" with NULL.
Are there bugs for any of these?

find . -type f -exec egrep -H "this\s*(\!|=)=\s*NULL" {} \;

./share/adlc/formssel.cpp:  if( this != NULL ) {
./share/adlc/formssel.cpp:  if( this == NULL ) return;
./share/libadt/set.cpp:  if( this == NULL ) return os::strdup("{no set}");
./share/runtime/perfData.cpp:  if (this == NULL)
./share/opto/chaitin.cpp:  if( this == NULL ) {          // Not got anything?
./share/asm/codeBuffer.cpp:  if (this == NULL) {
./share/oops/metadata.cpp:  if (this == NULL) {
./share/oops/symbol.cpp:  if (this == NULL) {
./share/oops/symbol.cpp:  if (this == NULL) {
./share/oops/metadata.hpp:    if (this == NULL)
./share/oops/metadata.hpp:    if (this == NULL)
./share/oops/method.cpp:  if (this == NULL) {
./os/bsd/osThread_bsd.cpp:  assert(this != NULL, "check");
./os/aix/osThread_aix.cpp:  assert(this != NULL, "check");
./os/linux/osThread_linux.cpp:  assert(this != NULL, "check");

Some of these might be things that really can't happen, and the check
is a mistaken attempt to be defensive.  In other cases, a NULL value
may be possible, but might not be handled as expected.

8202171 deals with these:
./share/oops/oop.cpp:  if (this == NULL) {
./share/oops/oop.cpp:  if (this == NULL) {
./share/oops/oop.cpp:  if (this != NULL) {

------------------------------------------------------------------------------