cmake: Add compiler diagnostic flags #84

hebasto · 2024-01-26T15:45:13Z

Sorry for not presenting the third commit as fixup ones ready to be auto-squashed.

hebasto · 2024-01-27T16:08:34Z

Rebased.

fanquake · 2024-01-29T10:17:44Z

src/qt/CMakeLists.txt

@@ -85,6 +85,9 @@ target_include_directories(bitcoinqt
  PUBLIC
    $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/src>
 )
+set_property(SOURCE macnotificationhandler.mm
+  APPEND PROPERTY COMPILE_OPTIONS -Wno-error=deprecated-declarations


This isn't in the master branch? Also not sure why it's -Wno-error? We aren't moving to a state where we are using, or trying to enable -Werror by default?

This isn't in the master branch?

bitcoin#29362 has been submitted to the master branch.

Ok, dropped.

fanquake · 2024-01-29T10:21:09Z

cmake/secp256k1.cmake

@@ -57,4 +57,6 @@ if(MSVC)
  )
 endif()

-target_link_libraries(secp256k1 PRIVATE core_interface)
+# Using core_base_interface instead of core_interface
+# to avoid C++-specific flags from warn_interface.


I think this comment can be improved. We are avoiding passing warning flags to libsecp256k1 because it's a subtree, and warnings produced aren't fixable in our tree, making them pointless, not because it's C code.

Thanks! Reworked.

fanquake · 2024-01-29T10:30:09Z

CMakeLists.txt

+    )
+    try_append_cxx_flags("-Wself-assign" TARGET warn_interface
+      IF_CHECK_PASSED "-Wno-self-assign"
+    )


Missing -Wno-deprecated-copy ?

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

hebasto · 2024-02-09T21:25:05Z

Drafted.

Please review #99 and #100 first.

33dee19 fixup! cmake: Build `leveldb` static library (Hennadii Stepanov) Pull request description: This PR split from #84 It introduces a new `warn_leveldb_interface`, which overrides `-Wconditional-uninitialized` and `-Wsuggest-override` if any. For MSVC builds, dropped warning suppressions that are provided at the global level. For example, on the staging branch: ``` $ env CC=clang CXX=clang++ CXXFLAGS="-Wconditional-uninitialized -Wsuggest-override" cmake -B build $ cmake --build build --target leveldb > /dev/null /home/hebasto/git/bitcoin/src/leveldb/db/c.cc:474:17: warning: 'Name' overrides a member function but is not marked 'override' [-Wsuggest-override] const char* Name() const { return rep_->Name(); } ^ /home/hebasto/git/bitcoin/src/leveldb/db/c.cc:110:15: note: overridden virtual function is here const char* Name() const override { return (*name_)(state_); } ^ /home/hebasto/git/bitcoin/src/leveldb/db/c.cc:475:10: warning: 'CreateFilter' overrides a member function but is not marked 'override' [-Wsuggest-override] void CreateFilter(const Slice* keys, int n, std::string* dst) const { ^ /home/hebasto/git/bitcoin/src/leveldb/db/c.cc:112:8: note: overridden virtual function is here void CreateFilter(const Slice* keys, int n, std::string* dst) const override { ^ /home/hebasto/git/bitcoin/src/leveldb/db/c.cc:478:10: warning: 'KeyMayMatch' overrides a member function but is not marked 'override' [-Wsuggest-override] bool KeyMayMatch(const Slice& key, const Slice& filter) const { ^ /home/hebasto/git/bitcoin/src/leveldb/db/c.cc:125:8: note: overridden virtual function is here bool KeyMayMatch(const Slice& key, const Slice& filter) const override { ^ 3 warnings generated. /home/hebasto/git/bitcoin/src/leveldb/db/version_set.cc:1016:55: warning: variable 'manifest_size' may be uninitialized when used here [-Wconditional-uninitialized] descriptor_log_ = new log::Writer(descriptor_file_, manifest_size); ^~~~~~~~~~~~~ /home/hebasto/git/bitcoin/src/leveldb/db/version_set.cc:997:25: note: initialize the variable 'manifest_size' to silence this warning uint64_t manifest_size; ^ = 0 1 warning generated. ``` ACKs for top commit: vasild: ACK 33dee19 theuni: ACK 33dee19 Tree-SHA512: ffe9efc438bc808a42abaa42a45f7073c1e3cdd6404e9b8b811e94b304c2464f3df26c276d363c97a3941d50f0e6bcb3788f1d870dd23df86b0d9a33363b7e77

