Details

Reviewers

Group Reviewers

Restricted Project

Commits

rSTAGINGeedb9d2b4d0f: [CMAKE] Override the flags for the Debug CMake configuration
rABCeedb9d2b4d0f: [CMAKE] Override the flags for the Debug CMake configuration

Summary

This diffs overrides the default flags for the Debug build type,
setting the optimization to -g3 -Og (or their fallbacks).

Depends on D2653

Test Plan

mkdir buildcmake && cd buildcmake
cmake -GNinja .. -DCMAKE_BUILD_TYPE=Debug
ninja -v

Check that the -g3 -Og flags (or their fallbacks) flags are set in the
compile commands for the CXX compile lines. Check that the -g -O2
flags are set in the compile commands for the CC compile lines.

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Fabien created this revision.Mar 4 2019, 21:13

Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 4 2019, 21:13

Herald added a reviewer: deadalnix. · View Herald Transcript

Herald added a subscriber: schancel. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5135: Diff 7583.Mar 4 2019, 21:26

deadalnix requested changes to this revision.Mar 4 2019, 21:55

deadalnix added inline comments.

src/CMakeLists.txt
33 ↗	(On Diff #7583)	The RELWITHDEBINFO_G_CXX_FLAG and it's O friend are now useless if you change this this way.
45 ↗	(On Diff #7583)	Looks like you'd need some kind of function or macro to do this. So you can do it for both C and C++. add_flag_with_fallback or something.

This revision now requires changes to proceed.Mar 4 2019, 21:55

Fabien mentioned this in D2639: [CMAKE] Override the flags for the RelWithDebInfo build configuration.Mar 5 2019, 07:59

Fabien added a subscriber: jasonbcox.Mar 5 2019, 08:06

Fabien added inline comments.

src/CMakeLists.txt
33 ↗	(On Diff #7583)	I kept the C and CXX flags separated deliberately so one can update them independently. @jasonbcox had the same comment in D2639, do you think they should be merged ?
45 ↗	(On Diff #7583)	I will add a function or macro. The C flags remain `-g -O2` here to get the same behavior than `configure --enable-debug`.

Add a add_cxx_compiler_flag_with_fallback function

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5146: Diff 7597.Mar 5 2019, 13:22

deadalnix requested changes to this revision.Mar 5 2019, 15:41

deadalnix added inline comments.

cmake/modules/AddCompilerFlags.cmake
43 ↗	(On Diff #7597)	You probably want to check that the fallback is supported. You also want to make sure you remove the fallback flag when you add the other one.
src/CMakeLists.txt
56 ↗	(On Diff #7597)	This clearly is not at the right place.

This revision now requires changes to proceed.Mar 5 2019, 15:41

Make the add_cxx_compiler_flag_with_fallback function more robust as per feedback

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5154: Diff 7608.Mar 5 2019, 18:30

Fabien edited the summary of this revision. (Show Details)Mar 5 2019, 19:58

Fabien added a parent revision: D2651: [CMAKE] Remove the -DNDEBUG flag from the release build configurations.

Fabien removed a parent revision: D2639: [CMAKE] Override the flags for the RelWithDebInfo build configuration.

deadalnix requested changes to this revision.Mar 5 2019, 21:48

deadalnix added inline comments.

cmake/modules/AddCompilerFlags.cmake
50 ↗	(On Diff #7608)	You can use `remove_compiler_flags` or create a cxx only variant if required and use that.
52 ↗	(On Diff #7608)	Pick a proper name.
53 ↗	(On Diff #7608)	Is that required ?
67 ↗	(On Diff #7608)	Use add_cxx_compiler_flag in there.
src/CMakeLists.txt
35 ↗	(On Diff #7608)	There is no point setting the temporaries and appending them this way.
37 ↗	(On Diff #7608)	This can be done on its own, as it's not dependent on the rest of the patch and would already be green by now.

This revision now requires changes to proceed.Mar 5 2019, 21:48

Address feedback

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

The DEBUG and DEBUG_LOCKORDER additions will be added in another diff

cmake/modules/AddCompilerFlags.cmake
50 ↗	(On Diff #7608)	The cxx variant is created in D2655
53 ↗	(On Diff #7608)	Yes, the value is cached internally, it even deserves a larger fix. This base is done in D2653, I will update on top of that.
67 ↗	(On Diff #7608)	Here you want to specifically modify the variable passed as an argument. I add a facility to simplify this part in D2654 and will rebase on top.