build: optionally skip external warnings

vasild commented at 8:12 pm on April 23, 2020: member

Add an option to ./configure to suppress compilation warnings from external headers. The option is off by default (no change in behavior, show warnings from external headers).

This option is useful if e.g. Boost or Qt is installed outside of /usr/include (warnings from headers in /usr/include are already suppressed by default) and those warnings stand in the way of compiling Bitcoin Core with -Werror[=...] or they just clutter the build output too much and make our own warnings hard to spot.

-isystem /usr/include bricks GCC’s #include_next, so we use -idirafter instead. This way we don’t have to treat /usr/include specially.

vasild commented at 8:13 pm on April 23, 2020: member

cc @Empact @hebasto @theuni

DrahtBot commented at 8:37 pm on April 23, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20125 (rpc, wallet: Expose database format in getwalletinfo by promag)
#19077 (wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101)

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 23, 2020

vasild commented at 9:02 am on April 24, 2020: member

To test that this works as intended plant some warning in a Qt header (if you don’t have already!). For example:

0inline bool f(int iii, unsigned uuu) { return iii < uuu; }

in /qt/installed/here/include/qt5/QtCore/qglobal.h and see that the warning is gone with ./configure --enable-suppress-external-warnings.

vasild commented at 9:16 am on April 24, 2020: member

We currently compile our code with some warnings explicitly silenced: -Wno-unused-local-typedef -Wno-deprecated-register -Wno-implicit-fallthrough.

Currently Bitcoin Core is free of such warnings, but they can sneak in at anytime completely unnoticed - due to the above flags nobody will notice if a new code introduces such a warning.

If this PR gets merged, then we can consider removing the above -Wno-... flags. If it turns out that some external header produces such warnings, then they can be silenced using the functionality from this PR. This way we will spot if a new code introduces such a warning in Bitcoin Core.

jonatack commented at 4:18 pm on April 25, 2020: member

Concept ACK

brakmic commented at 10:12 am on May 1, 2020: contributor

Concept ACK.

hebasto commented at 11:56 pm on May 3, 2020: member

Tested on Ubuntu 20.04 LTS (gcc 9.3.0) and macOS 10.15.4 (llvm clang 10.0). Works as expected.

I’ve looked through our docs, and didn’t find any compiler warning policy. Or did I miss it?

From the POV of a user who respects one’s own safety and builds one’s own node binaries from the source, I believe there is no a such thing like a “harmless compiler warning”. At least for major OSes (the two latest versions of Ubuntu LTS, Debian stable, Fedora, macOS, Windows with default system compilers) on the x86_64 platform. In the worst case, a rare warning should be documented in the release notes.

Concept ACK.

Is it possible to increase the granularity of --enable-suppress-external-warnings? E.g., --enable-suppress-external-warnings=qt? Related: #16722.

vasild commented at 6:35 am on May 4, 2020: member

Tested on Ubuntu 20.04 LTS (gcc 9.3.0) and macOS 10.15.4 (llvm clang 10.0). Works as expected.

Excellent!

From the POV of a user who respects one’s own safety and builds one’s own node binaries from the source, I believe there is no a such thing like a “harmless compiler warning”.

I agree. This is why the new option is off by default. And I do not think it should ever be changed (to on by default).

Is it possible to increase the granularity of --enable-suppress-external-warnings? E.g., --enable-suppress-external-warnings=qt?

I did consider this! It would increase the complexity of this feature to some extent, but there is a bigger problem - we suppress warnings based on the directory in which the header which produced the warning is located. So, if both qt and boost are installed in the same directory (e.g. /usr/local/include) then such separation will not work as expected.

DrahtBot added the label Needs rebase on Jun 13, 2020

luke-jr referenced this in commit 902eb453de on Jun 14, 2020

luke-jr referenced this in commit bebc04d73f on Jun 15, 2020

vasild force-pushed on Jun 15, 2020

vasild commented at 5:06 pm on June 15, 2020: member

