Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-6889002

CHECK macros in return constructs lead to unreachable code

    XMLWordPrintable

    Details

    • Type: Enhancement
    • Status: Closed
    • Priority: P4
    • Resolution: Duplicate
    • Affects Version/s: hs17, 9
    • Fix Version/s: 9
    • Component/s: hotspot
    • Subcomponent:
    • CPU:
      generic
    • OS:
      generic

      Description

      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.

        Attachments

          Issue Links

            Activity

              People

              Assignee:
              simonis Volker Simonis
              Reporter:
              twisti Christian Thalinger
              Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:
                Imported:
                Indexed: