This removes the -DNDEBUG flag from the RelWithDebInfo configuration
while keeping the optimization flags -g -O2.
This configuration can be used by calling cmake with
-DCMAKE_BUILD_TYPE=RelWithDebInfo.
Details
Details
- Reviewers
deadalnix - Group Reviewers
Restricted Project
mkdir buildcmake && cd buildcmake cmake -GNinja .. -DCMAKE_BUILD_TYPE=RelWithDebInfo ninja -v
Check that the -g -O2 flags are set in the compile commands for both
CC and CXX compile lines.
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- cmake_flags_RelDebWithInfo
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 5133 Build 8329: Bitcoin ABC Buildbot (legacy) Build 8328: arc lint + arc unit
Event Timeline
Comment Actions
As far as I can tell, building with RelWithDebInfo as build type already use the proper flags for optimization and debug. So at least a good chunk of this patch is just redoing what cmake already does, probably adding bugs in the process.
It does looks like this patch is trying to do it all at once, which is pretty much always a bad idea. In that case, this translate in a patch of large complexity that does things that do not seems useful, and doesn't do things that seems useful (like setting corresponding flags for C).
I would recommend to get to each build type on its own and make sure it works well, then move on to a second one, making sure to factor the utilities that are in common used to get there. Changing the default build for RelWithDebInfo can also be done on its own.
| src/CMakeLists.txt | ||
|---|---|---|
| 55 ↗ | (On Diff #7569) | Add empty lines, this isn't readable. |
Comment Actions
Ok I will split.
Regarding RelWithDebInfo, the default options are correct excepted the NDEBUG that we need to remove, hence the override.
Comment Actions
If I pass -DCMAKE_BUILD_TYPE=RelWithDebInfo to cmake, I already get these flags. As far as I can tell, this code does nothing more than what cmake already does.
| src/CMakeLists.txt | ||
|---|---|---|
| 32 | Is it likely that these flags will differ in the future? Otherwise, it seems redundant to define these again for CXX | |
Comment Actions
@deadalnix This is exact, the optimization flags are the same than the default with RelDebWithInfo. The point of overriding them with the same value is to remove the -DNDEBUG which is also part of the default flags.
To sum up it moves from default -g -O2 -DNDEBUG to -g -O2. I could have tweaked it differently and removed the NDEBUG with string substitution but I found it made the code less readable.
| src/CMakeLists.txt | ||
|---|---|---|
| 32 | Yes this is redundant for the RelDebWithInfo configuration, but if you look at D2644 I plan to add a Debug configuration; Regarding the likeliness of changing these flags in the future, I don't know (yet). The secp256k1 deserves more optimization (O3) but I'll certainly put it into its own CMakeLists.txt. | |
| src/CMakeLists.txt | ||
|---|---|---|
| 32 | Then you want a patch to replace -g with -g3 if/when it is supported, regardless of the build mode used. This is what the patch should do. | |
| src/CMakeLists.txt | ||
|---|---|---|
| 32 | Also, looking at what autotool does, it doesn't set the g3 flag on that build mode. | |