Open hebasto opened 2 weeks ago
Is it correct that the first commit is the actual fix and the second commit is just a refactor?
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.
Oh ok, I missed CMAKE_C_FLAGS_DEBUG
...
I'd Concept ACK this in principle, but I think there's a simpler solution:
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."
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.
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:
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.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.
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 theCMAKE_C_FLAGS_RELWITHDEBINFO
variable before processing libsecp256k1'sCMakeLists.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 otherCMAKE_C_FLAGS_<CONFIG>
variables.This PR fixes this issue.