Page MenuHomePhabricator
Differential D2644

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

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

Details

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

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
teamcity added a subscriber: teamcity.Mar 4 2019, 21:13

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

teamcity added a comment.Mar 4 2019, 21:26

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.

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

Add a add_cxx_compiler_flag_with_fallback function

teamcity added a comment.Mar 5 2019, 13:10

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

teamcity added a comment.Mar 5 2019, 13:22

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

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

teamcity added a comment.Mar 5 2019, 18:17

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

teamcity added a comment.Mar 5 2019, 18:30

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

Address feedback

teamcity added a comment.Mar 6 2019, 12:26

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

Fabien edited the summary of this revision. (Show Details)Mar 6 2019, 12:26
Fabien edited the test plan for this revision. (Show Details)
Fabien added parent revisions: D2654: [CMAKE] Add an append_flag_to_variable facility, D2655: [CMAKE] Split remove_compiler_flags into C and CXX variants.
Fabien removed a parent revision: D2651: [CMAKE] Remove the -DNDEBUG flag from the release build configurations.

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.

teamcity added a comment.Mar 6 2019, 12:40

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

Harbormaster completed remote builds in B5161: Diff 7617.Mar 6 2019, 12:40
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

Fabien edited the summary of this revision. (Show Details)Mar 8 2019, 12:35
Fabien added a parent revision: D2653: [CMAKE] Fix cached variable that prevents testing for flag support.
teamcity added a comment.Mar 8 2019, 12:35

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

Fabien removed parent revisions: D2655: [CMAKE] Split remove_compiler_flags into C and CXX variants, D2654: [CMAKE] Add an append_flag_to_variable facility.Mar 8 2019, 12:35
teamcity added a comment.Mar 8 2019, 12:50

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

Harbormaster completed remote builds in B5189: Diff 7654.Mar 8 2019, 12:50
deadalnix accepted this revision.Mar 12 2019, 20:14
This revision is now accepted and ready to land.Mar 12 2019, 20:14
Closed by commit rABCeedb9d2b4d0f: [CMAKE] Override the flags for the Debug CMake configuration (authored by Fabien). · Explain WhyMar 13 2019, 12:32
This revision was automatically updated to reflect the committed changes.
Fabien added a commit: rSTAGINGeedb9d2b4d0f: [CMAKE] Override the flags for the Debug CMake configuration.Sep 18 2019, 22:42

Revision Contents
Changeset List

PathSize
cmake/
modules/
10 lines
src/
24 lines

Diff 7597

View Options

cmake/modules/AddCompilerFlags.cmake

Loading...
View Options

src/CMakeLists.txt

Loading...