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

fix name clash of assert macro in debug.hpp with libc's assert macro

    Details

    • Type: Enhancement
    • Status: Open
    • Priority: P4
    • Resolution: Unresolved
    • Affects Version/s: 9
    • Fix Version/s: 11
    • Component/s: hotspot

      Description

      As reported in:

      https://bugs.openjdk.java.net/show_bug.cgi?id=100297

       Description From Radics Péter 2013-02-02 10:34:52 PDT

      Created an attachment (id=288) [details]
      Rename assert macro to assert_vm as it conflicts with libc's assert.

      hotspot/src/share/vm/utilities/debug.hpp defines an assert macro with two
      parameters. This macro definition clashes with libc's assert macro from
      <assert.h> (and to the best of my knowledge it's undefined behavior to redefine
      assert like this).

      The name clash causes build failures if any of the files using debug.hpp
      directly or indirectly include <assert.h> (or <cassert>), too.

      This doesn't seem to happen with libstdc++, but this error is triggered if
      trying to build with libc++ (http://libcxx.llvm.org).

      I propose changing the name of the assert macro defined in debug.hpp to
      assert_vm (based on the fact that this macro is used solely in the vm
      subsystem).

      I've broken up the patches to three parts, these should be applied in this
      order:

      1) patch-assert-vm-1.txt changes only the assert macro to assert_vm only in
      debug.hpp (note: this patch assumes that the typo fix from bug 100294 is
      already applied)
      2) patch-assert-vm-2.txt removes the trickery used in
      hotspot/src/share/vm/shark/llvmHeaders.hpp that was applied to avoid the
      name-clash brought forth by including LLVM headers
      3) fix-assert-vm-users.sh changes all other references to assert(cond, msg) to
      assert_vm(cond, msg). This script should be run in hotspot/src. I decided to
      post a fix-up script instead of a patch because the patch would be huge (there
      are a lot of uses of the "new" assert macro.

        Issue Links

          Activity

          Hide
          kbarrett Kim Barrett added a comment -
          Coleen and I were talking about this the other day, and she asked me to add my thoughts on a process for addressing this issue.

          First, rename assert() to vmassert() (or whatever name is ultimately chosen), but provide a definition for assert that expands into vmassert for (temporary) backward compatibility.

          Second, backport that rename + backward compatibility definition to earlier versions to which we expect to be targets for significant other backports unrelated to the assert change.

          Third, perform mechanical rename on the current code base. It can be broken up into pieces if that's convenient.

          By backporting the rename + compatibility definition we make it easier to backport post-mechanical-renaming changes.

          Once the rename is complete in the current code base, remove the compatibility definition from the current code base (only), leaving it in older versions to continue to support backporting.
          Show
          kbarrett Kim Barrett added a comment - Coleen and I were talking about this the other day, and she asked me to add my thoughts on a process for addressing this issue. First, rename assert() to vmassert() (or whatever name is ultimately chosen), but provide a definition for assert that expands into vmassert for (temporary) backward compatibility. Second, backport that rename + backward compatibility definition to earlier versions to which we expect to be targets for significant other backports unrelated to the assert change. Third, perform mechanical rename on the current code base. It can be broken up into pieces if that's convenient. By backporting the rename + compatibility definition we make it easier to backport post-mechanical-renaming changes. Once the rename is complete in the current code base, remove the compatibility definition from the current code base (only), leaving it in older versions to continue to support backporting.
          Hide
          kbarrett Kim Barrett added a comment -
          As noted in a previous comment, it is not undefined behavior to merely provide a macro named "assert" which is different from that provided by the standard <assert.h> / <cassert> headers.

          It *is* undefined behavior to have two definitions of a macro simultaneously active, unless the two definitions are of the same kind (object-like or function-like) and have identical replacement lists. As such, the standard "assert" and the hotspot "assert" cannot coexist in a given preprocessing context.

          The reason to rename the hotspot "assert" to "vmassert" is to make it easier to use code being imported into / used by hotspot that is not "owned" by hotspot, and so might be using the standard "assert" macro.

          - The example in the original complaint is the use of headers from libc++.

          - The "#undef assert ...code... (re) #define assert <copy of hotspot defn>" dance performed in shark/llvmHeaders.hpp is another example.

          - OS system headers and headers for third-party packages that hotspot might need or wish to use can run into similar problems.
          Show
          kbarrett Kim Barrett added a comment - As noted in a previous comment, it is not undefined behavior to merely provide a macro named "assert" which is different from that provided by the standard <assert.h> / <cassert> headers. It *is* undefined behavior to have two definitions of a macro simultaneously active, unless the two definitions are of the same kind (object-like or function-like) and have identical replacement lists. As such, the standard "assert" and the hotspot "assert" cannot coexist in a given preprocessing context. The reason to rename the hotspot "assert" to "vmassert" is to make it easier to use code being imported into / used by hotspot that is not "owned" by hotspot, and so might be using the standard "assert" macro. - The example in the original complaint is the use of headers from libc++. - The "#undef assert ...code... (re) #define assert <copy of hotspot defn>" dance performed in shark/llvmHeaders.hpp is another example. - OS system headers and headers for third-party packages that hotspot might need or wish to use can run into similar problems.
          Hide
          dholmes David Holmes added a comment -
          Unclear how this now relates to JDK-8068396 ?
          Show
          dholmes David Holmes added a comment - Unclear how this now relates to JDK-8068396 ?
          Hide
          kbarrett Kim Barrett added a comment -
          David: JDK-8068396 was a step toward the proposed resolution to this bug. See comment from 2014-06-27.
          Show
          kbarrett Kim Barrett added a comment - David: JDK-8068396 was a step toward the proposed resolution to this bug. See comment from 2014-06-27.
          Hide
          gziemski Gerard Ziemski added a comment - - edited
          A simple workaround if anyone runs into this issue while bringing a 3rd party lib:

          #include “header.hpp"
          #ifdef assert
          #undef assert
          #endif
          #define assert(p, ...) vmassert(p, __VA_ARGS__)

          At the top of the file before including other hotspot VM headers, where the “header.hpp” is the 3rd party lib header that pulls standard “assert”.
          Show
          gziemski Gerard Ziemski added a comment - - edited A simple workaround if anyone runs into this issue while bringing a 3rd party lib: #include “header.hpp" #ifdef assert #undef assert #endif #define assert(p, ...) vmassert(p, __VA_ARGS__) At the top of the file before including other hotspot VM headers, where the “header.hpp” is the 3rd party lib header that pulls standard “assert”.

            People

            • Assignee:
              Unassigned
              Reporter:
              dholmes David Holmes
            • Votes:
              1 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated: