-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FIXUP] cmake: Rework compile/link flags summary #93
Conversation
This change was requested by @fanquake in #82. @TheCharlatan gave his 👍 . |
Tested on a macOS system with Cross compiling ....................... FALSE
Build configuration ................... RelWithDebInfo
Preprocessor defined macros ........... -DMAC_OSX -D -D -D -D -D -DHAVE_CONFIG_H
C compiler ............................ /Library/Developer/CommandLineTools/usr/bin/cc
C compiler flags ...................... -O2 -g -Wstack-protector -fstack-protector-all -mbranch-protection=bti
C++ compiler .......................... /Library/Developer/CommandLineTools/usr/bin/c++
C++ compiler flags .................... -O2 -g -Wstack-protector -fstack-protector-all -mbranch-protection=bti
Linker flags .......................... -Wl,-dead_strip -Wl,-dead_strip_dylibs -Wl,-headerpad_max_install_names -Wl,-fixup_chains What is It'd be nice if the compiler output was the actual compiler, i.e On two Linux machines, one aarch64 Fedora, the other x86_64 Ubuntu, both configured with C compiler ............................ /usr/lib64/ccache/cc
C compiler flags ...................... -O2 -g -fno-extended-identifiers -fstack-reuse=none -Wstack-protector -fstack-protector-all -fstack-clash-protection -mbranch-protection=bti
C++ compiler .......................... /usr/lib64/ccache/c++
...
Use ccache for compiling .............. ON but the other shows me: C compiler ............................ /usr/bin/cc
C compiler flags ...................... -O2 -g -fno-extended-identifiers -fstack-reuse=none -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection
C++ compiler .......................... /usr/bin/c++
...
Use ccache for compiling .............. ON Is there a reason for the difference? |
aafd8d3
to
184c663
Compare
I did my best efforts to address @fanquake's comment.
I would assume it depends on how the |
17f4870
to
6367599
Compare
184c663
to
7c82984
Compare
Rebased. |
7c82984
to
2890ef1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 2890ef1
Tested on Ubuntu 22.04.
cmake -B build -DCMAKE_BUILD_TYPE=Debug
- Before:
Cross compiling ....................... FALSE
Preprocessor defined macros ...........
C compiler ............................ /usr/bin/cc
CFLAGS ................................
C++ compiler .......................... /usr/bin/c++
CXXFLAGS ..............................
Common compile options ................ -fno-extended-identifiers -fstack-reuse=none -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection
Common link options ................... -fno-extended-identifiers -fstack-reuse=none -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code
Linker flags for executables ..........
Linker flags for shared libraries .....
Build type (configuration):
- CMAKE_BUILD_TYPE ................... Debug
- Preprocessor defined macros ........ DEBUG DEBUG_LOCKORDER DEBUG_LOCKCONTENTION RPC_DOC_CHECK ABORT_ON_FAILED_ASSUME
- CFLAGS ............................. -O0 -g3
- CXXFLAGS ........................... -O0 -g3 -ftrapv
- LDFLAGS for executables ............
- LDFLAGS for shared libraries .......
Use assembly routines ................. ON
Attempt to harden executables ......... ON
Treat compiler warnings as errors ..... OFF
Use ccache for compiling .............. ON
- After:
Cross compiling ....................... FALSE
Build configuration ................... Debug
Preprocessor defined macros ........... -DHAVE_CONFIG_H -DDEBUG -DDEBUG_LOCKORDER -DDEBUG_LOCKCONTENTION -DRPC_DOC_CHECK -DABORT_ON_FAILED_ASSUME
C compiler ............................ GNU 11.4.0, /usr/bin/cc
C compiler flags ...................... -O0 -g3 -fno-extended-identifiers -fstack-reuse=none -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection
C++ compiler .......................... GNU 11.4.0, /usr/bin/c++
C++ compiler flags .................... -O0 -g3 -ftrapv -fno-extended-identifiers -fstack-reuse=none -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection
Linker flags .......................... -fno-extended-identifiers -fstack-reuse=none -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code
Use assembly routines ................. ON
Attempt to harden executables ......... ON
Treat compiler warnings as errors ..... OFF
Use ccache for compiling .............. ON
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK d8d9ced
- With multi config:
cmake -B build -G "Ninja Multi-Config"
...
Cross compiling ....................... FALSE
Available build configurations ........ RelWithDebInfo, Debug, Release
Default build configuration ........... RelWithDebInfo
Preprocessor defined macros ........... -DHAVE_CONFIG_H
C compiler ............................ GNU 11.4.0, /usr/bin/cc
C compiler flags ...................... -O2 -g -fno-extended-identifiers -fstack-reuse=none -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection
C++ compiler .......................... GNU 11.4.0, /usr/bin/c++
C++ compiler flags .................... -O2 -g -fno-extended-identifiers -fstack-reuse=none -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection
Linker flags .......................... -fno-extended-identifiers -fstack-reuse=none -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code
Use assembly routines ................. ON
Attempt to harden executables ......... ON
Treat compiler warnings as errors ..... OFF
Use ccache for compiling .............. ON
...
With this PR on multi-config build we lose information on what the flags would be if a build configuration other than the default is picked? Before this PR for example the summary told me that I prefer this new format for single config build configurations but not as much for multi-config. With the PR I get -DHAVE_CONFIG_H in the preprocessor macro line but it's not there in |
d8d9ced
to
d2e335e
Compare
When using multi-config generators, you can specify
I doubt that we want introduce a such an inconsistency.
Yes, that is correct. See the diff in the |
Friendly ping @TheCharlatan @vasild @fanquake :) |
I agree with @m3dwards that it is easier to print all configs right away and with @fanquake that printing "CFLAGS: x, Debug CFLAGS: y" is confusing - it is unclear which one wins if x and y are mutually exclusive and also for me it was not clear whether y replaces x or is merged with it. Consider the following patch, it resolves both complaints: [patch] print all without a default beforehanddiff --git i/cmake/module/FlagsSummary.cmake w/cmake/module/FlagsSummary.cmake
index bec15f489d..0a3aed17f9 100644
--- i/cmake/module/FlagsSummary.cmake
+++ w/cmake/module/FlagsSummary.cmake
@@ -44,39 +44,40 @@ function(flags_summary)
if(CMAKE_GENERATOR MATCHES "Visual Studio")
set(default_config "Debug")
else()
list(GET CMAKE_CONFIGURATION_TYPES 0 default_config)
endif()
message("Default build configuration ........... ${default_config}")
- string(TOUPPER "${default_config}" config_uppercase)
- if(DEFINED CMAKE_BUILD_TYPE)
- message("Flags for configuration \"${CMAKE_BUILD_TYPE}\":")
- string(TOUPPER "${CMAKE_BUILD_TYPE}" config_uppercase)
- endif()
else()
message("Build configuration ................... ${CMAKE_BUILD_TYPE}")
- string(TOUPPER "${CMAKE_BUILD_TYPE}" config_uppercase)
endif()
- string(STRIP "${definitions_ALL} ${definitions_${config_uppercase}}" combined_cpp_flags)
- normalize_preprocessor_definitions(combined_cpp_flags)
- message("Preprocessor defined macros ........... ${combined_cpp_flags}")
-
message("C compiler ............................ ${CMAKE_C_COMPILER_ID} ${CMAKE_C_COMPILER_VERSION}, ${CMAKE_C_COMPILER}")
- string(STRIP "${CMAKE_C_FLAGS} ${CMAKE_C_FLAGS_${config_uppercase}}" combined_c_flags)
+ message("C++ compiler .......................... ${CMAKE_CXX_COMPILER_ID} ${CMAKE_CXX_COMPILER_VERSION}, ${CMAKE_CXX_COMPILER}")
+
list(JOIN DEPENDS_C_COMPILER_FLAGS " " depends_c_flags)
- string(STRIP "${combined_c_flags} ${depends_c_flags}" combined_c_flags)
+ list(JOIN DEPENDS_CXX_COMPILER_FLAGS " " depends_cxx_flags)
get_target_interface(common_compile_options core_interface COMPILE_OPTIONS)
- string(STRIP "${combined_c_flags} ${common_compile_options}" combined_c_flags)
- message("C compiler flags ...................... ${combined_c_flags}")
- message("C++ compiler .......................... ${CMAKE_CXX_COMPILER_ID} ${CMAKE_CXX_COMPILER_VERSION}, ${CMAKE_CXX_COMPILER}")
- string(STRIP "${CMAKE_CXX_FLAGS} ${CMAKE_CXX_FLAGS_${config_uppercase}}" combined_cxx_flags)
- list(JOIN DEPENDS_CXX_COMPILER_FLAGS " " depends_cxx_flags)
- string(STRIP "${combined_cxx_flags} ${depends_cxx_flags}" combined_cxx_flags)
- string(STRIP "${combined_cxx_flags} ${common_compile_options}" combined_cxx_flags)
- message("C++ compiler flags .................... ${combined_cxx_flags}")
+ foreach(config IN LISTS CMAKE_CONFIGURATION_TYPES)
+ message("'${config}' build configuration:")
+ string(TOUPPER "${config}" config_uppercase)
+
+ string(STRIP "${definitions_ALL} ${definitions_${config_uppercase}}" combined_cpp_flags)
+ normalize_preprocessor_definitions(combined_cpp_flags)
+ message(" Preprocessor defined macros ......... ${combined_cpp_flags}")
+
+ string(STRIP "${CMAKE_C_FLAGS} ${CMAKE_C_FLAGS_${config_uppercase}}" combined_c_flags)
+ string(STRIP "${combined_c_flags} ${depends_c_flags}" combined_c_flags)
+ string(STRIP "${combined_c_flags} ${common_compile_options}" combined_c_flags)
+ message(" C compiler flags .................... ${combined_c_flags}")
+
+ string(STRIP "${CMAKE_CXX_FLAGS} ${CMAKE_CXX_FLAGS_${config_uppercase}}" combined_cxx_flags)
+ string(STRIP "${combined_cxx_flags} ${depends_cxx_flags}" combined_cxx_flags)
+ string(STRIP "${combined_cxx_flags} ${common_compile_options}" combined_cxx_flags)
+ message(" C++ compiler flags .................. ${combined_cxx_flags}")
+ endforeach()
get_target_interface(common_link_options core_interface LINK_OPTIONS)
string(STRIP "${CMAKE_EXE_LINKER_FLAGS} ${common_link_options}" combined_linker_flags)
message("Linker flags .......................... ${combined_linker_flags}")
endfunction() Output:
or
|
f4f8a6c
to
a3a3d10
Compare
a3a3d10
to
484004b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 484004b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 4e52664
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 4e52664 tested on WSL Ubuntu 22.04.
Built both single and multi config both compiled properly
Addressed the recent @vasild's comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 628cae3
cmake/module/FlagsSummary.cmake
Outdated
indent_message("Preprocessor defined macros ..........." "${definitions}" ${indent_num}) | ||
|
||
string(STRIP "${CMAKE_C_FLAGS} ${CMAKE_C_FLAGS_${config_uppercase}}" combined_c_flags) | ||
if(CMAKE_INTERPROCEDURAL_OPTIMIZATION) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(CMAKE_INTERPROCEDURAL_OPTIMIZATION)
if(CMAKE_POSITION_INDEPENDENT_CODE)
if(CMAKE_CXX_LINK_PIE_SUPPORTED)
I bought this up here: #93 (comment), and was hoping there would be a more maintainable solution than just adding more conditionals, because this seems fragile, and hard to maintain.
For example, on my Alpine box, which uses a recent CMake, if I configure with cmake -B build -DCMAKE_LINKER_TYPE=LLD
, -fuse-ld=lld
isn't shown in the link flags summary:
Linker flags -O2 -g -fstack-reuse=none -fstack-protector-all -fstack-clash-protection -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -fPIE -pie
even though it is used during linking:
/usr/bin/c++ -O2 -g -fstack-reuse=none -fstack-protector-all -fstack-clash-protection -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -fuse-ld=lld -fPIE -pie <snip>
The approach used here basically requires us to maintain awareness of all relevant CMake options across all versions, and ensure that we are always keeping our own build system updated to accomodate them (as well as backport all these changes, so newer CMake versions work properly with older branches). I've been picking out examples here, but I'm sure there are more relevant CMake options that are still missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this moment, I cannot suggest a more maintainable solution. Maybe someone could suggest it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative has been suggested in #218. It guarantees 100% accuracy, but still relies on CMake's implementation details.
e2f2f23 fixup! cmake: Add `HARDENING` option (Hennadii Stepanov) Pull request description: During testing #93, a few issues were [noticed](#93 (comment)): - > `-mbranch-protection=bti` shouldn't be in C flags The other change simplifies the summary code in #93. ACKs for top commit: m3dwards: ACK e2f2f23 Tree-SHA512: 386e3c180dbe68d76602bea12737651bbec398bfd5d75c32e3b235d418e60322e1c82ce08c4fb719e205705c5af2895d78d17d6907d8b370b5225dd8d4f0604b
Rebased. |
I agree. As someone with a fresh look (I had opened #221 without being aware of this PR...), I doubt that this is the right approach, despite the many ACKs. Replicating CMake's flag logic is difficult and requires a lot of maintenance. And it somehow defeats the purpose: CMake's philosophy is that handles some of the logic for us, and we don't have to bother with it. I think the appropriate approach is to figure out the flags is run the build system and observe what it does. IIUC #218 is a step in this direction, but it's even more complex (more code and also more magic). This is "just" for a nice summary. Taking a step back, I wonder if it's worth keeping the summary in this form at all. Alternatively, we could print a best-effort summary, which includes most of the flag variable and maybe some of the most common abstract options, e.g., |
On our call yesterday I think I largely agreed with @real-or-random here. I proposed that we:
As for the note, how about:
|
This change mirrors the master branch behavior.
core_depends_release_interface -> core_interface_relwithdebinfo core_depends_debug_interface -> core_interface_debug There is no need to refer to depends.
Reworked per the recent discussions and comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 5c786ec
Tested and works as expected. -DAPPEND_...
are present in the summary. Compared the summary flags with the actual flags. The actual flags have some additions which are not printed in the summary:
C++
:
-DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION
-D_THREAD_SAFE
-I/SOURCE/src
-I/SOURCE/src/leveldb/include
-I/SOURCE/src/minisketch/include
-I/SOURCE/src/univalue/include
-I/BUILD/src
-fcolor-diagnostics # because of -DCMAKE_COLOR_DIAGNOSTICS=ON
-isystem /usr/local/include
-pthread
-std=c++20
Linking
:
-Wl,-rpath,/usr/local/lib:
-pthread
CMake abstracts |
4cd15f1 cmake: Remove unused `TristateOption` module (Hennadii Stepanov) 686a731 cmake [KILL 3-STATE]: Switch `WITH_CCACHE` to boolean (Hennadii Stepanov) Pull request description: This PR makes `WITH_CCACHE` an opportunistic boolean option with the default value `ON`. Also the build system learned to tell ccache in the [masquerade](https://ccache.dev/manual/4.9.1.html#_run_modes) mode, which addresses this #93 (comment). The `TristateOption` module has been deleted as planned. ACKs for top commit: m3dwards: ACK 4cd15f1 Tree-SHA512: 6df4d0b9ca06a2362596cfa972d5914ef95d3def15ba51a30745933a8a9bab212d02a7fc22ca9f8a564874aca853c3a9203b421f5e2dd7a1121f7cce7aaa968d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 6068714
-std=
is now added to the summary.
It is somewhat strange that we compile C in -std=c90
and C++ in -std=c++20
mode, but I guess they do not necessary have to be the same. Btw that naming has a y2k problem ;-)
All flags have the only line to be displayed in the "Configure summary" now. This change is expected to simplify parsing the summary.
Here are output examples:
Closes #221