Turn on -Wundef.
Note that this is still beneficial with CMake, and may even be nice to have enabled prior, to catch any change in behaviour.
If we end up with this enabled, it should probably be enough to fix #16419.
build: add -Wundef
#29876
pull
fanquake
wants to merge
5
commits into
bitcoin:master
from
fanquake:add_wundef
changing
5
files
+24 −20
-
fanquake commented at 12:26 pm on April 15, 2024: member
-
DrahtBot commented at 12:26 pm on April 15, 2024: contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Code Coverage
For detailed information about the code coverage, see the test coverage report.
Reviews
See the guideline for information on the review process.
Type Reviewers ACK hebasto Concept ACK katesalazar Stale ACK laanwj, ryanofsky If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Conflicts
Reviewers, this pull request conflicts with the following ones:
- #29775 (Testnet4 including PoW difficulty adjustment fix by fjahr)
- #29432 (Stratum v2 Template Provider (take 3) by Sjors)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
-
DrahtBot added the label Build system on Apr 15, 2024
-
maflcko added the label DrahtBot Guix build requested on Apr 15, 2024
-
laanwj commented at 1:32 pm on April 15, 2024: memberConcept ACK, issues with misspelled or otherwise missing defines can be really sneaky (and have caused some hard to find bugs in the past), this ia good check to have.
-
fanquake force-pushed on Apr 15, 2024
-
DrahtBot added the label CI failed on Apr 15, 2024
-
DrahtBot commented at 2:11 pm on April 15, 2024: contributor
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
-
DrahtBot removed the label CI failed on Apr 15, 2024
-
DrahtBot commented at 11:08 pm on April 15, 2024: contributor
Guix builds (on x86_64)
-
DrahtBot removed the label DrahtBot Guix build requested on Apr 15, 2024
-
fanquake marked this as ready for review on Apr 16, 2024
-
in src/randomenv.cpp:45 in 550980f23f outdated
43@@ -44,15 +44,15 @@ 44 #if HAVE_DECL_GETIFADDRS && HAVE_DECL_FREEIFADDRS 45 #include <ifaddrs.h> 46 #endif 47-#if HAVE_SYSCTL 48+#ifdef HAVE_SYSCTL
laanwj commented at 5:37 pm on April 16, 2024:Would it be possible to change these to values 0/1 instead of defined/undefined? It seems to me that changing these toifdefmakes it less safe because you lose the protection added by the warning in the first place. (say, when you forget to include bitcoin-config)
fanquake commented at 3:32 pm on April 17, 2024:Would it be possible to change these to values 0/1 instead of defined/undefined?
These are currently checked via:
AC_CHECK_HEADERS([sys/select.h sys/prctl.h sys/sysctl.h vm/vm_param.h sys/vmmeter.h sys/resources.h]). If we wanted, to we could change this.
fanquake commented at 10:11 am on April 24, 2024:but probably not worth doing this last-minute change for autotools.
Not sure. Everything not done for Autotools, but done in CMake is just more behaviour/divergence to maintain, so I’d prefer to do as much as possible prior.
laanwj commented at 8:21 am on April 26, 2024:i don’t know, i don’t think what i imagined is actually doable: make sure the entire build system->program interface consists of 0/1 defines (mandatory) not error-prone#define/#undef. There’s just too many. So resolving this, retracting my comment.laanwj added the label Needs CMake port on Apr 24, 2024laanwj approvedlaanwj commented at 8:27 am on April 26, 2024: memberACK cc09c165eb0bbb4b41a9132734bee55969e9d9a8hebasto commented at 11:55 am on April 29, 2024: memberConcept ACK.hebasto approvedhebasto commented at 12:52 pm on April 29, 2024: memberACK cc09c165eb0bbb4b41a9132734bee55969e9d9a8.
I suggest to consider updating a help string in https://github.com/bitcoin/bitcoin/blob/a46065e36cf868265c909dc5edf29dc17be53c1f/configure.ac#L1133 to “Define to 1 if O_CLOEXEC flag is available, and to 0 if not.”
As a follow up, the #16344 might be reconsidered.
fanquake commented at 7:41 am on May 3, 2024: memberRebased on #25972. Please review that first.
I suggest to consider updating a help string in
I don’t touch that here, and it’s not clear why we need to change a single instsance out of many help strings that could be similarly updated, so going to leave things as they are.
fanquake force-pushed on May 3, 2024fanquake marked this as a draft on May 3, 2024DrahtBot added the label CI failed on May 3, 2024DrahtBot commented at 10:04 am on May 3, 2024: contributor🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
fanquake force-pushed on May 4, 2024katesalazar commented at 6:58 pm on May 4, 2024: contributorConcept ACKtheuni commented at 6:35 pm on June 6, 2024: memberAny reason this is still in draft? I agree it seems useful do enable before the CMake switch.fanquake force-pushed on Jun 10, 2024fanquake commented at 8:49 am on June 10, 2024: memberAny reason this is still in draft? I agree it seems useful do enable before the CMake switch.
Rebased, and fixed the conflict. Added another change for the Win64 CI. We’ll probably need to fixup the failing ARM and previous releases job upstream:
0In file included from minisketch/src/fields/generic_common_impl.h:12, 1 from minisketch/src/fields/generic_4bytes.cpp:12: 2minisketch/src/fields/../int_utils.h:162:7: error: "HAVE_CLZ" is not defined, evaluates to 0 [-Werror=undef] 3 162 | #elif HAVE_CLZ 4 | ^~~~~~~~This was the multiprocess job failure:
0/bin/bash ../libtool --tag=CXX --preserve-dup-deps --mode=link /usr/bin/ccache clang++ -m32 -std=c++20 -g -O2 -O0 -g3 -ftrapv -fdebug-prefix-map=/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu=. -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wdocumentation -Wundef -Wno-unused-parameter -Wno-self-assign -Werror -fPIE -pipe -std=c++20 -O1 -g -Wno-error=documentation -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -pie -pthread -lpthread -L/ci_container_base/depends/i686-pc-linux-gnu/lib -o bitcoind bitcoind-bitcoind.o init/bitcoind-bitcoind.o libbitcoin_node.a libbitcoin_wallet.a libbitcoin_common.a libbitcoin_util.a libunivalue.la libbitcoin_zmq.a libbitcoin_consensus.a crypto/libbitcoin_crypto_base.la crypto/libbitcoin_crypto_sse41.la crypto/libbitcoin_crypto_avx2.la crypto/libbitcoin_crypto_x86_shani.la leveldb/libleveldb.la crc32c/libcrc32c.la leveldb/libmemenv.la secp256k1/libsecp256k1.la -ldb_cxx-4.8 -lminiupnpc -lnatpmp -L/ci_container_base/depends/i686-pc-linux-gnu/lib -levent_pthreads -levent -L/ci_container_base/depends/i686-pc-linux-gnu/lib -levent -L/ci_container_base/depends/i686-pc-linux-gnu/lib -lzmq -lpthread -lrt -L/ci_container_base/depends/i686-pc-linux-gnu/lib -lsqlite3 -lm -latomic 1libtool: link: /usr/bin/ccache clang++ -m32 -std=c++20 -g -O2 -O0 -g3 -ftrapv -fdebug-prefix-map=/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu=. -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wdocumentation -Wundef -Wno-unused-parameter -Wno-self-assign -Werror -fPIE -pipe -std=c++20 -O1 -g -Wno-error=documentation -Wl,-z -Wl,relro -Wl,-z -Wl,now -Wl,-z -Wl,separate-code -pie -o bitcoind bitcoind-bitcoind.o init/bitcoind-bitcoind.o -lpthread -L/ci_container_base/depends/i686-pc-linux-gnu/lib libbitcoin_node.a libbitcoin_wallet.a libbitcoin_common.a libbitcoin_util.a ./.libs/libunivalue.a libbitcoin_zmq.a libbitcoin_consensus.a crypto/.libs/libbitcoin_crypto_base.a crypto/.libs/libbitcoin_crypto_sse41.a crypto/.libs/libbitcoin_crypto_avx2.a crypto/.libs/libbitcoin_crypto_x86_shani.a leveldb/.libs/libleveldb.a crc32c/.libs/libcrc32c.a leveldb/.libs/libmemenv.a secp256k1/.libs/libsecp256k1.a -ldb_cxx-4.8 -lminiupnpc -lnatpmp -levent_pthreads -levent -levent -lzmq -lpthread -lrt -lsqlite3 -lm -latomic -pthread 2/usr/bin/ccache clang++ -m32 -std=c++20 -DHAVE_CONFIG_H -I. -I../src/config -DDEBUG -DDEBUG_LOCKORDER -DDEBUG_LOCKCONTENTION -DRPC_DOC_CHECK -DABORT_ON_FAILED_ASSUME -fmacro-prefix-map=/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu=. -DHAVE_BUILD_INFO -D_FILE_OFFSET_BITS=64 -DPROVIDE_FUZZ_MAIN_FUNCTION -I. -I./minisketch/include -I./secp256k1/include -I./univalue/include -D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC -D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG -I/ci_container_base/depends/i686-pc-linux-gnu/include/ -DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE -g -O2 -O0 -g3 -ftrapv -fdebug-prefix-map=/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu=. -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wdocumentation -Wundef -Wno-unused-parameter -Wno-self-assign -Werror -fPIE -I/ci_container_base/depends/i686-pc-linux-gnu/include -pthread -pipe -std=c++20 -O1 -g -Wno-error=documentation -c -o ipc/capnp/libbitcoin_ipc_a-protocol.o `test -f 'ipc/capnp/protocol.cpp' || echo './'`ipc/capnp/protocol.cpp 3In file included from ipc/capnp/protocol.cpp:7: 4In file included from ./ipc/capnp/init.capnp.h:6: 5In file included from /ci_container_base/depends/i686-pc-linux-gnu/include/capnp/generated-header-support.h:26: 6In file included from /ci_container_base/depends/i686-pc-linux-gnu/include/capnp/raw-schema.h:24: 7In file included from /ci_container_base/depends/i686-pc-linux-gnu/include/capnp/common.h:31: 8/ci_container_base/depends/i686-pc-linux-gnu/include/kj/windows-sanity.h:39:6: error: '_WIN32' is not defined, evaluates to 0 [-Werror,-Wundef] 9 39 | #if !_WIN32 && !__CYGWIN__ 10 | ^ 11/ci_container_base/depends/i686-pc-linux-gnu/include/kj/windows-sanity.h:39:17: error: '__CYGWIN__' is not defined, evaluates to 0 [-Werror,-Wundef] 12 39 | #if !_WIN32 && !__CYGWIN__ 13 | ^ 14In file included from ipc/capnp/protocol.cpp:7: 15In file included from ./ipc/capnp/init.capnp.h:6: 16In file included from /ci_container_base/depends/i686-pc-linux-gnu/include/capnp/generated-header-support.h:26: 17In file included from /ci_container_base/depends/i686-pc-linux-gnu/include/capnp/raw-schema.h:24: 18/ci_container_base/depends/i686-pc-linux-gnu/include/capnp/common.h:33:5: error: 'CAPNP_DEBUG_TYPES' is not defined, evaluates to 0 [-Werror,-Wundef] 19 33 | #if CAPNP_DEBUG_TYPES 20 | ^ 21In file included from ipc/capnp/protocol.cpp:7: 22In file included from ./ipc/capnp/init.capnp.h:6: 23In file included from /ci_container_base/depends/i686-pc-linux-gnu/include/capnp/generated-header-support.h:26: 24/ci_container_base/depends/i686-pc-linux-gnu/include/capnp/raw-schema.h:26:5: error: '_MSC_VER' is not defined, evaluates to 0 [-Werror,-Wundef] 25 26 | #if _MSC_VER && !defined(__clang__) 26 | ^ 27In file included from ipc/capnp/protocol.cpp:7: 28In file included from ./ipc/capnp/init.capnp.h:9: 29In file included from /ci_container_base/depends/i686-pc-linux-gnu/include/capnp/capability.h:28: 30In file included from /ci_container_base/depends/i686-pc-linux-gnu/include/kj/async.h:24: 31/ci_container_base/depends/i686-pc-linux-gnu/include/kj/async-prelude.h:34:6: error: 'KJ_NO_EXCEPTIONS' is not defined, evaluates to 0 [-Werror,-Wundef] 32 34 | #if !KJ_NO_EXCEPTIONS 33 | ^ 34In file included from ipc/capnp/protocol.cpp:7: 35In file included from ./ipc/capnp/init.capnp.h:9: 36In file included from /ci_container_base/depends/i686-pc-linux-gnu/include/capnp/capability.h:28: 37In file included from /ci_container_base/depends/i686-pc-linux-gnu/include/kj/async.h:26: 38/ci_container_base/depends/i686-pc-linux-gnu/include/kj/refcount.h:26:5: error: '_MSC_VER' is not defined, evaluates to 0 [-Werror,-Wundef] 39 26 | #if _MSC_VER 40 | ^ 416 errors generated. 42make[2]: *** [Makefile:9738: ipc/capnp/libbitcoin_ipc_a-protocol.o] Error 1 43make[2]: Leaving directory '/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/src' 44make[1]: *** [Makefile:20118: install-recursive] Error 1 45make[1]: Leaving directory '/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/src'hebasto commented at 9:03 am on June 10, 2024: memberThis was the multiprocess job failure:
cc @ryanofsky
hebasto commented at 10:25 am on June 10, 2024: memberWe’ll probably need to fixup the failing ARM and previous releases job upstream:
0In file included from minisketch/src/fields/generic_common_impl.h:12, 1 from minisketch/src/fields/generic_4bytes.cpp:12: 2minisketch/src/fields/../int_utils.h:162:7: error: "HAVE_CLZ" is not defined, evaluates to 0 [-Werror=undef] 3 162 | #elif HAVE_CLZ 4 | ^~~~~~~~The upstream issue has been addressed in https://github.com/sipa/minisketch/pull/88.
ryanofsky commented at 4:49 pm on June 11, 2024: contributorHmm, this is the first time I’ve seen the -Wundef warning, and my initial reaction is to be skeptical of it, because it seems like it would require patching a lot of upstream headers, and I’m not sure if it it just forces writing
#if defined(SYMBOL1) && defined(SYMBOL2)instead of#if SYMBOL1 && SYMBOL2everywhere it is likely to prevent any bugs. It seems more like it is breaking a commonly used idiom.But if I’m not seeing the benefits, or others just think Wundef is more useful, I can create a patch for depends and an upstream pull request for capnproto. I don’t want capnproto to get in the way of having better diagnostics or improvements to the build system.
sipa referenced this in commit fe6557642e on Jun 12, 2024fanquake referenced this in commit 080a47cb8a on Jun 13, 2024fanquake force-pushed on Jun 13, 2024ryanofsky commented at 5:37 pm on June 17, 2024: contributorHmm, this is the first time I’ve seen the -Wundef warning, and my initial reaction is to be skeptical of it, because it seems like it would require patching a lot of upstream headers
Actually I think I’m wrong about this. It seems like compiler will not trigger warnings in headers if headers are found on an -isystem path instead of a normal include path. So as long as we include capnproto headers with -isystem, maybe there will be no problems. I actually implemented that last week in commit a491cea18ee63e514359a5ba699e8cc159664efc from #10102 to suppress warnings from boost headers, and just realized that fix could be applicable here.
Maybe a491cea18ee63e514359a5ba699e8cc159664efc could be cherry-picked here to fix the multiprocess failure #29876 (comment)?
fanquake force-pushed on Jun 18, 2024fanquake commented at 9:53 am on June 18, 2024: memberMaybe https://github.com/bitcoin/bitcoin/commit/a491cea18ee63e514359a5ba699e8cc159664efc could be cherry-picked here to fix the multiprocess failure #29876 (comment)?
That very-much looks like the correct fix.
fanquake marked this as ready for review on Jun 18, 2024fanquake commented at 12:01 pm on June 18, 2024: memberThis is ready for further review.DrahtBot removed the label CI failed on Jun 18, 2024fanquake added the label DrahtBot Guix build requested on Jun 18, 2024DrahtBot commented at 4:05 am on June 19, 2024: contributorGuix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]
DrahtBot removed the label DrahtBot Guix build requested on Jun 19, 2024fanquake requested review from theuni on Jun 20, 2024DrahtBot added the label Needs rebase on Jun 20, 2024ryanofsky approvedryanofsky commented at 6:17 pm on June 20, 2024: contributorCode review ACK 9e5bd977688f28a29806236f0faa55d5272f5b65DrahtBot requested review from hebasto on Jun 20, 2024DrahtBot requested review from laanwj on Jun 20, 202479e197b175build: Suppress warnings from boost and capnproto in multiprocess code
Without this change there are errors from boost like: /ci_container_base/depends/i686-pc-linux-gnu/include/boost/signals2/expired_slot.hpp:23:28: error: 'what' overrides a member function but is not marked 'override' [-Werror,-Wsuggest-override] /ci_container_base/depends/i686-pc-linux-gnu/include/boost/signals2/detail/signal_template.hpp:750:32: error: 'lock_pimpl' overrides a member function but is not marked 'override' [-Werror,-Wsuggest-override] /ci_container_base/depends/i686-pc-linux-gnu/include/boost/signals2/connection.hpp:150:22: error: 'connected' overrides a member function but is not marked 'override' [-Werror,-Wsuggest-override] There do not seem to be errors from capnproto currently, but add a suppression for it, too, to be consistent with other libraries.
zmq: use #ifdef ENABLE_ZMQ 7839503b3040cd7585a0randomenv: use ifdef over if
randomenv.cpp:48:5: warning: 'HAVE_VM_VM_PARAM_H' is not defined, evaluates to 0 [-Wundef] randomenv.cpp:51:5: warning: 'HAVE_SYS_RESOURCES_H' is not defined, evaluates to 0 [-Wundef] randomenv.cpp:424:5: error: 'HAVE_SYSCTL' is not defined, evaluates to 0 [-Werror,-Wundef]
82b43955f7refactor: use #ifdef HAVE_SOCKADDR_UN
```bash init.cpp:526:5: error: "HAVE_SOCKADDR_UN" is not defined, evaluates to 0 [-Werror=undef] 526 | #if HAVE_SOCKADDR_UN | ^~~~~~~~~~~~~~~~ init.cpp:541:5: error: "HAVE_SOCKADDR_UN" is not defined, evaluates to 0 [-Werror=undef] 541 | #if HAVE_SOCKADDR_UN | ^~~~~~~~~~~~~~~~ init.cpp:1318:5: error: "HAVE_SOCKADDR_UN" is not defined, evaluates to 0 [-Werror=undef] 1318 | #if HAVE_SOCKADDR_UN ``` ``` netbase.cpp:26:5: error: "HAVE_SOCKADDR_UN" is not defined, evaluates to 0 [-Werror=undef] 26 | #if HAVE_SOCKADDR_UN | ^~~~~~~~~~~~~~~~ netbase.cpp:221:5: error: "HAVE_SOCKADDR_UN" is not defined, evaluates to 0 [-Werror=undef] 221 | #if HAVE_SOCKADDR_UN | ^~~~~~~~~~~~~~~~ netbase.cpp:496:5: error: "HAVE_SOCKADDR_UN" is not defined, evaluates to 0 [-Werror=undef] 496 | #if HAVE_SOCKADDR_UN | ^~~~~~~~~~~~~~~~ netbase.cpp:531:5: error: "HAVE_SOCKADDR_UN" is not defined, evaluates to 0 [-Werror=undef] 531 | #if HAVE_SOCKADDR_UN | ^~~~~~~~~~~~~~~~ netbase.cpp:639:5: error: "HAVE_SOCKADDR_UN" is not defined, evaluates to 0 [-Werror=undef] 639 | #if HAVE_SOCKADDR_UN ```e3dc64f499build: add -Wundef
"Warn if an undefined identifier is evaluated in an #if directive. Such identifiers are replaced with zero."
fanquake force-pushed on Jun 21, 2024fanquake commented at 8:47 am on June 21, 2024: memberRebased to fix conflicts.DrahtBot removed the label Needs rebase on Jun 21, 2024hebasto approvedhebasto commented at 1:17 pm on June 24, 2024: memberACK e3dc64f4990a15df3fd6147831f66fc2a31c71ad, I have reviewed the code and it looks OK.DrahtBot requested review from ryanofsky on Jun 24, 2024fanquake merged this on Jun 24, 2024fanquake closed this on Jun 24, 2024
fanquake deleted the branch on Jun 24, 2024
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-06-29 07:13 UTC
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-06-29 07:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me
More mirrored repositories can be found on mirror.b10c.me