Errors linking with LLVM 5.0 - dump() missing

Hi,

I am finding that my project that previously successfully built with
versions 3.5 to 4.0 is now failing to link because of missing
implementation for dump(). Errors I get are:

Undefined symbols for architecture x86_64:

  "llvm::Type::dump() const", referenced from:
      ravi::LuaLLVMTypes::dump() in ravi_llvmtypes.cpp.o
      dump_content(lua_State*) in ravi_llvmluaapi.cpp.o
  "llvm::Value::dump() const", referenced from:
      dump_content(lua_State*) in ravi_llvmluaapi.cpp.o
  "llvm::Module::dump() const", referenced from:

This appears to be a change that is not documented in the release
notes of 5.0. Please can someone describe what the change is and how I
can detect whether the dump() implementation is available or not?

It also seems strange that dump() implementation was removed - surely
it would have been better ti stub it so that client code does not
break?

Regards
Dibyendu

Hi Dibyendu,

Are you building a Debug or Release version of the compiler?

I had similar problems a few days ago when I was migrating to v5.0, and it turned out that I had calls to some of the dump routines that were not guarded by either 'DEBUG(...)' or '#ifndef NDEBUG ... #endif' and I was seeing the link failures with a Release build. These had been there since LLVM v3.1 and had never broken before. There was some clean-up done in the sources so that may of the 'dump' routines are enabled only for Debug builds, and I fixed these (in my code) by adding these guards.

All the best,

  MartinO

Hi Martin,

Are you building a Debug or Release version of the compiler?

It seems that in Release builds of LLVM 5.0 the dump() implementation
is absent, although the method is available in the interface. This is
plain wrong in my view. If the dump() has to be removed then it should
not be present in the interface.

I had similar problems a few days ago when I was migrating to v5.0, and it turned out that I had calls to some of the dump routines that were not guarded by either 'DEBUG(...)' or '#ifndef NDEBUG ... #endif' and I was seeing the link failures with a Release build. These had been there since LLVM v3.1 and had never broken before. There was some clean-up done in the sources so that may of the 'dump' routines are enabled only for Debug builds, and I fixed these (in my code) by adding these guards.

Well the problem is that there is no obvious way for a client to
detect whether the dump() implementation is available or not - as it
does not depend upon the Release/Debug build status of the client, but
how LLVM 5.0 was originally compiled. So the guards you have put are
not really going to work (that is my finding anyway).

Regards
Dibyendu

Yes, if it is in the interface it would make more sense to have a null implementation at the very least. In my out-of-tree target, I also removed them from the interface if the build was for Release to ensure that I got compile-time errors to reveal other places I might have otherwise missed.

All the best,

  MartinO

Hi Martin

Yes, if it is in the interface it would make more sense to have a null implementation at the very least. In my out-of-tree target, I also removed them from the interface if the build was for Release to ensure that I got compile-time errors to reveal other places I might have otherwise missed.

I assume you now need to create two LLVM builds - a debug build and a
release build. Then you need to link your own app's debug build /
release build to the right LLVM build.

On Windows this has to happen anyway due to dependencies on the C
libraries etc. but on Linux/Mac OSX I never had to until now build a
debug build of LLVM.

As I mentioned the real problem is that the client has no way of
knowing how LLVM was built - as there is no #define to flag the
missing dump() implementation either.

Regards
Dibyendu

Thanks for reporting this.

Looks like this one was missed – the declaration should have been #ifdef’d away along with the definition. A quick grep indicates there are a number of them that need to be fixed.

Here’s the original commit:

commit 88d207542b618ca6054b24491ddd67f8ca397540
Author: Matthias Braun <matze@braunis.de>

Cleanup dump() functions.

