Question about LLVM Building Error with "-DLLVM_ENABLE_DUMP" and "RelWithDebInfo"

Hi All,

I have tried to build llvm tip as following:

cmake -DCMAKE_CXX_FLAGS:STRING="-DLLVM_ENABLE_DUMP" -DCMAKE_BUILD_TYPE=RelWithDebInfo ../llvm

After running 'make', I have got error messages like below.

llvm/lib/CodeGen/MachineRegisterInfo.cpp:462:67: error: no ‘void llvm::MachineRegisterInfo::dumpUses(unsigned int) const’ member function declared in class ‘llvm::MachineRegisterInfo’

llvm/lib/CodeGen/MachineScheduler.cpp:2331:57: error: no ‘void llvm::SchedBoundary::dumpScheduledState()’ member function declared in class ‘llvm::SchedBoundary’

...

It seems the "defined(LLVM_ENABLE_DUMP)" is needed on several locations. How do you think about it? I have attached the diff file about the locations for reference. If I missed something, please let me know.

Thanks,

JinGu Kang

dump.diff (2.68 KB)

llvm/lib/CodeGen/MachineRegisterInfo.cpp:462:67: error: no ‘void
llvm::MachineRegisterInfo::dumpUses(unsigned int) const’ member function
declared in class ‘llvm::MachineRegisterInfo’

llvm/lib/CodeGen/MachineScheduler.cpp:2331:57: error: no ‘void
llvm::SchedBoundary::dumpScheduledState()’ member function declared in
class ‘llvm::SchedBoundary’

​I see above two already have the fix,

I think the idea is to keep NDEBUG out of headers when possible. So I think this should better be something like:

-#ifndef NDEBUG
   void dumpUses(unsigned RegNo) const;
-#endif

to be inline with various other dumpers (like MachineInstr::dump(), Pass::dump(), ...)

If that works for you please submit a patch to phabricator as described in http://llvm.org/docs/DeveloperPolicy.html#making-and-submitting-a-patch

- Matthias

Hi Matthias and 陳韋任,

I appreciate your comments. I have submitted patch to https://reviews.llvm.org/D31866 accoding to Matthias comment. Please review it.

Thanks,

JinGu Kang

I’m fine with leaving methods there, but we need to be able to compile-out fields in structure.

We already have ABI_BREAKING_CHECKS for instance to this end, the naming isn’t completely in line with LLVM_ENABLE_DUMP but could be unified.

From: llvm-dev [mailto:llvm-dev-bounces@lists.llvm.org] On Behalf Of Mehdi
Amini via llvm-dev
Sent: Sunday, April 09, 2017 2:26 PM
To: Matthias Braun
Cc: llvm-dev@lists.llvm.org; jingu@codeplay.com
Subject: Re: [llvm-dev] Question about LLVM Building Error with "-
DLLVM_ENABLE_DUMP" and "RelWithDebInfo"

>
> I think the idea is to keep NDEBUG out of headers when possible. So I
think this should better be something like:
>
> -#ifndef NDEBUG
> void dumpUses(unsigned RegNo) const;
> -#endif
>
> to be inline with various other dumpers (like MachineInstr::dump(),
Pass::dump(), …)

I’m fine with leaving methods there, but we need to be able to compile-out
fields in structure.

Hmmm seems to me this has come up in the past, and somebody pointed out
that it prevents building a debug-mode front-end against a release-mode LLVM.
(Why is that a valid use-case? If I have an out-of-tree front end, and
especially one with a different license, I might well prefer to download
only LLVM releases rather than keep up-to-date with a live tree that I
build myself. IIRC we do not provide debug-mode downloads, therefore
anything that affects struct size/layout will break this use-case.)
--paulr

Presently several of our headers have definitions like:

#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
void dump() const;
#endif

Would it make sense to modify the build system to define LLVM_ENABLE_DUMP in config.h on debug builds?

Then we could wrap dump methods just based on LLVM_ENABLE_DUMP instead of two variables.

-Chris

That’s why we’re not using NDEBUG but the ABI_BREAKING_… macro which is set by the llvm-config.h.

The situation is not consistent. Yes there are several places where we have the #if in the headers however there are far more cases where it is not. Some points here:

  • This whole LLVM_DUMP_FUNCTION/LLVM_ENABLE_DUMP is about enabling the linker to strip (or not strip) the dumping function in release (debug) builds.

  • For this it doesn’t matter whether you have a declaration in the header or not, so it seems we standardized on not having it there.

  • Things are 100% consistent so we sometimes have #ifs anyway.

  • In case of templates we not only have the declaration but also an implementation in the header and need the #if there

  • A similar problem arises in cases where the dump function was declared virtual and ends up in the vtable

  • If you ask me then we shouldn’t have LLVM_ENABLE_DUMP and just rely on NDEBUG to keep things simple… (We’re in this strange state anyway where LLVM_ENABLE_DEBUG isn’t even exposed as a cmake option).

  • Either way, putting LLVM_ENABLE_DUMP into config.h would make the status-quo more consistent.

  • Matthias