Rebased (trivial conflict with the merged #18297).

DrahtBot removed the label Needs rebase on Jun 15, 2020

DrahtBot added the label Needs rebase on Aug 5, 2020

vasild force-pushed on Aug 11, 2020

vasild commented at 12:18 pm on August 11, 2020: member

Rebased to resolve conflicts.

DrahtBot removed the label Needs rebase on Aug 11, 2020

DrahtBot added the label Needs rebase on Aug 24, 2020

vasild commented at 1:02 pm on September 1, 2020: member

Rebased to resolve conflicts.

vasild force-pushed on Sep 1, 2020

DrahtBot removed the label Needs rebase on Sep 1, 2020

practicalswift commented at 6:49 am on September 13, 2020: contributor

ACK 272066393dd3a70df7fc889cea95e1c7c943fc1a

Thanks for doing this @vasild: having this in master would allow us to enable additional compiler diagnostics without having to worry about warning spam from external dependencies :)

in configure.ac:1202 in 272066393d outdated

1193+  QT_INCLUDES_UNSUPPRESSED=$QT_INCLUDES
1194+  if test x$suppress_external_warnings != xno ; then
1195+    QT_INCLUDES=SUPPRESS_WARNINGS($QT_INCLUDES)
1196+    QT_DBUS_INCLUDES=SUPPRESS_WARNINGS($QT_DBUS_INCLUDES)
1197+    QT_TEST_INCLUDES=SUPPRESS_WARNINGS($QT_TEST_INCLUDES)
1198+  fi

hebasto commented at 9:31 am on September 16, 2020:

Should these lines be a part of Makefile.qt.include?

vasild commented at 11:04 am on September 18, 2020:

I don’t think it is possible to call functions defined in configure.ac from a Makefile.

in configure.ac:1168 in 272066393d outdated

1163+dnl Replace -I with -idirafter in $SOME_CPPFLAGS to suppress warnings from
1164+dnl headers from its include directories and return the result.
1165+dnl See -idirafter documentation:
1166+dnl https://gcc.gnu.org/onlinedocs/gcc/Directory-Options.html
1167+dnl https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-idirafter-arg
1168+AC_DEFUN([SUPPRESS_WARNINGS], [`echo $1 |sed -E -e 's/(^| )-I/\1-idirafter /g'`])

hebasto commented at 9:00 pm on September 20, 2020:

nit: Use $(…) notation instead of legacy backticked `…`.

vasild commented at 9:43 am on September 23, 2020:

Done.

hebasto commented at 9:01 pm on September 20, 2020: member

