cmake: NDEBUG needlessly defined in non-Release builds

[• janusz]

Summary

Currently, when llvm is configured with cmake (DEBUG build but with assertions OFF), then NDEBUG flag is defined, except when building for MSVC or XCode.
I think this is wrong, the NDEBUG should only be defined in Release build.

[• janusz]

Reid, are you the right person to ask for cmake changes review?

[rnk]

Sure, I can review this.

LLVM_ENABLE_ASSERTIONS is supposed to directly control NDEBUG, and Release/Debug is supposed to control optimizations and debug info. Therefore IMO this is correct as written.

I imagine it is confusing to users to have to explicitly flip LLVM_ENABLE_ASSERTIONS=ON in a debug build. I don't think adding tristate logic to detect when the user does a debug build without setting LLVM_ENABLE_ASSERTIONS is worth it, though.

Does that seem reasonable?

I don't know why the VS and XCode logic exists, that seems annoyingly inconsistent.

[• janusz]

Current situation is very inconsistent and confusing. I hit the issue when I first built a Release version and then switched to Debug in the same build directory. Cmake somehow preserved LLVM_ENABLE_ASSERTIONS=OFF from the Release build, so I ended up with a config, where CMAKE_BUILD_TYPE = DEBUG and LLVM_ENABLE_ASSERTIONS = OFF.

The most confusing bit was that the binaries i.e. llc didn't support '-debug'.
I think it is fair to expect that '-debug' parameter will be supported in DEBUG build, regardless of other options.

The documentation about LLVM_ENABLE_ASSERTIONS says it controls assertions. It doesn't say it controls debugging printfs too.

My understanding of the code in lines 50 - 69 in HandleLLVMOptions.cmake is that LLVM_ENABLE_ASSERTIONS depends on debugging output support, so it is enabled implicitly even in Release build. I'm OK with that because it allows to assert() in optimized code.

The change in lines 71 - 75 in the original file was introduced to fix http://llvm.org/bugs/show_bug.cgi?id=9155. Why is this change specific to MSVC and Xcode? I have no idea. I think this is wrong. It should be done consistently for all IDEs.

[rnk]

In D4257#9, @janusz wrote:

Current situation is very inconsistent and confusing. I hit the issue when I first built a Release version and then switched to Debug in the same build directory. Cmake somehow preserved LLVM_ENABLE_ASSERTIONS=OFF from the Release build, so I ended up with a config, where CMAKE_BUILD_TYPE = DEBUG and LLVM_ENABLE_ASSERTIONS = OFF.

This is normal CMake behavior. It's a bit confusing, but it's more of a CMake issue than an LLVM issue.

The most confusing bit was that the binaries i.e. llc didn't support '-debug'.
I think it is fair to expect that '-debug' parameter will be supported in DEBUG build, regardless of other options.

The documentation about LLVM_ENABLE_ASSERTIONS says it controls assertions. It doesn't say it controls debugging printfs too.

That could be improved.

My understanding of the code in lines 50 - 69 in HandleLLVMOptions.cmake is that LLVM_ENABLE_ASSERTIONS depends on debugging output support, so it is enabled implicitly even in Release build. I'm OK with that because it allows to assert() in optimized code.

Yeah, the idea is to support optimized builds with assertions.

The change in lines 71 - 75 in the original file was introduced to fix http://llvm.org/bugs/show_bug.cgi?id=9155. Why is this change specific to MSVC and Xcode? I have no idea. I think this is wrong. It should be done consistently for all IDEs.

It's trying to handle IDEs that support multiple configurations. If you generate a VS solution or XCode project, you can flip back and forth between release and debug in the IDE without rerunning CMake.

I'm starting to think we should take this patch. The effect of landing this patch is to drop support for debug builds without assertions, which is basically a useless configuration anyway. If we drop this else clause, we basically leave the definition of NDEBUG up to CMake, which is probably fine.

[• janusz]

I'm starting to think we should take this patch. The effect of landing this patch is to drop support for debug builds without assertions, which is basically a useless configuration anyway. If we drop this else clause, we basically leave the definition of NDEBUG up to CMake, which is probably fine.

I don't have write permission to the repo. Could you commit this patch?

[rnk]

Thanks, r211915

[rnk]

really closing

cmake/modules/HandleLLVMOptions.cmake

foreach (flags_var_to_scrub
         CMAKE_CXX_FLAGS_MINSIZEREL
         CMAKE_C_FLAGS_RELEASE
         CMAKE_C_FLAGS_RELWITHDEBINFO
         CMAKE_C_FLAGS_MINSIZEREL)
       string (REGEX REPLACE "(^| )[/-]D *NDEBUG($| )" " "
         "${flags_var_to_scrub}" "${${flags_var_to_scrub}}")
     endforeach()
   endif()
-else()
-  if( NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "RELEASE" )
-    if( NOT MSVC_IDE AND NOT XCODE )
-      add_definitions( -DNDEBUG )
-    endif()
-  endif()
 endif()
 
 if(WIN32)
   set(LLVM_HAVE_LINK_VERSION_SCRIPT 0)
   if(CYGWIN)
     set(LLVM_ON_WIN32 0)
     set(LLVM_ON_UNIX 1)
   else(CYGWIN)