Skip to content
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.

Sign up for GitHub

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

cmake: Add compiler diagnostic flags #84

Merged
merged 4 commits into from
Mar 11, 2024
Merged

cmake: Add compiler diagnostic flags #84

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
172b7f5
cmake: Add compiler diagnostic flags
hebasto Mar 8, 2024
4d9c08f
FIXUP: Introduce core_base_interface
hebasto Feb 15, 2024
f67e9a7
test: Fix MSVC warning C4101 "unreferenced local variable"
hebasto Feb 26, 2024
695a5ca
fixup! ci: Test CMake edge cases
hebasto Jan 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions .github/workflows/cmake.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,8 @@ jobs:
c_compiler: 'clang-14 -m32'
cxx_compiler: 'clang++-14 -m32'
depends_options: ''
# For -Wno-error=unreachable-code, please refer to https://github.com/bitcoin/bitcoin/issues/29334
configure_env: 'env CXXFLAGS="-Wno-error=unreachable-code"'
Comment on lines +275 to +276
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this to silence a particular issue, obviously has the problem that if new issues like that are created, then they will be silenced as well (on this platform). 👎 I understand that this is the best thing to do if there is resistance to change the source code.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The source code was changed to solve other issues, and so far no-one has shown why this version of the compiler isn't broken and/or emitting false positives (why is only the older version emitting these warnings?). Generally we don't refactor source code to work around warnings from compilers, let alone older compilers. Especially not when it's just in pursuit of using -Werror.

configure_options: '-DWERROR=ON'
- name: 'MinGW-w64'
triplet: 'x86_64-w64-mingw32'
Expand Down Expand Up @@ -343,7 +345,7 @@ jobs:

- name: Generate build system
run: |
cmake -B build --toolchain depends/${{ matrix.host.triplet }}/share/toolchain.cmake ${{ matrix.host.configure_options }}
${{ matrix.host.configure_env }} cmake -B build --toolchain depends/${{ matrix.host.triplet }}/share/toolchain.cmake ${{ matrix.host.configure_options }}

- name: Build
run: |
Expand Down Expand Up @@ -436,7 +438,7 @@ jobs:

- name: Generate build system
run: |
cmake -B build --preset ${{ matrix.conf.preset }}
cmake -B build --preset ${{ matrix.conf.preset }} -DWERROR=ON

- name: Save vcpkg binary cache
uses: actions/cache/save@v4
Expand Down Expand Up @@ -503,6 +505,8 @@ jobs:
- name: 'Xcode 14.3'
id: 'xcode-14.3'
path: '/Applications/Xcode_14.3.app'
# For -Wno-error=unreachable-code, please refer to https://github.com/bitcoin/bitcoin/issues/29334
configure_env: 'env CXXFLAGS="-Wno-error=unreachable-code"'
- name: 'Xcode 15.2'
id: 'xcode-15.2'
path: '/Applications/Xcode_15.2.app'
Expand Down Expand Up @@ -538,7 +542,7 @@ jobs:

- name: Generate build system
run: |
cmake -B build
${{ matrix.xcode.configure_env }} cmake -B build -DWERROR=ON

- name: Build
run: |
Expand Down
119 changes: 84 additions & 35 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,12 @@ unset(check_pie_output)
# The core_interface library aims to encapsulate common build flags.
# It is intended to be a usage requirement for all other targets.
add_library(core_interface INTERFACE)
# The core_base_interface library is a subset of the core_interface
# designated to be used with subtrees. In particular, it does not
# include the warn_interface as subtree's warnings are not fixable
# in our tree.
add_library(core_base_interface INTERFACE)
target_link_libraries(core_interface INTERFACE core_base_interface)

if(FUZZ)
message(WARNING "FUZZ=ON will disable all other targets and force BUILD_FUZZ_BINARY=ON.")
Expand Down Expand Up @@ -150,7 +156,7 @@ if(WIN32)
- A run-time library must be specified explicitly using _MT definition.
]=]

target_compile_definitions(core_interface INTERFACE
target_compile_definitions(core_base_interface INTERFACE
_WIN32_WINNT=0x0601
_WIN32_IE=0x0501
WIN32_LEAN_AND_MEAN
Expand All @@ -166,7 +172,7 @@ if(WIN32)
set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>${msvc_library_linkage}")
unset(msvc_library_linkage)

target_compile_definitions(core_interface INTERFACE
target_compile_definitions(core_base_interface INTERFACE
_UNICODE;UNICODE
)
target_compile_options(core_interface INTERFACE
Expand All @@ -176,66 +182,55 @@ if(WIN32)
# Improve parallelism in MSBuild.
# See: https://devblogs.microsoft.com/cppblog/improved-parallelism-in-msbuild/.
list(APPEND CMAKE_VS_GLOBALS "UseMultiToolTask=true")

try_append_cxx_flags("/W3" TARGET core_interface SKIP_LINK)
try_append_cxx_flags("/wd4018" TARGET core_interface SKIP_LINK)
try_append_cxx_flags("/wd4244" TARGET core_interface SKIP_LINK)
try_append_cxx_flags("/wd4267" TARGET core_interface SKIP_LINK)
try_append_cxx_flags("/wd4715" TARGET core_interface SKIP_LINK)
try_append_cxx_flags("/wd4805" TARGET core_interface SKIP_LINK)
target_compile_definitions(core_interface INTERFACE
_CRT_SECURE_NO_WARNINGS
_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING
)
endif()

if(MINGW)
target_compile_definitions(core_interface INTERFACE
target_compile_definitions(core_base_interface INTERFACE
WIN32
_WINDOWS
_MT
)
# Avoid the use of aligned vector instructions when building for Windows.
# See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54412.
try_append_cxx_flags("-Wa,-muse-unaligned-vector-move" TARGET core_interface SKIP_LINK)
try_append_linker_flag("-static" TARGET core_interface)
try_append_cxx_flags("-Wa,-muse-unaligned-vector-move" TARGET core_base_interface SKIP_LINK)
try_append_linker_flag("-static" TARGET core_base_interface)
# We require Windows 7 (NT 6.1) or later.
try_append_linker_flag("-Wl,--major-subsystem-version,6" TARGET core_interface)
try_append_linker_flag("-Wl,--minor-subsystem-version,1" TARGET core_interface)
try_append_linker_flag("-Wl,--major-subsystem-version,6" TARGET core_base_interface)
try_append_linker_flag("-Wl,--minor-subsystem-version,1" TARGET core_base_interface)
endif()
endif()

# Use 64-bit off_t on 32-bit Linux.
if (CMAKE_SYSTEM_NAME STREQUAL "Linux" AND CMAKE_SIZEOF_VOID_P EQUAL 4)
# Ensure 64-bit offsets are used for filesystem accesses for 32-bit compilation.
target_compile_definitions(core_interface INTERFACE
target_compile_definitions(core_base_interface INTERFACE
_FILE_OFFSET_BITS=64
)
endif()

if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
target_compile_definitions(core_interface INTERFACE
target_compile_definitions(core_base_interface INTERFACE
MAC_OSX
)
# These flags are specific to ld64, and may cause issues with other linkers.
# For example: GNU ld will interpret -dead_strip as -de and then try and use
# "ad_strip" as the symbol for the entry point.
try_append_linker_flag("-Wl,-dead_strip" TARGET core_interface)
try_append_linker_flag("-Wl,-dead_strip_dylibs" TARGET core_interface)
try_append_linker_flag("-Wl,-headerpad_max_install_names" TARGET core_interface)
try_append_linker_flag("-Wl,-dead_strip" TARGET core_base_interface)
try_append_linker_flag("-Wl,-dead_strip_dylibs" TARGET core_base_interface)
try_append_linker_flag("-Wl,-headerpad_max_install_names" TARGET core_base_interface)
endif()

if(CMAKE_CROSSCOMPILING)
target_compile_definitions(core_interface INTERFACE ${DEPENDS_COMPILE_DEFINITIONS})
target_compile_options(core_interface INTERFACE "$<$<COMPILE_LANGUAGE:C>:${DEPENDS_C_COMPILER_FLAGS}>")
target_compile_options(core_interface INTERFACE "$<$<COMPILE_LANGUAGE:CXX>:${DEPENDS_CXX_COMPILER_FLAGS}>")
target_compile_definitions(core_base_interface INTERFACE ${DEPENDS_COMPILE_DEFINITIONS})
target_compile_options(core_base_interface INTERFACE "$<$<COMPILE_LANGUAGE:C>:${DEPENDS_C_COMPILER_FLAGS}>")
target_compile_options(core_base_interface INTERFACE "$<$<COMPILE_LANGUAGE:CXX>:${DEPENDS_CXX_COMPILER_FLAGS}>")
endif()

include(AddThreadsIfNeeded)
add_threads_if_needed()

add_library(sanitizing_interface INTERFACE)
target_link_libraries(core_interface INTERFACE sanitizing_interface)
target_link_libraries(core_base_interface INTERFACE sanitizing_interface)
if(SANITIZERS)
# First check if the compiler accepts flags. If an incompatible pair like
# -fsanitize=address,thread is used here, this check will fail. This will also
Expand Down Expand Up @@ -282,11 +277,65 @@ include(cmake/leveldb.cmake)
include(cmake/minisketch.cmake)
include(cmake/secp256k1.cmake)

string(STRIP "${CMAKE_CXX_FLAGS}" cxx_flags)
string(STRIP "${CMAKE_CXX_FLAGS_INIT}" cxx_flags_init)
if(cxx_flags STREQUAL cxx_flags_init)
Comment on lines +280 to +282
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocker IMO, just mentioning: with this change it is no longer possible to set your own flags on the command line like cmake -DCMAKE_CXX_FLAGS=-myflag and have stuff from try_append_cxx_flags() from inside CMakeLists.txt appended to them.

To achieve this either one of the following worked for me:

export CXXFLAGS="-myflag" (and then run cmake)
or
-DCMAKE_CXX_COMPILER="c++;-myflag"

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-DCMAKE_CXX_COMPILER="c++;-myflag"

This is definitely not someting that we would want (or encourage others) to do.

# CMAKE_CXX_FLAGS are not overridden.
add_library(warn_interface INTERFACE)
target_link_libraries(core_interface INTERFACE warn_interface)
if(MSVC)
try_append_cxx_flags("/W3" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("/wd4018" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("/wd4244" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("/wd4267" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("/wd4715" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("/wd4805" TARGET warn_interface SKIP_LINK)
target_compile_definitions(warn_interface INTERFACE
_CRT_SECURE_NO_WARNINGS
_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING
)
else()
try_append_cxx_flags("-Wall" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("-Wextra" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("-Wgnu" TARGET warn_interface SKIP_LINK)
# Some compilers will ignore -Wformat-security without -Wformat, so just combine the two here.
try_append_cxx_flags("-Wformat;-Wformat-security" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("-Wvla" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("-Wshadow-field" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("-Wthread-safety" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("-Wloop-analysis" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("-Wredundant-decls" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("-Wunused-member-function" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("-Wdate-time" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("-Wconditional-uninitialized" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("-Wduplicated-branches" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("-Wduplicated-cond" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("-Wlogical-op" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("-Woverloaded-virtual" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("-Wsuggest-override" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("-Wimplicit-fallthrough" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("-Wunreachable-code" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("-Wdocumentation" TARGET warn_interface SKIP_LINK)

# Some compilers (gcc) ignore unknown -Wno-* options, but warn about all
# unknown options if any other warning is produced. Test the -Wfoo case, and
# set the -Wno-foo case if it works.
try_append_cxx_flags("-Wunused-parameter" TARGET warn_interface SKIP_LINK
IF_CHECK_PASSED "-Wno-unused-parameter"
)
try_append_cxx_flags("-Wself-assign" TARGET warn_interface SKIP_LINK
IF_CHECK_PASSED "-Wno-self-assign"
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing -Wno-deprecated-copy ?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It is not required anymore as CMake includes imported packages headers as system ones, which effectively suppresses external warnings.

endif()
endif()
unset(cxx_flags)
unset(cxx_flags_init)

include(ProcessConfigurations)
set_default_config(RelWithDebInfo)

# Redefine configuration flags.
target_compile_definitions(core_interface INTERFACE
target_compile_definitions(core_base_interface INTERFACE
$<$<CONFIG:Debug>:DEBUG>
$<$<CONFIG:Debug>:DEBUG_LOCKORDER>
$<$<CONFIG:Debug>:DEBUG_LOCKCONTENTION>
Expand Down Expand Up @@ -322,17 +371,17 @@ include(cmake/optional_qt.cmake)
include(cmake/optional.cmake)

# Don't allow extended (non-ASCII) symbols in identifiers. This is easier for code review.
try_append_cxx_flags("-fno-extended-identifiers" TARGET core_interface)
try_append_cxx_flags("-fno-extended-identifiers" TARGET core_base_interface)

# Currently all versions of gcc are subject to a class of bugs, see the
# gccbug_90348 test case (only reproduces on GCC 11 and earlier) and
# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111843. To work around that, set
# -fstack-reuse=none for all gcc builds. (Only gcc understands this flag).
try_append_cxx_flags("-fstack-reuse=none" TARGET core_interface)
try_append_cxx_flags("-fstack-reuse=none" TARGET core_base_interface)

if(HARDENING)
add_library(hardening_interface INTERFACE)
target_link_libraries(core_interface INTERFACE hardening_interface)
target_link_libraries(core_base_interface INTERFACE hardening_interface)
if(MSVC)
try_append_linker_flag("/DYNAMICBASE" TARGET hardening_interface)
try_append_linker_flag("/HIGHENTROPYVA" TARGET hardening_interface)
Expand Down Expand Up @@ -375,7 +424,7 @@ endif()
if(REDUCE_EXPORTS)
set(CMAKE_CXX_VISIBILITY_PRESET hidden)
set(CMAKE_VISIBILITY_INLINES_HIDDEN 1)
try_append_linker_flag("-Wl,--exclude-libs,ALL" TARGET core_interface)
try_append_linker_flag("-Wl,--exclude-libs,ALL" TARGET core_base_interface)
endif()

if(WERROR)
Expand All @@ -384,7 +433,7 @@ if(WERROR)
else()
set(werror_flag "-Werror")
endif()
try_append_cxx_flags(${werror_flag} TARGET core_interface RESULT_VAR compiler_supports_werror SKIP_LINK)
try_append_cxx_flags(${werror_flag} TARGET core_base_interface SKIP_LINK RESULT_VAR compiler_supports_werror)
if(NOT compiler_supports_werror)
message(FATAL_ERROR "WERROR set but ${werror_flag} is not usable.")
endif()
Expand All @@ -410,7 +459,8 @@ setup_split_debug_script()
add_maintenance_targets()
add_windows_deploy_target()

get_target_property(definitions core_interface INTERFACE_COMPILE_DEFINITIONS)
include(GetTargetInterface)
get_target_interface(definitions core_interface COMPILE_DEFINITIONS)
separate_by_configs(definitions)

message("\n")
Expand Down Expand Up @@ -452,7 +502,6 @@ message("C++ compiler .......................... ${CMAKE_CXX_COMPILER}")
list(JOIN DEPENDS_CXX_COMPILER_FLAGS " " depends_cxx_flags)
string(STRIP "${CMAKE_CXX_FLAGS} ${depends_cxx_flags}" combined_cxx_flags)
message("CXXFLAGS .............................. ${combined_cxx_flags}")
include(GetTargetInterface)
get_target_interface(common_compile_options core_interface COMPILE_OPTIONS)
message("Common compile options ................ ${common_compile_options}")
get_target_interface(common_link_options core_interface LINK_OPTIONS)
Expand Down
2 changes: 1 addition & 1 deletion cmake/crc32c.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,4 +112,4 @@ if(HAVE_ARM64_CRC32C)
)
endif()

target_link_libraries(crc32c PRIVATE core_interface)
target_link_libraries(crc32c PRIVATE core_base_interface)
2 changes: 1 addition & 1 deletion cmake/leveldb.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ else()
endif()

target_link_libraries(leveldb PRIVATE
core_interface
core_base_interface
nowarn_leveldb_interface
crc32c
)
4 changes: 2 additions & 2 deletions cmake/minisketch.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ if(HAVE_CLMUL)
target_compile_options(minisketch_clmul PRIVATE ${CLMUL_CXXFLAGS})
target_link_libraries(minisketch_clmul
PRIVATE
core_interface
core_base_interface
minisketch_common
)
endif()
Expand All @@ -79,7 +79,7 @@ target_include_directories(minisketch

target_link_libraries(minisketch
PRIVATE
core_interface
core_base_interface
minisketch_common
$<TARGET_NAME_IF_EXISTS:minisketch_clmul>
)
4 changes: 3 additions & 1 deletion cmake/module/GetTargetInterface.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ include_guard(GLOBAL)
function(get_target_interface var target property)
get_target_property(result ${target} INTERFACE_${property})
if(result)
string(GENEX_STRIP "${result}" result)
if(NOT ${property} STREQUAL "COMPILE_DEFINITIONS")
string(GENEX_STRIP "${result}" result)
endif()
list(JOIN result " " result)
else()
set(result)
Expand Down
2 changes: 1 addition & 1 deletion cmake/secp256k1.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,4 @@ if(MSVC)
)
endif()

target_link_libraries(secp256k1 PRIVATE core_interface)
target_link_libraries(secp256k1 PRIVATE core_base_interface)
4 changes: 4 additions & 0 deletions src/qt/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,10 @@ target_include_directories(bitcoinqt
PUBLIC
$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/src>
)
set_property(SOURCE macnotificationhandler.mm
# Ignore warnings "'NSUserNotificationCenter' is deprecated: first deprecated in macOS 11.0".
APPEND PROPERTY COMPILE_OPTIONS -Wno-deprecated-declarations
)
target_link_libraries(bitcoinqt
PUBLIC
Qt5::Widgets
Expand Down
2 changes: 1 addition & 1 deletion src/univalue/test/object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
try { \
(stmt); \
assert(0 && "No exception caught"); \
} catch (excMatch & e) { \
} catch (excMatch&) { \
} catch (...) { \
assert(0 && "Wrong exception caught"); \
} \
Expand Down
5 changes: 0 additions & 5 deletions src/util/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,6 @@ add_library(bitcoin_util STATIC EXCLUDE_FROM_ALL
../sync.cpp
)

target_compile_definitions(bitcoin_util
PRIVATE
$<$<CXX_COMPILER_ID:MSVC>:_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING>
)

target_link_libraries(bitcoin_util
PRIVATE
core_interface
Expand Down
Loading