Tested 272066393dd3a70df7fc889cea95e1c7c943fc1a on top of the master (b99a1633b270e0e89479b2bb2ae19a8a8dc0fa05) on Linux Mint 20 (x86_64, focal code base).

 0$ make -C depends CC=clang CXX='clang++ -stdlib=libc++' NO_QT=1
 1$ CONFIG_SITE=$PWD/depends/x86_64-pc-linux-gnu/share/config.site ./configure --enable-suppress-external-warnings CC=clang CXX='clang++ -stdlib=libc++'
 2$ make clean
 3$ make
 4$ lldb src/bitcoind -- -regtest
 5...
 6Process 366548 stopped
 7* thread [#1](/bitcoin-bitcoin/1/), name = 'bitcoind', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
 8    frame [#0](/bitcoin-bitcoin/0/): 0x00007ffff7cb14e5 libc.so.6`__strlen_avx2 at strlen-avx2.S:65
 9(lldb) bt
10* thread [#1](/bitcoin-bitcoin/1/), name = 'bitcoind', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
11  * frame [#0](/bitcoin-bitcoin/0/): 0x00007ffff7cb14e5 libc.so.6`__strlen_avx2 at strlen-avx2.S:65
12    frame [#1](/bitcoin-bitcoin/1/): 0x0000555555b51453 bitcoind`__os_strdup + 35
13    frame [#2](/bitcoin-bitcoin/2/): 0x0000555555b2b847 bitcoind`__env_add_data_dir + 199
14    frame [#3](/bitcoin-bitcoin/3/): 0x0000555555b2b722 bitcoind`__env_set_data_dir + 18
15    frame [#4](/bitcoin-bitcoin/4/): 0x0000555555afbe99 bitcoind`DbEnv::set_data_dir(char const*) + 57
16    frame [#5](/bitcoin-bitcoin/5/): 0x0000555555957676 bitcoind`BerkeleyEnvironment::Open(bilingual_str&) + 950
17    frame [#6](/bitcoin-bitcoin/6/): 0x0000555555958f12 bitcoind`BerkeleyDatabase::Verify(bilingual_str&) + 210
18    frame [#7](/bitcoin-bitcoin/7/): 0x000055555595d63c bitcoind`MakeBerkeleyDatabase(boost::filesystem::path const&, DatabaseOptions const&, DatabaseStatus&, bilingual_str&) + 524
19    frame [#8](/bitcoin-bitcoin/8/): 0x000055555594354b bitcoind`MakeDatabase(boost::filesystem::path const&, DatabaseOptions const&, DatabaseStatus&, bilingual_str&) + 619
20    frame [#9](/bitcoin-bitcoin/9/): 0x00005555558fb624 bitcoind`MakeWalletDatabase(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, DatabaseOptions const&, DatabaseStatus&, bilingual_str&) + 180
21    frame [#10](/bitcoin-bitcoin/10/): 0x000055555585001c bitcoind`VerifyWallets(interfaces::Chain&) + 2892
22    frame [#11](/bitcoin-bitcoin/11/): 0x0000555555839e3b bitcoind`interfaces::(anonymous namespace)::WalletClientImpl::verify() + 27
23    frame [#12](/bitcoin-bitcoin/12/): 0x00005555555b7139 bitcoind`AppInitMain(util::Ref const&, NodeContext&, interfaces::BlockAndHeaderTipInfo*) + 5177
24    frame [#13](/bitcoin-bitcoin/13/): 0x00005555555a2776 bitcoind`main + 2550
25    frame [#14](/bitcoin-bitcoin/14/): 0x00007ffff7b4d0b3 libc.so.6`__libc_start_main(main=(bitcoind`main), argc=2, argv=0x00007fffffffdd78, init=<unavailable>, fini=<unavailable>, rtld_fini=<unavailable>, stack_end=0x00007fffffffdd68) at libc-start.c:308:16
26    frame [#15](/bitcoin-bitcoin/15/): 0x00005555555a1cbe bitcoind`_start + 46

vasild force-pushed on Sep 23, 2020

vasild commented at 9:47 am on September 23, 2020: member

Updated: use -isystem instead of -idirafter because if there are two incompatible boost installations in the system, for example in /usr/include/boost and in /whatever/boost, then -I/whatever or -isystem /whatever will cause the second one to be picked (correct) while -idirafter /whatever will cause the first one to be picked from /usr/include (incorrect). Thanks, @hebasto!

Also added BDB to the list.

hebasto commented at 11:00 am on September 25, 2020: member

Approach ACK 359870798edb77474a57536e5b13512b2d388253, tested on macOS 10.15.7 with Apple clang 11.0.3 and Qt 5.15.1, and Linux Mint 20 (focal codebase) with gcc and clang.

It would be nice to list some system setups when --enable-suppress-external-warnings does suppress warning, as all of the systems I tested emit only warnings that I’ve injected myself :)

… or list planned additional compiler diagnostics.

vasild commented at 1:57 pm on September 25, 2020: member

It would be nice to list some system setups …

You mean here on GitHub? Here is one: FreeBSD 12, all defaults (boost 1.72, Qt 5.15.0, bdb 5.3.28), without this option 27519 warnings are printed. Some of them:

0/usr/local/include/boost/system/error_code.hpp:290:18: warning: 'name' overrides a member function but is not marked 'override' [-Wsuggest-override]
1
2/usr/local/include/db5/db_cxx.h:1403:22: warning: 'what' overrides a member function but is not marked 'override' [-Wsuggest-override]
3
4/usr/local/include/qt5/QtWidgets/qstackedwidget.h:54:5: warning: 'qt_metacall' overrides a member function but is not marked 'override' [-Wsuggest-override]

vasild commented at 8:28 am on September 29, 2020: member

Another example: https://travis-ci.org/github/bitcoin/bitcoin/jobs/730973642#L2748

0/home/travis/build/bitcoin/bitcoin/depends/arm-linux-gnueabihf/share/../include/boost/process/detail/posix/executor.hpp:191:16: warning: ignoring return value of ‘ssize_t write(int, const void*, size_t)’, declared with attribute warn_unused_result [-Wunused-result]
1
2         ::write(_pipe_sink, &len, sizeof(int));
3
4         ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This patch would allow us to use --enable-werror on ARM [GOAL: install] [buster] [unit tests, functional tests] in CI so that newly added warnings on this platform in Bitcoin Core are detected (cause CI failure).

practicalswift commented at 6:14 pm on September 29, 2020: contributor

ACK 359870798edb77474a57536e5b13512b2d388253: patch looks correct

MarcoFalke added the label Needs gitian build on Sep 30, 2020

MarcoFalke added the label Needs Guix build on Sep 30, 2020

DrahtBot commented at 7:28 am on October 1, 2020: member

Guix builds

File	commit 1769828684f16b53e5fbf65173f508b9ea1b4b9c(master)	commit 75b42f93afd3071ef89fa9b179350e2585b98a9c(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz	`2c90411d96eeef0a...`	`d1b1c6711dab92dc...`
*-aarch64-linux-gnu.tar.gz	`9316a0f77c5cf478...`	`895d7365e18f30bc...`
*-arm-linux-gnueabihf-debug.tar.gz	`222c50ce8f2c0ffc...`	`9ec9ea5de526a12f...`
*-arm-linux-gnueabihf.tar.gz	`4e2977e8a8a33266...`	`3dc0c21c7abc1d80...`
*-riscv64-linux-gnu-debug.tar.gz	`820db95fd8891d61...`	`25306dd7bfd1f2e5...`
*-riscv64-linux-gnu.tar.gz	`4d51430614ca2fc7...`	`a945cc0f5faf05c8...`
*-win-unsigned.tar.gz	`51f33713e3d706f9...`	`be3b56bbda932053...`
*-win64-debug.zip	`dc6355584170c13d...`	`adb36e02fc77d847...`
*-win64-setup-unsigned.exe	`6280c2c1ee411214...`	`673ddc3eb2c13da5...`
*-win64.zip	`1c63275334074f40...`	`7544fbb63518b312...`
*-x86_64-linux-gnu-debug.tar.gz	`f249f8a755a16fbb...`	`95c9072e4bf7595f...`
*-x86_64-linux-gnu.tar.gz	`3cab2d29de25d7ac...`	`fc93fa6117ce766c...`
*.tar.gz	`bac5492f2d68c97c...`	`bb2f70ea90c584ac...`
guix_build.log	`e97379d33c122448...`	`86e53b0040f36f5c...`
guix_build.log.diff		`57e8da602d723af1...`

DrahtBot removed the label Needs Guix build on Oct 1, 2020

DrahtBot commented at 4:53 pm on October 2, 2020: member

Gitian builds

File	commit 3487e421a7fef4b28381efcf21a7e38483946cec(master)	commit 075064ee51fc09455f3d2c52732f45a43fb58bcf(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz	`610763669e7e3cb7...`	`33035bce46b95145...`
*-aarch64-linux-gnu.tar.gz	`51812ca091622ed9...`	`b406d50cec49a3f2...`
*-arm-linux-gnueabihf-debug.tar.gz	`f93c0427cd047290...`	`06cbc1ca11daa6f1...`
*-arm-linux-gnueabihf.tar.gz	`6df65db3f3a7a123...`	`c6ff1969ea796a5f...`
*-osx-unsigned.dmg	`0d4a4bf011b2019e...`	`3c1b7b22e158ebfa...`
*-osx64.tar.gz	`0aa9fd6f4187d33f...`	`b3f3b6a4df2af0a3...`
*-riscv64-linux-gnu-debug.tar.gz	`58914f8feac72537...`	`33b5790f09aaad56...`
*-riscv64-linux-gnu.tar.gz	`1a30336e17f7cd6b...`	`022fb921d6fa95ef...`
*-win64-debug.zip	`2f06f34ffd627e79...`	`d721eda9e9bfb8e8...`
*-win64-setup-unsigned.exe	`f23026e90602164d...`	`a5ee4164e3273583...`
*-win64.zip	`8d3acee5e2584d3b...`	`c3de365b9adceb46...`
*-x86_64-linux-gnu-debug.tar.gz	`be688faf54cfea87...`	`5c5bc415d8c7b2c3...`
*-x86_64-linux-gnu.tar.gz	`2a78323df8c70758...`	`c0f268c3ebf21bdc...`
*.tar.gz	`7bf0d038afb59072...`	`ae81bc977248923e...`
bitcoin-core-linux-0.21-res.yml	`2b37aee7c44927ee...`	`e4ebc15cddc8666b...`
bitcoin-core-osx-0.21-res.yml	`1c3793321abc2f61...`	`9d5183fb50a6f3e9...`
bitcoin-core-win-0.21-res.yml	`5de911b923fdda5a...`	`9bd083214ee81fd2...`
linux-build.log	`e44bb3ce2f5117e8...`	`8e0d48eaf53688fe...`
osx-build.log	`886fd63ccaf3e7cb...`	`36567233ae47375f...`
win-build.log	`aba8b79f48c7ff90...`	`237c646313597eeb...`
bitcoin-core-linux-0.21-res.yml.diff		`11b392af50349d0e...`
bitcoin-core-osx-0.21-res.yml.diff		`7aef47fbcda76204...`
bitcoin-core-win-0.21-res.yml.diff		`5178884b6c020cf6...`
linux-build.log.diff		`d835a581c9dbfcd2...`
osx-build.log.diff		`e877416b362f608b...`
win-build.log.diff		`75c0c7e2054c8217...`

DrahtBot removed the label Needs gitian build on Oct 2, 2020

in configure.ac:1172 in 359870798e outdated

1167+dnl https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-isystem-directory
1168+dnl Do not change "-I/usr/include" to "-isystem /usr/include" because that
1169+dnl is not necessary (/usr/include is already a system directory) and because
1170+dnl it would break GCC's #include_next.
1171+AC_DEFUN([SUPPRESS_WARNINGS],
1172+         [$(echo $1 |sed -E -e 's/(^| )-I/\1-isystem /g' -e 's|-isystem /usr/include|-I/usr/include|g')])

luke-jr commented at 2:17 pm on October 12, 2020:

Should use ${SED} here in place of sed

luke-jr commented at 2:28 pm on October 12, 2020:

This incorrectly excludes /usr/includefoo

Perhaps use s#-isystem /usr/include([/ ]|$)#-I/usr/include\1#g

vasild commented at 4:19 pm on October 12, 2020:

Done, except that the # separator causes the following during autogen.sh:

 0configure.ac:11: error: m4_init: unbalanced m4_divert_push:
 1configure.ac:1199: m4_divert_push: GROW
 2configure.ac:11: m4_divert: BODY
 3configure.ac:1171: SUPPRESS_WARNINGS is expanded from...
 4configure.ac:1171: SUPPRESS_WARNINGS is expanded from...
 5configure.ac:1171: SUPPRESS_WARNINGS is expanded from...
 6configure.ac:1171: SUPPRESS_WARNINGS is expanded from...
 7configure.ac:1171: SUPPRESS_WARNINGS is expanded from...
 8configure.ac:11: the top level
 9autom4te-2.69: /usr/local/bin/gm4 failed with exit status: 1
10aclocal: error: echo failed with exit status: 1
11autoreconf-2.69: aclocal failed with exit status: 1

so I used ; as a separator instead.

vasild commented at 4:19 pm on October 12, 2020:

Done

luke-jr changes_requested

build: optionally skip external warnings

Add an option to `./configure` to suppress compilation warnings from
external headers. The option is off by default (no change in behavior,
show warnings from external headers).

This option is useful if e.g. Boost or Qt is installed outside of
`/usr/include` (warnings from headers in `/usr/include` are already
suppressed by default) and those warnings stand in the way of compiling
Bitcoin Core with `-Werror[=...]` or they just clutter the build output
too much and make our own warnings hard to spot.

ba8950ee01

vasild force-pushed on Oct 12, 2020

hebasto approved

hebasto commented at 5:26 pm on October 12, 2020: member

ACK ba8950ee0134a7958e3e9b041cd54d222feb09a1, tested on Linux Mint 20 (x86_64).

With --enable-suppress-external-warnings having warnings from leveldb only:

 0$ make > /dev/null 
 1leveldb/db/c.cc: In function ‘leveldb_filterpolicy_t* leveldb_filterpolicy_create_bloom(int)’:
 2leveldb/db/c.cc:474:17: warning: ‘virtual const char* leveldb_filterpolicy_create_bloom(int)::Wrapper::Name() const’ can be marked override [-Wsuggest-override]
 3  474 |     const char* Name() const { return rep_->Name(); }
 4      |                 ^~~~
 5leveldb/db/c.cc:475:10: warning: ‘virtual void leveldb_filterpolicy_create_bloom(int)::Wrapper::CreateFilter(const leveldb::Slice*, int, std::string*) const’ can be marked override [-Wsuggest-override]
 6  475 |     void CreateFilter(const Slice* keys, int n, std::string* dst) const {
 7      |          ^~~~~~~~~~~~
 8leveldb/db/c.cc:478:10: warning: ‘virtual bool leveldb_filterpolicy_create_bloom(int)::Wrapper::KeyMayMatch(const leveldb::Slice&, const leveldb::Slice&) const’ can be marked override [-Wsuggest-override]
 9  478 |     bool KeyMayMatch(const Slice& key, const Slice& filter) const {
10      |          ^~~~~~~~~~~

luke-jr approved

luke-jr commented at 2:24 pm on October 13, 2020: member

utACK ba8950ee0134a7958e3e9b041cd54d222feb09a1

practicalswift commented at 2:33 pm on October 13, 2020: contributor

ACK ba8950ee0134a7958e3e9b041cd54d222feb09a1: diff looks correct!

Really looking forward to having this in master :)

laanwj merged this on Oct 14, 2020

laanwj closed this on Oct 14, 2020

vasild deleted the branch on Oct 14, 2020

laanwj commented at 5:11 pm on October 14, 2020: member

Thanks for adding this option, it definitely helps reduce the flood of warnings in boost and qt etc.

sidhujag referenced this in commit 96f66c9cdc on Oct 16, 2020

PastaPastaPasta referenced this in commit b9b7d1f620 on Oct 2, 2021

PastaPastaPasta referenced this in commit 09a6ed6194 on Oct 2, 2021

PastaPastaPasta referenced this in commit df5c6ba13a on Oct 2, 2021

PastaPastaPasta referenced this in commit 7cbea152ce on Oct 2, 2021

PastaPastaPasta referenced this in commit ff163d37b2 on Oct 5, 2021

PastaPastaPasta referenced this in commit 027a3abbc4 on Oct 5, 2021

PastaPastaPasta referenced this in commit 702bc8cb85 on Oct 5, 2021

PastaPastaPasta referenced this in commit d024d32280 on Oct 11, 2021

PastaPastaPasta referenced this in commit 9b130a691f on Oct 11, 2021

PastaPastaPasta referenced this in commit 9fe6c14f92 on Oct 11, 2021

UdjinM6 referenced this in commit 7be48286f4 on Oct 11, 2021

pravblockc referenced this in commit 87dc5d2f8e on Nov 18, 2021

DrahtBot locked this on Feb 15, 2022

build: optionally skip external warnings #18750

Conflicts

Guix builds

Gitian builds