We had various variants of defining dump() functions in LLVM. Normalize
them (this should just consistently implement the things discussed in
http://lists.llvm.org/pipermail/cfe-dev/2014-January/034323.html

For reference:

  • Public headers should just declare the dump() method but not use
    LLVM_DUMP_METHOD or #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
  • The definition of a dump method should look like this:
    #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
    LLVM_DUMP_METHOD void MyClass::dump() {
    // print stuff to dbgs()…
    }
    #endif

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@293359 91177308-0d34-0410-b5e6-96231b3b80d8

Hi Don,

Thanks for reporting this.

Looks like this one was missed -- the declaration should have been #ifdef'd
away along with the definition. A quick grep indicates there are a number
of them that need to be fixed.

Here's the original commit:

commit 88d207542b618ca6054b24491ddd67f8ca397540
Author: Matthias Braun <matze@braunis.de>
Date: Sat Jan 28 02:02:38 2017 +0000

    Cleanup dump() functions.

    We had various variants of defining dump() functions in LLVM. Normalize
    them (this should just consistently implement the things discussed in
    http://lists.llvm.org/pipermail/cfe-dev/2014-January/034323.html

    For reference:
    - Public headers should just declare the dump() method but not use
      LLVM_DUMP_METHOD or #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
    - The definition of a dump method should look like this:
      #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
      LLVM_DUMP_METHOD void MyClass::dump() {
        // print stuff to dbgs()...
      }
      #endif

    git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@293359
91177308-0d34-0410-b5e6-96231b3b80d8

I would argue that removing dump() is the wrong thing to do even in
Release builds. I am using LLVM as a JIT. In this use case, it is
essential that one can inspect the IR at runtime. Perhaps in the AOT
case dump() is not used in release versions but that is not the case
in a JIT use case.

Unless there is an equivalent alternative to dump() that is always available.

Regards

Dibyendu

It’ll work in release builds – just rebuild llvm with LLVM_ENABLE_DUMP enabled.

Hi Don,

It'll work in release builds -- just rebuild llvm with LLVM_ENABLE_DUMP
enabled.

That assumes one has control over the LLVM build options.

The dump() methods are only meant to be used in debuggers and are only available in debug builds of LLVM. There are often similar print() methods available though.

- Matthias

Or, instead of lobbying for llvm to always define dump(), you could lobby for it to enabled by default.

Hi Don,

Or, instead of lobbying for llvm to always define dump(), you could lobby
for it to enabled by default.

I was about to suggest the same thing - i.e. should not the default be
to maintain compatibility with earlier releases, and the new build
option to change the default?

Hi Matthias,

The dump() methods are only meant to be used in debuggers and are only available in debug builds of LLVM. There are often similar print() methods available though.

I am not sure how this is the case seeing that the dump() function has
been working in release builds in all the LLVM releases I have used
(from 3.5 to 4.0).

Hi Matthias,

The dump() methods are only meant to be used in debuggers and are only available in debug builds of LLVM. There are often similar print() methods available though.

I am not sure how this is the case seeing that the dump() function has
been working in release builds in all the LLVM releases I have used
(from 3.5 to 4.0).

The idea behind r198456 and the whole discussion in http://lists.llvm.org/pipermail/cfe-dev/2014-January/034323.html was that the dump() method are not available in release builds to safe code size. The dump methods can be included in the release builds anyway by enabling LLVM_ENABLE_DUMP.

The actual implementation of this policy however lacked and depending on which dump() method you looked you would find a different pattern of when it is disable/enabled; I cleaned this up in r293359 so all dump methods are enabled/disabled consistenly.

In any case if your client code uses dump() methods then I’d recommend to change it.

  • Matthias

None of these were removed.
lib/IR/AsmWriter.cpp:void Module::dump() const {

lib/IR/AsmWriter.cpp:void Value::dump() const { print(dbgs(), /IsForDebug=/true); dbgs() << ‘\n’; }

lib/IR/AsmWriter.cpp:void Type::dump() const { print(dbgs(), /IsForDebug=/true); dbgs() << ‘\n’; }

It also seems strange that dump() implementation was removed - surely
it would have been better ti stub it so that client code does not
break?

LLVM explicitly does not guarantee API compatibility.

Is there a way to pass LLVM_ENABLE_DUMP into cmake from outside without changing any of the configuration files? With Chapel's use of LLVM, we try to avoid modifying any of the source files that come from the LLVM distribution. Our own Makefile that builds Chapel also invokes cmake to build LLVM. I have tried setting environment variables and passing command-line arguments, and nothing seems to work.

      I hope I've just done it wrong and there is a way to make this work without modifying files from the distribution. A debug build of LLVM is too slow for our purposes, but we need the dump() methods for debugging Chapel.

          David

Looks like nobody got around adding a flag. (this really should be fixed).

In the meantime `cmake -DCMAKE_CXX_FLAGS="-DLLVM_ENABLE_DUMP"` should do the trick.

- Matthias

Thank you. That was the flag we needed.

      Unfortunately, when I tried this just now, I found that the declaration of llvm::MachineRegisterInfo::dumpUses() in include/llvm/CodeGen/MachineRegisterInfo.h is missing the check against LLVM_ENABLE_DUMP. It has only

#ifndef NDEBUG

which causes the build to fail because the definition in lib/CodeGen/MachineRegisterInfo.cpp is guarded by this.

#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)

          David

Unfortunately, this was only the beginning. I tried updating the header file to test LLVM_ENABLE_DUMP as well, and when I did that, the build hit another such case, which I tried fixing, and then it hit another, and another. Here is what I have found so far that would need to be fixed under the current arrangement.

LLVM_ENABLE_DUMP added to:

include/llvm/CodeGen/MachineRegisterInfo.h
  dumpUses()

include/llvm/CodeGen/MachineScheduler.h
  dumpScheduledState()

include/llvm/CodeGen/TargetSchedule.h
  getResourceName()

include/llvm/MC/MCSchedule.h
  Name

utils/TableGen/SubtargetEmitter.cpp
  emitted code inside EmitSchedModel()

Unknown location in AArch64
  This is not the end. I just lost track of what to change at this point.

      Perhaps it would be easier to revert the change that hid the dump() methods. That would have another advantage as well. Before LLVM 5.0.0, Chapel had the ability to link with an existing pre-built LLVM if it was installed on the system. With LLVM 5.0.0, we cannot do that any longer because we need to compile with LLVM_ENABLE_DUMP defined. It takes 10x longer to compile LLVM than it does to compile the rest of Chapel, so it is quite a burden to require a recompilation of LLVM to get the dump() methods.

          David

In the meantime `cmake -DCMAKE_CXX_FLAGS="-DLLVM_ENABLE_DUMP"` should do the trick.

     Thank you. That was the flag we needed.
     Unfortunately, when I tried this just now, I found that the declaration of llvm::MachineRegisterInfo::dumpUses() in include/llvm/CodeGen/MachineRegisterInfo.h is missing the check against LLVM_ENABLE_DUMP. It has only
#ifndef NDEBUG
which causes the build to fail because the definition in lib/CodeGen/MachineRegisterInfo.cpp is guarded by this.
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)

    Unfortunately, this was only the beginning. I tried updating the header file to test LLVM_ENABLE_DUMP as well, and when I did that, the build hit another such case, which I tried fixing, and then it hit another, and another. Here is what I have found so far that would need to be fixed under the current arrangement.

LLVM_ENABLE_DUMP added to:

include/llvm/CodeGen/MachineRegisterInfo.h
  dumpUses()

include/llvm/CodeGen/MachineScheduler.h
  dumpScheduledState()

include/llvm/CodeGen/TargetSchedule.h
  getResourceName()

include/llvm/MC/MCSchedule.h
  Name

utils/TableGen/SubtargetEmitter.cpp
  emitted code inside EmitSchedModel()

Unknown location in AArch64
  This is not the end. I just lost track of what to change at this point.

I guess that was to be expected with an option that didn't even have a cmake flag. I would propose to just check for LLVM_ENABLE_DUMP in the sourcecode and move the logic that debug builds have dump() methods to the cmake side of things to simplify things. But I think that needs agreement and someone to write a patch.

+CC some of the people who I believe made the decision for the current style.

    Perhaps it would be easier to revert the change that hid the dump() methods. That would have another advantage as well. Before LLVM 5.0.0, Chapel had the ability to link with an existing pre-built LLVM if it was installed on the system. With LLVM 5.0.0, we cannot do that any longer because we need to compile with LLVM_ENABLE_DUMP defined. It takes 10x longer to compile LLVM than it does to compile the rest of Chapel, so it is quite a burden to require a recompilation of LLVM to get the dump() methods.

Again: I consider client code calling dump() to be a misuse.

If you want to dump IR, can't you just switch your code from
    Value.dump(); to Value.print(dbgs(), true); dbgs() << '\n';
?

- Matthias