Page MenuHomePhabricator

[CMAKE] Override the flags for the Debug CMake configuration
ClosedPublic

Authored by Fabien on Mar 4 2019, 21:13.

Details

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
Branch
cmake_flags_Debug
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5146
Build 8355: Bitcoin ABC Buildbot (legacy)
Build 8354: arc lint + arc unit

Event Timeline

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 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

deadalnix requested changes to this revision.Mar 5 2019, 15:41
deadalnix added inline comments.
cmake/modules/AddCompilerFlags.cmake
43

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

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

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
Fabien edited the test plan for this revision. (Show Details)

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.

src/CMakeLists.txt
28 ↗(On Diff #7617)

Add a comment mentioning that this there to match what autotool is producing. There is really no reasons to do this.

Rebase on top of D2653 and add a comment for the weird CFLAGS

This revision is now accepted and ready to land.Mar 12 2019, 20:14
This revision was automatically updated to reflect the committed changes.