Enable plain `assert()` in non-release builds in CI
Apparently we have the assert macro disabled in CI, even in non-release builds - at least on Windows (see https://github.com/ethereum/solidity/pull/12634#discussion_r800191742). While this macro is generally discouraged in our project in favor of the solAssert() family of macros, we still do have them in some places and I think that they should not be disabled. I wouldn't disable them even if we did not have any - some of the libraries we depend on might also be using assert for example.
We do not define NDEBUG so I'm not really sure why they're disabled. Is that maybe because we default to RelWithDebInfo mode rather than Debug? The task here is to investigate, enable the macro and verify that it does actually work in non-release builds on all platforms.
Make sure the STL assertions work too. A good example is one that is triggered by dereferencing an empty optional. Verify by reverting #15909 and checking whether that assertion fails.
PR #12644 enables asserts for MSVC RelWithDevInfo by adjusting CMAKE_CXX_FLAGS_RELWITHDEBINFO:
string(REPLACE "/DNDEBUG" " " CMAKE_CXX_FLAGS_RELWITHDEBINFO "${CMAKE_CXX_FLAGS_RELWITHDEBINFO}")
Implementation differs from setting CMAKE_CXX_FLAGS for GNU/Clang though:
set(CMAKE_CXX_FLAGS_DEBUG "-O0 -g3 -DETH_DEBUG")
set(CMAKE_CXX_FLAGS_MINSIZEREL "-Os -DNDEBUG")
set(CMAKE_CXX_FLAGS_RELEASE "-O3 -DNDEBUG")
set(CMAKE_CXX_FLAGS_RELWITHDEBINFO "-O2 -g3")
It fails locally, as expected, but succeeds on CI, which runs Release.
I do support the idea to enable the standard C++ asserts. I would even consider doing it in Release, even if that sounds unorthodox.
Please check the performance. For boost multiprecision, it makes a huge difference if it is built in debug or release mode. I would not be surprised if they also use some assertions.
Does not seem to affect runtime performance in a significant way:
| CI workflow | t_win_soltest |
b_bytecode_win |
t_win_release_soltest |
|---|---|---|---|
develop |
12:18 | 10:05 | 12:32 |
| PR #12664 | 12:34 | 10:47 | 12:28 |
The non-release jobs ran slightly slower but still within the normal variability we see in CI so it might not even be actual slowdown. Even if it is, it seems to be small enough to be acceptable.
It fails locally, as expected, but succeeds on CI, which runs
Release.
I think it runs RelWithDebInfo actually (as long as we're talking about Linux runs). For example for b_ubu we do not specify build type in .circleci/config.yml so it should be RelWithDebInfo. So I guess assertions should be enabled there? Have you checked this one specifically?
It looks like we should add CMAKE_BUILD_TYPE=Release to some release jobs though, like b_ubu_release. Right now it does not specify anything so it must be running RelWithDebInfo too. Unless we're doing that on purpose?
We should check some more computation-intensive examples. Most of our tests do not need a lot of computation on bigints, but for example the linear solver will.
Reopening, because we really do need this. We have just been bitten by not having these asserts in #15871. The bug where we were dereferencing an empty optional went undetected for 4+ months, with all our builds in CI were passing just fine. The problem was found only when the maintainer of the Arch Linux package (using -D CMAKE_BUILD_TYPE=None build) ran into it in a tagged release.