build: add `-Wundef` #29876

fanquake commented at 12:26 PM on April 15, 2024: member

Turn on -Wundef.

> Warn if an undefined identifier is evaluated in an #if directive. Such identifiers are replaced with zero..

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.

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

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

<sub>Debug: https://github.com/bitcoin/bitcoin/runs/23825959417</sub>

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)

File	commit 58446e1d92c7da46da1fc48e1eb5eefe2e0748cb<br>(master)	commit 36d728a5b091e538c50f116fd68fccdfc4a53711<br>(master and this pull)
SHA256SUMS.part	`e64bcc6f819997a4...`	`3435d6ddfe201135...`
*-aarch64-linux-gnu-debug.tar.gz	`e9dbd78db7ec28a6...`	`798d3da5d7bea84f...`
*-aarch64-linux-gnu.tar.gz	`9aa2e1822187deeb...`	`6a6909988b7b4ba7...`
*-arm-linux-gnueabihf-debug.tar.gz	`b16c1210927617d7...`	`695cb3aab6fe5378...`
*-arm-linux-gnueabihf.tar.gz	`573dd0d104ff2f18...`	`d31b3bfaca31bfc5...`
*-arm64-apple-darwin-unsigned.tar.gz	`5dc92c1599c5b5b8...`	`b68a96df1fc4f423...`
*-arm64-apple-darwin-unsigned.zip	`3c56b6dc994cadeb...`	`442cd22ce81d573a...`
*-arm64-apple-darwin.tar.gz	`e83e7e6c83aa4796...`	`aa2bfb98a7fbf383...`
*-powerpc64-linux-gnu-debug.tar.gz	`7134fd5e2ada4a9e...`	`8ac217c17a2884c6...`
*-powerpc64-linux-gnu.tar.gz	`a1d3dac39d378d8d...`	`65654bf93e1c5408...`
*-riscv64-linux-gnu-debug.tar.gz	`171fd29eee7c062b...`	`117e59ebe5ecb6e4...`
*-riscv64-linux-gnu.tar.gz	`84d2958f708b4b0b...`	`2a3cec2b987ac31e...`
*-x86_64-apple-darwin-unsigned.tar.gz	`4b45d8205294d3af...`	`449d4036c5c530fe...`
*-x86_64-apple-darwin-unsigned.zip	`2d5af9a020c99f17...`	`bfb9940ed37d3729...`
*-x86_64-apple-darwin.tar.gz	`6adce47f81f36e76...`	`a56c483bfcdb34ac...`
*-x86_64-linux-gnu-debug.tar.gz	`30d17373f2655e6c...`	`c111f0081452b7fe...`
*-x86_64-linux-gnu.tar.gz	`4829a910aa342010...`	`e15c51770902d52f...`
*.tar.gz	`2621583f029a602f...`	`a205f40ba1639482...`
guix_build.log	`ab18d5cc2f844ae9...`	`7a9079f009388c74...`
guix_build.log.diff		`f556fe2ac3b66192...`

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 to ifdef makes it less safe because you lose the protection added by the warning in the first place. (say, when you forget to include bitcoin-config)

maflcko commented at 7:19 PM on April 16, 2024:

say, when you forget to include bitcoin-config)

In theory, there is a linter to check this. However, the linter could be tricked by guarding the include by #if defined(HAVE_C0NFIG_H) (O replaced by 0). This is fixed in #29494.

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.

laanwj commented at 8:00 AM on April 24, 2024:

i still think it'd be better, but probably not worth doing this last-minute change for autotools. Don't know how the CMake port handles these kind of checks anyway, but might consider it there. @hebasto

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

laanwj approved

laanwj commented at 8:27 AM on April 26, 2024: member

ACK cc09c165eb0bbb4b41a9132734bee55969e9d9a8

hebasto commented at 11:55 AM on April 29, 2024: member

Concept ACK.

hebasto approved

hebasto commented at 12:52 PM on April 29, 2024: member

ACK 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: member

Rebased 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, 2024

fanquake marked this as a draft on May 3, 2024

DrahtBot added the label CI failed on May 3, 2024

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

<sub>Debug: https://github.com/bitcoin/bitcoin/runs/24544741593</sub>

fanquake force-pushed on May 4, 2024

katesalazar commented at 6:58 PM on May 4, 2024: contributor

Concept ACK

theuni commented at 6:35 PM on June 6, 2024: member

