[libcxx-commits] [libcxx] [llvm] [libcxx][CMake] Rework LLVM_ENABLE_ASSERTIONS interaction w. libcxx hardening (PR #229359)

[Mészáros Gergely via libcxx-commits]

Maetveis wrote:

> I am not a fan for this change, because:
> 
> * LLVM_ENABLE_ASSERTION is a global debugging switch. It makes sense to build libcxx binary with extensive mode when configured together.

I would like to slightly push back against this.
What's different between other uses of LLVM_ENABLE_ASSERTIONS and this one is that by tying together LIBCXX_HARDENING_MODE with LLVM_ENABLE_ASSERTIONS we are controlling the performance/safety of the generated code and not of the code generator (e.g. clang). I think this is not something that most vendors of the toolchain expect (it was unexpected for me).

Vendors of the LLVM toolchain might reasonably want to make the choice that they want to make a safer (but slower) code generator, but do not want to make compromises on the performance of the code it generates by default.

As an analogy LLVM_ENABLE_ASSERTIONS could cause the undefined behavior sanitizer to be linked by default in clang. I believe if proposed such a change would be rejected today due to this reason.

I understand that libc++ is different than clang in this regard because it is a runtime, not a compiler component, by definition it affects only the generated code and not the code generator.
I think when looking at it from the libc++ developer perspective your point stands, but trying to look at it from a vendor's POV it is counter-intuitive.

If nothing else I believe this needs to be addressed in the documentation, for example currently the page [Building LLVM with CMake](https://llvm.org/docs/CMake.html) says this about LLVM_ENABLE_ASSERTIONS:

> **LLVM_ENABLE_ASSERTIONS:BOOL**
Enables code assertions.

and earlier explaining what assertions are:

> Assertions are internal checks to help you find bugs. They typically slow down LLVM and Clang when enabled but can be useful during development.

no mention of impact to the generated code.

> * If different configuration between LLVM and libcxx is needed, you can always configure them separately.
> 
> I think if you either support complete separate configuration between libcxx and llvm, or just lock them and ask user to configure them separately. This patch is trying to do something in between and only adds confusion.

This forgoes the simplicity of the single cmake invocation to build LLVM and libc++, but that is a reasonable price to pay in exchange for greater flexibility.



https://github.com/llvm/llvm-project/pull/229359