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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; 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 subscriber: schancel. · View Herald Transcript
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 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.

Fabien updated this revision to Diff 7597.Mar 5 2019, 13:09

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 ↗(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
Fabien updated this revision to Diff 7608.Mar 5 2019, 18:16

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 updated this revision to Diff 7617.Mar 6 2019, 12:25

Address feedback

Fabien edited the summary of this revision. (Show Details)Mar 6 2019, 12:26
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.

deadalnix added inline comments.Mar 7 2019, 17:42
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.

Fabien updated this revision to Diff 7654.Mar 8 2019, 12:34

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

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