cbd43cc fixup! cmake: Build `bitcoind` executable (Hennadii Stepanov) 6be6d78 fixup! cmake: Build `bitcoin_util` static library (Hennadii Stepanov) Pull request description: The `HAVE_THREAD_LOCAL` is not visible in `src/test/util_threadnames_tests.cpp`. It is easy to verify with the diff as follows: ```diff --- a/src/test/util_threadnames_tests.cpp +++ b/src/test/util_threadnames_tests.cpp @@ -55,6 +55,7 @@ std::set<std::string> RenameEnMasse(int num_threads) BOOST_AUTO_TEST_CASE(util_threadnames_test_rename_threaded) { #if !defined(HAVE_THREAD_LOCAL) +#error // This test doesn't apply to platforms where we don't have thread_local. return; #endif ``` This PR fixes this issue by moving the macro definition to the `core_interface` library. Split from #84. ACKs for top commit: vasild: ACK cbd43cc Tree-SHA512: bd558ae35251d0e9bbdf494690a06ed57f6a36bdbf94840f1b134779bef5aa1de94ee53c962948a5a4b9cc11927777b5a919a7506779149e5fe0c6355c416419

hebasto · 2024-02-15T09:58:11Z

Rebased and undrafted.

vasild

ACK ab1a745

vasild · 2024-02-16T11:07:38Z

.github/workflows/cmake.yml

+            # For -Wno-error=unreachable-code, please refer to https://github.com/bitcoin/bitcoin/issues/29334
+            configure_env: 'env CXXFLAGS="-Wno-error=unreachable-code"'


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.

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.

vasild · 2024-02-16T13:04:16Z

CMakeLists.txt

+string(STRIP "${CMAKE_CXX_FLAGS}" cxx_flags)
+string(STRIP "${CMAKE_CXX_FLAGS_INIT}" cxx_flags_init)
+if(cxx_flags STREQUAL cxx_flags_init)


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"

-DCMAKE_CXX_COMPILER="c++;-myflag"

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

vasild

ACK ba8a3c3

The sneaked fixups look ok too.

hebasto · 2024-02-28T12:01:48Z

Rebased to avoid merge conflicts.

@vasild

ACK ba8a3c3

The sneaked fixups look ok too.

Sorry for invalidating your ACK :)

vasild

ACK 3abba8b

pablomartin4btc

tACK e4ef404

Before (or when with this PR user overrides CMAKE_CXX_FLAGS):

Common compile options ................ -fno-extended-identifiers -fstack-reuse=none -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection

After (when CMAKE_CXX_FLAGS matches CMAKE_CXX_FLAGS_INIT -> meant to be set by a toolchain file):

Common compile options ................ -fno-extended-identifiers -fstack-reuse=none -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wall -Wextra -Wformat -Wformat-security -Wvla -Wredundant-decls -Wdate-time -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wno-unused-parameter

vasild

ACK e4ef404

hebasto · 2024-03-08T15:36:19Z

Rebased on top of the bitcoin#29577 after the latest syncing.

It made OBJCXXFLAGS="-Wno-error=deprecated-declarations" unneeded in the CI.

The core_base_interface does not have C++-specific compiler options.

Add more `-DWERROR=ON`.

vasild

ACK 695a5ca

hebasto force-pushed the 240126-cmake-BA branch 2 times, most recently from 131db28 to d9d0ce7 Compare January 26, 2024 17:53

hebasto force-pushed the cmake-staging branch from ff2f574 to dacc6f7 Compare January 27, 2024 15:54

hebasto force-pushed the 240126-cmake-BA branch from d9d0ce7 to 9d64cb6 Compare January 27, 2024 16:07

hebasto force-pushed the 240126-cmake-BA branch from 9d64cb6 to 92291c0 Compare January 27, 2024 21:59

hebasto added the enhancement New feature or request label Jan 27, 2024

fanquake reviewed Jan 29, 2024

View reviewed changes

hebasto force-pushed the cmake-staging branch from 17f4870 to 6367599 Compare February 9, 2024 18:38

hebasto force-pushed the 240126-cmake-BA branch from 92291c0 to be01037 Compare February 9, 2024 19:08

hebasto mentioned this pull request Feb 9, 2024

cmake: Silent leveldb-specific warnings #99

Merged

hebasto marked this pull request as draft February 9, 2024 21:24

