Volker Simonis <###@###.###> wrote:

Hi,

in src/share/vm/utilities/exceptions.hpp some helper macros get
defined as follows:

#define CHECK                                    THREAD); if
(HAS_PENDING_EXCEPTION) return       ; (0
#define CHECK_(result)                           THREAD); if
(HAS_PENDING_EXCEPTION) return result; (0
#define CHECK_0                                  CHECK_(0)
#define CHECK_NH                                 CHECK_(Handle())
#define CHECK_NULL                               CHECK_(NULL)
#define CHECK_false                              CHECK_(false)

They are intended to simplify the calling of methods which require a THREAD
reference and which can potentially raise an exception.

A typical use case is the following (taken from 'klass.cpp'):

  Klass*   kl = (Klass*) vtbl.allocate_permanent(klass, size, CHECK_NULL);
  klassOop k  = kl->as_klassOop();

which will be macro expanded into:

  Klass*   kl = (Klass*) vtbl.allocate_permanent(klass, size, THREAD);
  if (HAS_PENDING_EXCEPTION) return NULL; (0);
  klassOop k  = kl->as_klassOop();

This use case is reasonable and fine.

However there are a lot of places in the Hotspot, where the check macros are
used as follows (taken from 'perfData.hpp'):

  static PerfStringVariable* create_string_variable(CounterNS ns,
                                                    const char* name,
                                                    const char *s, TRAPS) {
    return create_string_variable(ns, name, 0, s, CHECK_NULL);
  };

This will expand into:

  static PerfStringVariable* create_string_variable(CounterNS ns,
                                                    const char* name,
                                                    const char *s, TRAPS) {
    return create_string_variable(ns, name, 0, s, THREAD);
    if (HAS_PENDING_EXCEPTION) return NULL; (0);
  };

which contains unreachable code after the return statement.

Now this is not only a problem of an omitted check for a pending exception
(which is probably not so problematic here because the function returns
anyway) but more a problem with modern compilers which issue a warning here
because of unreachable code.

And because this wrong usage pattern of the CHECK macros is spread across
several include files (e.g. constantPoolOop.hpp, oopFactory.hpp,
typeArrayKlass.hpp, perfData.hpp, synchronizer.hpp) we get A LOT of warnings
for nearly every compilation unit.

I would therefore suggest to replace such "wrong" usages of the CHECK macros
with simple THREAD macros. If somebody feels that the checks for pending
exceptions are really necessary in some places, we should use a local variable
to save the return value of the function call and return that variable in the
next statement.