NDEBUG in Header Files

Hi everyone,

we have several header files in which the NDEBUG macro is being used. In some of these, the NDEBUG macro changes the size, the interface, or the layout of a type. One example is AssertingVH, which turns into a flat, transparent Value* wrapper in Release build. Now everywhere you use these headers, the state of the NDEBUG flag must be the same, or else bad things could happen.

This turns out to be quite problematic for out-of-tree targets and projects, because it effectively forces them to use the same build type with which LLVM was compiled. Thus, a project maintainer needs to keep a Debug and a Release build of LLVM around, and is mostly barred from using binary distributions.

If the community agrees that this is a problem, I’d like to discuss two possible solutions.

Solution #1:
Don’t use NDEBUG (or any similar or depending macro) inside public-facing header files. I think this is the cleanest solution, but it requires a great amount of discipline on the side of developers and reviewers to stick with this pattern, and it will probably have a performance impact (e.g., for AssertingVH).

Solution #2:
Replace NDEBUG with a custom flag (e.g. LLVM_NDEBUG), and have CMake take care of synchronizing the two. I.e. ensure that LLVM_NDEBUG is set iff NDEBUG is set. Introducing this is probably a lot more work, because it touches all subprojects.

What do you think?

Best,

  • Philip

When we added the ABI breaking EpochTracker to DenseMap, we added a macro called LLVM_ENABLE_ABI_BREAKING_CHECKS, which is controlled by the CMake option LLVM_ABI_BREAKING_CHECKS. I think we should make all ABI-impacting uses of NDEBUG in headers use this macro instead.

I don’t know what the current behavior is, but I would like this macro to correlate with NDEBUG by default. People packaging LLVM as a library for redistribution can turn off the ABI breaking checks manually.

Also, this has come up before, although Alp was concerned with Clang, not LLVM:
http://lists.llvm.org/pipermail/llvm-dev/2013-November/067410.html

This has also come up with llvm before: http://reviews.llvm.org/D11833

  • Matthias

Hi,

I only checked the dev mailing list for a duplicate, didn’t think of looking for it in commits.

There’s some valid criticism in that thread, so I’ll think about this some more and if I come up with any more ideas I’ll reply on phab.

Best,

  • Philip