The situation is not consistent. Yes there are several places where we have the #if in the headers however there are far more cases where it is not. Some points here:

  • This whole LLVM_DUMP_FUNCTION/LLVM_ENABLE_DUMP is about enabling the linker to strip (or not strip) the dumping function in release (debug) builds.
  • For this it doesn’t matter whether you have a declaration in the header or not, so it seems we standardized on not having it there.

This should read:
“… so it seems we standardized on not having the #if there.”

Using the config.h instead of the NDEBUG makes it robust against client using NDEBUG differently from what LLVM was built with. This seems really better to me.

Jingu: Why do you even want a configuration that has LLVM_ENABLE_DUMP but does not have asserts enabled at the same time?
If this is just about getting a compiler fast enough to handle big code, most people seem to settle on CMAKE_BUILD_TYPE=Release, LLVM_ENABLE_ASSERTIONS=1 (aka. “RelWithAsserts”).

The situation is not consistent. Yes there are several places where we have the #if in the headers however there are far more cases where it is not. Some points here:

  • This whole LLVM_DUMP_FUNCTION/LLVM_ENABLE_DUMP is about enabling the linker to strip (or not strip) the dumping function in release (debug) builds.
  • For this it doesn’t matter whether you have a declaration in the header or not, so it seems we standardized on not having it there.
  • Things are 100% consistent so we sometimes have #ifs anyway.
  • In case of templates we not only have the declaration but also an implementation in the header and need the #if there
  • A similar problem arises in cases where the dump function was declared virtual and ends up in the vtable
  • If you ask me then we shouldn’t have LLVM_ENABLE_DUMP and just rely on NDEBUG to keep things simple… (We’re in this strange state anyway where LLVM_ENABLE_DEBUG isn’t even exposed as a cmake option).
  • Either way, putting LLVM_ENABLE_DUMP into config.h would make the status-quo more consistent.

Using the config.h instead of the NDEBUG makes it robust against client using NDEBUG differently from what LLVM was built with. This seems really better to me.

Maybe we should rather go and remove the whole LLVM_ENABLE_DUMP setting as we probably will not set up a bot to it while at the same time we probably can’t keep this consistently in a working state…

  • Matthias

The situation is not consistent. Yes there are several places where we have the #if in the headers however there are far more cases where it is not. Some points here:

  • This whole LLVM_DUMP_FUNCTION/LLVM_ENABLE_DUMP is about enabling the linker to strip (or not strip) the dumping function in release (debug) builds.
  • For this it doesn’t matter whether you have a declaration in the header or not, so it seems we standardized on not having it there.
  • Things are 100% consistent so we sometimes have #ifs anyway.
  • In case of templates we not only have the declaration but also an implementation in the header and need the #if there
  • A similar problem arises in cases where the dump function was declared virtual and ends up in the vtable
  • If you ask me then we shouldn’t have LLVM_ENABLE_DUMP and just rely on NDEBUG to keep things simple… (We’re in this strange state anyway where LLVM_ENABLE_DEBUG isn’t even exposed as a cmake option).

See: http://bugs.llvm.org/show_bug.cgi?id=32283

I might just have to fix that this week now that you’ve brought it up :slight_smile:

-Chris

Hi Matthias,

Jingu: Why do you even want a configuration that has LLVM_ENABLE_DUMP but does not have asserts enabled at the same time?

My colleague and I am doing custom project using clang/llvm. We have always wanted to use the IR Value’s dump() to check our implementation correctly with Debug, Release and another builds. We thought the LLVM_ENABLE_DUMP is for it.

If Chris fixes http://bugs.llvm.org/show_bug.cgi?id=32283, I will be happy with it. :slight_smile:

Thanks,

JinGu Kang

I missed this email earlier, but that totally makes sense to me (ideally with a link/runtime check like the ABI_BREAKING_CHECK one).

Thanks,

Hi Matthias,

Jingu: Why do you even want a configuration that has LLVM_ENABLE_DUMP but does not have asserts enabled at the same time?

My colleague and I am doing custom project using clang/llvm. We have always wanted to use the IR Value’s dump() to check our implementation correctly with Debug, Release and another builds. We thought the LLVM_ENABLE_DUMP is for it.

dump() is meant as a helper for people using a debugger and we tend not to use it in code.

There are usually alternatives that you could use in code like Value->print(errs());

  • Matthias

I appreciate for your kind comment Matthias. I will try it. :slight_smile:

Thanks again,

JinGu Kang