Any reason this is still in draft? I agree it seems useful do enable before the CMake switch.

fanquake force-pushed on Jun 10, 2024

fanquake commented at 8:49 AM on June 10, 2024: member

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

In file included from minisketch/src/fields/generic_common_impl.h:12,
                 from minisketch/src/fields/generic_4bytes.cpp:12:
minisketch/src/fields/../int_utils.h:162:7: error: "HAVE_CLZ" is not defined, evaluates to 0 [-Werror=undef]
  162 | #elif HAVE_CLZ
      |       ^~~~~~~~

This was the multiprocess job failure:

/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
libtool: 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
/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
In file included from ipc/capnp/protocol.cpp:7:
In file included from ./ipc/capnp/init.capnp.h:6:
In file included from /ci_container_base/depends/i686-pc-linux-gnu/include/capnp/generated-header-support.h:26:
In file included from /ci_container_base/depends/i686-pc-linux-gnu/include/capnp/raw-schema.h:24:
In file included from /ci_container_base/depends/i686-pc-linux-gnu/include/capnp/common.h:31:
/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]
   39 | #if !_WIN32 && !__CYGWIN__
      |      ^
/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]
   39 | #if !_WIN32 && !__CYGWIN__
      |                 ^
In file included from ipc/capnp/protocol.cpp:7:
In file included from ./ipc/capnp/init.capnp.h:6:
In file included from /ci_container_base/depends/i686-pc-linux-gnu/include/capnp/generated-header-support.h:26:
In file included from /ci_container_base/depends/i686-pc-linux-gnu/include/capnp/raw-schema.h:24:
/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]
   33 | #if CAPNP_DEBUG_TYPES
      |     ^
In file included from ipc/capnp/protocol.cpp:7:
In file included from ./ipc/capnp/init.capnp.h:6:
In file included from /ci_container_base/depends/i686-pc-linux-gnu/include/capnp/generated-header-support.h:26:
/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]
   26 | #if _MSC_VER && !defined(__clang__)
      |     ^
In file included from ipc/capnp/protocol.cpp:7:
In file included from ./ipc/capnp/init.capnp.h:9:
In file included from /ci_container_base/depends/i686-pc-linux-gnu/include/capnp/capability.h:28:
In file included from /ci_container_base/depends/i686-pc-linux-gnu/include/kj/async.h:24:
/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]
   34 | #if !KJ_NO_EXCEPTIONS
      |      ^
In file included from ipc/capnp/protocol.cpp:7:
In file included from ./ipc/capnp/init.capnp.h:9:
In file included from /ci_container_base/depends/i686-pc-linux-gnu/include/capnp/capability.h:28:
In file included from /ci_container_base/depends/i686-pc-linux-gnu/include/kj/async.h:26:
/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]
   26 | #if _MSC_VER
      |     ^
