cmake: Delete -DNDEBUG from all available config-specific flags #1606

1. 
   hebasto commented at 5:58 pm on September 16, 2024: member

   During the integration of libsecp256k1’s build system into Bitcoin Core’s build system, it was decided to always build the most tested “RelWithDebInfo” configuration, regardless of the Bitcoin Core’s actual build configuration.

   To achieve this goal for muli-config generators, we assign to each CMAKE_C_FLAGS_<CONFIG> variable the default value of the CMAKE_C_FLAGS_RELWITHDEBINFO variable before processing libsecp256k1’s CMakeLists.txt file.

   The problem with this approach is that, at this point, the CMAKE_C_FLAGS_RELWITHDEBINFO variable has not yet been stripped of the -DNDEBUG flag, which leaks into other CMAKE_C_FLAGS_<CONFIG> variables.

   This PR fixes this issue.

2. cmake: Set `CMAKE_BUILD_TYPE` before adjusting config-specific flags

   This is a move-only change and required for the following commit.

   f3ec9771c0
3. cmake: Delete `-DNDEBUG` from all available config-specific flags d99c8b8e97
4. real-or-random added the label build on Sep 17, 2024
5. real-or-random added the label refactor/smell on Sep 17, 2024
6. 
   real-or-random commented at 2:45 pm on September 17, 2024: contributor

   Is it correct that the first commit is the actual fix and the second commit is just a refactor?
7. 
   hebasto commented at 2:54 pm on September 17, 2024: member

   Is it correct that the first commit is the actual fix and the second commit is just a refactor?

   The second commit is the fix, as the current implementation skips the CMAKE_C_FLAGS_DEBUG variable, falsely assuming it doesn’t contain -DNDEBUG.

   The first commit is a prerequisite since the new code processes the CMAKE_BUILD_TYPE variable, which must be set beforehand.

8. 
   real-or-random commented at 8:57 am on September 20, 2024: contributor

   Oh ok, I missed CMAKE_C_FLAGS_DEBUG…

   I’d Concept ACK this in principle, but I think there’s a simpler solution:

   Background

   The reason why we remove this entire -DNDEBUG thing is only because we use assert() (from the standard library’s <assert.h>) in the examples. Assertions can be made no-ops by passing -DNDEBUG, which CMake’s defaults always set, even in debug builds. See also the comment in the CMake file “# We leave assertions on because they are only used in the examples, and we want them always on there.”

   Alternative suggestion

   So this added complexity in the build system is only there because of the examples. We could just replace assert(expr) in the examples by if (!expr) exit(1), or even if (!expr) exit(EXIT_FAILURE). This gets us rid of the fiddling with CMAKE_C_FLAGS_* variables entirely.

   We also have some cases of return 1 in main(), which is equivalent to exit(1). But also these are a bit bad for examples because they change their semantics if people copy-n-paste them out of our main() in different functions.

   Rationale

   I assume we thought it’s a “good” and idiomatic example to use a standard library function, but it may be bad practice after all:

   • I think assert() is more for debugging (that’s why it can be turned off by defining NDEBUG). But in the case of real failures occurring in production, it’s more conservative to halt the program. And we want to be conservative in cryptographic code.
   • Another thing which is wrong with the ability to disable assertion is this entire story of unintentionally disabled side effects.
   • And if CMake disables assertions by default, it’s apparently an easy mistake to make, even without knowing about it.
   • assert() itself calls abort() but exit() is probably better practice, at least according to https://wiki.sei.cmu.edu/confluence/display/c/ERR04-C.+Choose+an+appropriate+termination+strategy.

Contributors
hebasto real-or-random

Labels
build refactor/smell