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

pull hebasto wants to merge 2 commits into bitcoin-core:master from hebasto:240916-build-conf changing 1 files +32 −30
  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.

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-08 09:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me