hebasto force-pushed the 240126-cmake-BA branch 4 times, most recently from 3ef5cc5 to 7878e2b Compare February 10, 2024 13:01

This was referenced Feb 10, 2024

[BUGFIX] cmake: Fix HAVE_THREAD_LOCAL macro visibility #100

Merged

build: Add missed definition for AM_OBJCXXFLAGS bitcoin/bitcoin#29362

Closed

hebasto force-pushed the 240126-cmake-BA branch from 7878e2b to ab1a745 Compare February 15, 2024 09:57

hebasto marked this pull request as ready for review February 15, 2024 09:57

vasild approved these changes Feb 16, 2024

View reviewed changes

hebasto force-pushed the 240126-cmake-BA branch from ab1a745 to ba8a3c3 Compare February 26, 2024 23:42

vasild approved these changes Feb 28, 2024

View reviewed changes

hebasto force-pushed the 240126-cmake-BA branch from ba8a3c3 to 3abba8b Compare February 28, 2024 12:00

vasild approved these changes Feb 28, 2024

View reviewed changes

hebasto force-pushed the cmake-staging branch from 15edb82 to 9e07ce3 Compare March 1, 2024 15:32

hebasto force-pushed the 240126-cmake-BA branch from 3abba8b to e4ef404 Compare March 1, 2024 17:38

pablomartin4btc approved these changes Mar 2, 2024

View reviewed changes

pablomartin4btc mentioned this pull request Mar 5, 2024

cmake: Rework handling compiler flags #82

Merged

vasild approved these changes Mar 7, 2024

View reviewed changes

hebasto force-pushed the cmake-staging branch from 2716ccf to 81f3d4f Compare March 8, 2024 15:22

hebasto force-pushed the 240126-cmake-BA branch from e4ef404 to a9fcf86 Compare March 8, 2024 15:33

hebasto added 4 commits March 8, 2024 20:46

cmake: Add compiler diagnostic flags

172b7f5

FIXUP: Introduce core_base_interface

4d9c08f

The core_base_interface does not have C++-specific compiler options.

test: Fix MSVC warning C4101 "unreferenced local variable"

f67e9a7

fixup! ci: Test CMake edge cases

695a5ca

Add more `-DWERROR=ON`.

hebasto force-pushed the 240126-cmake-BA branch from a9fcf86 to 695a5ca Compare March 8, 2024 20:46

vasild approved these changes Mar 11, 2024

View reviewed changes

hebasto merged commit d01f59f into cmake-staging Mar 11, 2024
27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake: Add compiler diagnostic flags #84

cmake: Add compiler diagnostic flags #84

hebasto commented Jan 26, 2024 •

edited

Loading

hebasto commented Jan 27, 2024

fanquake Jan 29, 2024

hebasto Feb 1, 2024

hebasto Feb 10, 2024

fanquake Jan 29, 2024

hebasto Feb 15, 2024

fanquake Jan 29, 2024

hebasto Feb 15, 2024

hebasto commented Feb 9, 2024 •

edited

Loading

hebasto commented Feb 15, 2024

vasild left a comment

vasild Feb 16, 2024

fanquake Feb 16, 2024

vasild Feb 16, 2024

fanquake Feb 16, 2024

vasild left a comment

hebasto commented Feb 28, 2024

vasild left a comment

pablomartin4btc left a comment

vasild left a comment

hebasto commented Mar 8, 2024 •

edited

Loading

vasild left a comment

		# For -Wno-error=unreachable-code, please refer to https://github.com/bitcoin/bitcoin/issues/29334
		configure_env: 'env CXXFLAGS="-Wno-error=unreachable-code"'

cmake: Add compiler diagnostic flags #84

cmake: Add compiler diagnostic flags #84

Conversation

hebasto commented Jan 26, 2024 • edited Loading

hebasto commented Jan 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto commented Feb 9, 2024 • edited Loading

hebasto commented Feb 15, 2024

vasild left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasild left a comment

Choose a reason for hiding this comment

hebasto commented Feb 28, 2024

vasild left a comment

Choose a reason for hiding this comment

pablomartin4btc left a comment

Choose a reason for hiding this comment

vasild left a comment

Choose a reason for hiding this comment

hebasto commented Mar 8, 2024 • edited Loading

vasild left a comment

Choose a reason for hiding this comment

hebasto commented Jan 26, 2024 •

edited

Loading

hebasto commented Feb 9, 2024 •

edited

Loading

hebasto commented Mar 8, 2024 •

edited

Loading