[PATCH] cmake: NDEBUG needlessly defined in non-Release builds

[Reid Kleckner]

>>! 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.

http://reviews.llvm.org/D4257