6 errors generated.
make[2]: *** [Makefile:9738: ipc/capnp/libbitcoin_ipc_a-protocol.o] Error 1
make[2]: Leaving directory '/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/src'
make[1]: *** [Makefile:20118: install-recursive] Error 1
make[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: member

This was the multiprocess job failure:

cc @ryanofsky

hebasto commented at 10:25 AM on June 10, 2024: member

We'll probably need to fixup the failing ARM and previous releases job upstream:

In file included from minisketch/src/fields/generic_common_impl.h:12,
                 from minisketch/src/fields/generic_4bytes.cpp:12:
minisketch/src/fields/../int_utils.h:162:7: error: "HAVE_CLZ" is not defined, evaluates to 0 [-Werror=undef]
  162 | #elif HAVE_CLZ
      |       ^~~~~~~~

The upstream issue has been addressed in https://github.com/sipa/minisketch/pull/88.

ryanofsky commented at 4:49 PM on June 11, 2024: contributor

Hmm, 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 && SYMBOL2 everywhere 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, 2024

fanquake referenced this in commit 080a47cb8a on Jun 13, 2024

fanquake force-pushed on Jun 13, 2024

ryanofsky commented at 5:37 PM on June 17, 2024: contributor

Hmm, 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, 2024

fanquake commented at 9:53 AM on June 18, 2024: member

Maybe 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, 2024

fanquake commented at 12:01 PM on June 18, 2024: member

This is ready for further review.

DrahtBot removed the label CI failed on Jun 18, 2024

fanquake added the label DrahtBot Guix build requested on Jun 18, 2024

DrahtBot commented at 4:05 AM on June 19, 2024: contributor

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

File	commit 9c5cdf07f30f816cd134e2cd2dca9c27ef7067a5<br>(master)	commit f41ceb6a81754ed2f0c94182e2ae037efc1ab96e<br>(master and this pull)
SHA256SUMS.part	`8f176cd889e88c8c...`	`92d21483e82b2c1b...`
*-aarch64-linux-gnu-debug.tar.gz	`c4603906c5e521c8...`	`fdc8c845f2e6712d...`
*-aarch64-linux-gnu.tar.gz	`7f003e10e7ca2269...`	`b6577e5985b7da0c...`
*-arm-linux-gnueabihf-debug.tar.gz	`e4a3366f4709760b...`	`4d5e494959a1d4d6...`
*-arm-linux-gnueabihf.tar.gz	`5ce86cdc3b81597c...`	`5bdaf259bc089ed2...`
*-arm64-apple-darwin-unsigned.tar.gz	`e44ea6e8bee2b7f5...`	`256cfe6927d88cbb...`
*-arm64-apple-darwin-unsigned.zip	`bd2feb2c203fd085...`	`8702bade54b719f3...`
*-arm64-apple-darwin.tar.gz	`e6e67247faef3bd2...`	`1117533e8c50ee2b...`
*-powerpc64-linux-gnu-debug.tar.gz	`59c87ca578af3674...`	`94e8e0491e60cfa9...`
*-powerpc64-linux-gnu.tar.gz	`c42749de73d93831...`	`eda06147241168b9...`
*-riscv64-linux-gnu-debug.tar.gz	`088a788947cf2c8e...`	`5c208613a156427c...`
*-riscv64-linux-gnu.tar.gz	`7160ceb124bc9cf2...`	`45530d68ca9aafd3...`
*-x86_64-apple-darwin-unsigned.tar.gz	`78167426b180f468...`	`c0342ac292fc7609...`
*-x86_64-apple-darwin-unsigned.zip	`779c80cc3c01c447...`	`1d806bf0ecd64155...`
*-x86_64-apple-darwin.tar.gz	`8b498a9181e91cdb...`	`02dd017c2db27448...`
*-x86_64-linux-gnu-debug.tar.gz	`c44d3d010deda4b7...`	`57a68a9bbe06fc76...`
*-x86_64-linux-gnu.tar.gz	`1700f4111e39a2b7...`	`bdfc16c0956a4575...`
*.tar.gz	`8ebf38cec0d3f4eb...`	`8ed68c8fceaf06c4...`
guix_build.log	`554e151400868aa2...`	`670b3c580f73892f...`
guix_build.log.diff		`f4e73142d6abed0e...`

DrahtBot removed the label DrahtBot Guix build requested on Jun 19, 2024

fanquake requested review from theuni on Jun 20, 2024

DrahtBot added the label Needs rebase on Jun 20, 2024

ryanofsky approved

ryanofsky commented at 6:17 PM on June 20, 2024: contributor

Code review ACK 9e5bd977688f28a29806236f0faa55d5272f5b65

DrahtBot requested review from hebasto on Jun 20, 2024

DrahtBot requested review from laanwj on Jun 20, 2024

build: 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.

79e197b175

zmq: use #ifdef ENABLE_ZMQ 7839503b30

randomenv: 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]

40cd7585a0

refactor: 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
```

82b43955f7

build: add -Wundef

"Warn if an undefined identifier is evaluated in an #if directive. Such
identifiers are replaced with zero."

e3dc64f499

fanquake force-pushed on Jun 21, 2024

fanquake commented at 8:47 AM on June 21, 2024: member

Rebased to fix conflicts.

DrahtBot removed the label Needs rebase on Jun 21, 2024

hebasto approved

hebasto commented at 1:17 PM on June 24, 2024: member

ACK e3dc64f4990a15df3fd6147831f66fc2a31c71ad, I have reviewed the code and it looks OK.

DrahtBot requested review from ryanofsky on Jun 24, 2024

fanquake merged this on Jun 24, 2024

fanquake closed this on Jun 24, 2024

fanquake deleted the branch on Jun 24, 2024

hebasto commented at 6:52 AM on July 14, 2024: member

Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/264.

hebasto removed the label Needs CMake port on Jul 14, 2024

bitcoin locked this on Jul 14, 2025