Fix build flags for tests #903
Conversation
|
Won't this make the bn_unit test take 30s again?
What's really needed is a way not to set NDEBUG in release builds. I'm
pretty sure there are still security relevant asserts() where a crash is
vastly preferable over whatever the outcome is from having them disabled.
|
|
`remove_definitions("-DNDEBUG")` in the appropriate place should do the
trick:
https://discourse.cmake.org/t/remove-definitions-ndebug-doesnt-seem-to-remove-the-macro-from-the-build/6876
|
|
This is targeted just a tests, so it shouldn't affect the speed of the underlying library being linked for the tests. You'd think that remove_definitions would work, but it doesn't work in reality. remove_definitions("-DNDEBUG") seems to have no effect anywhere that I tried it. Open to suggestions in case I missed something! Maybe I just couldn't figure out the appropriate place, but tried in the base CMakeLists.txt, the one for tests, in various places. I noticed we're using a mixture of OPENSSL_assert and assert in the libraries. If an assert is security-critical and should not be optimized out, shouldn't it be using a mechanism that is unconditional? |
|
On Sun, Aug 27, 2023 at 12:10:44PM -0700, Brent Cook wrote:
This is targeted just a tests, so it shouldn't affect the speed of the underlying library being linked for the tests.
Right. This occurred to me later, too.
You'd think that remove_definitions would work, but it doesn't work in reality. remove_definitions("-DNDEBUG") seems to have no effect anywhere that I tried it. Open to suggestions in case I missed something!
What does work (at least with CMake 3.26.3) is this:
```
# Set a default build type if none was specified
set(default_build_type "Release")
+add_definitions(-UNDEBUG)
```
What is also quite surprising with CMake is that the release settings
(including -O3) override whatever is set in CFLAGS. Isn't this wrong?
I noticed we're using a mixture of OPENSSL_assert and assert in the libraries. If an assert is security-critical and should not be optimized out, shouldn't it be using a mechanism that is unconditional?
We probably should... As you know, there's an infinite supply of messes
to be cleaned up in libcrypto...
|
|
I think this should do it. Not 100% sure if the MSVC conditional is necessary on all systems, but easier to write it this way for now. |
|
Looks good to me. Thanks!
|
|
FYI, I merged these changes with rebase, which made it look weird here. |
The default C flags for all build types other than 'Debug' sets -DNDEBUG which disables assert(), and breaks tests. This switches tests to use 'Debug' instead, reenabling asserts.