[PATCH] D38511: [cmake] Add LLVM_DEBUG variable which can be used to replace NDEBUG

[Don Hinton via Phabricator via llvm-commits]

hintonda added a comment.

In https://reviews.llvm.org/D38511#887415, @MatzeB wrote:

> Will we only use LLVM_DEBUG in the public headers or also in the implementation files and private headers?


I'd suggest outlawing NDEBUG, but initially the problem is usage of NDEBUG in public headers.

> Apart from `NDEBUG` there's also the peculiar case of `assert()`s in the headers; though they are probably less of a problem as they do not change the datastructures.

We could always add something like LLVM_ASSERT (or llvm_assert), if that becomes an issue.

> There is a danger that we will regress given that we are forced to set NDEBUG as well to keep `assert()` working, so we wouldn't notice if someone incorrectly adds new assert() or `#ifdef NDEBUG` into the headers. I sometimes wonder if we shouldn't add some verification/linting scripts to LLVM that are then run for each commit by a buildbot machine. In this case for example we could search the public headers for NDEBUG and warn/bail out if we find them. We can have this discussion separate from this patch of course.

Okay, sounds good.



================
Comment at: CMakeLists.txt:393-402
+  option(LLVM_FORCE_ENABLE_DUMP "Enable dump functions in release builds" OFF)
 else()
   option(LLVM_ENABLE_ASSERTIONS "Enable assertions" ON)
+  option(LLVM_FORCE_ENABLE_DUMP "Enable dump functions in release builds" ON)
+  set(LLVM_DEBUG ON)
 endif()
 
----------------
MatzeB wrote:
> I think we already have the `LLVM_ENABLE_ASSERTIONS` flag and should not introduce yet another one. We may or may not use `LLVM_ENABLE_DEBUG` instead of `LLVM_ENABLE_ASSERTIONS` for the preprocessor define.
This change only adds LLVM_DEBUG.  

The other changes you see are from D38406 which hasn't been approved yet, and just clean up setting LLVM_ENABLE_DUMP.


================
Comment at: include/llvm/Config/llvm-config.h.cmake:17-18
 
+/* Define if CMAKE_BUILD_TYPE is Debug */
+#cmakedefine LLVM_DEBUG
+
----------------
MatzeB wrote:
> I think `LLVM_ENABLE_DEBUG` would be more in line with other existing flags.
Will do...


https://reviews.llvm.org/D38511