build: Optionally enable -Wzero-as-null-pointer-constant

Empact commented at 10:47 pm on January 5, 2019: member

Note: This is based on #14920

This enables -Wzero-as-null-pointer-constant, while avoiding applying it to dependencies.

~~Used Qt::NoItemFlags, Qt::Widget instead where applicable.~~

See #10483, #10645, #13802 for more on nullptr.

Empact force-pushed on Jan 5, 2019

practicalswift commented at 11:07 pm on January 5, 2019: contributor

Concept ACK and in accordance with our developer notes.

Thanks for doing this!

Using zero as null pointer constant is sloppy at best and dangerous at worst :-)

fanquake added the label Build system on Jan 5, 2019

Empact force-pushed on Jan 6, 2019

hebasto commented at 9:21 am on January 6, 2019: member

Concept ACK.

maflcko commented at 9:34 am on January 6, 2019: member

Could submit the src/qt changes as separate pull request, since those are the bulk of the changes?

practicalswift commented at 9:38 am on January 6, 2019: contributor

@MarcoFalke Perhaps 10f81e916d5fbb2751d03d06901f68d9b507807c (including the two non QT fixes) can be posted as a separate PR since they are all trivial to review?

practicalswift commented at 3:30 pm on January 7, 2019: contributor

@Empact What about -Werror=zero-as-null-pointer-constant as discussed in #15114 (comment)?

Empact force-pushed on Jan 7, 2019

Empact renamed this:
~~build: Enable -Wzero-as-null-pointer-constant~~
build: Enable -Werror=zero-as-null-pointer-constant
on Jan 7, 2019

Empact commented at 7:04 pm on January 7, 2019: member

Enabled now. 👍

practicalswift commented at 9:07 pm on January 7, 2019: contributor

utACK 873145a56315ba032aa100646d682ec31d39e8d9

Empact force-pushed on Jan 9, 2019

Empact commented at 6:23 am on January 9, 2019: member

As per conversation with @theuni, this now is only active if the user configures with --enable-isystem, so the build possibilities are: default: disabled --enable-isystem: -Wzero-as-null-pointer-constant --enable-isystem --enable-werror: -Werror=zero-as-null-pointer-constant

The idea being to add a travis run that exercises the latter combination to deny the introduction of violations. #14920 (comment)

Empact renamed this:
~~build: Enable -Werror=zero-as-null-pointer-constant~~
build: Optionally enable -Wzero-as-null-pointer-constant
on Jan 9, 2019

Empact force-pushed on Jan 9, 2019

DrahtBot commented at 10:49 pm on January 9, 2019: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24914 (wallet: Load database records in a particular order by achow101)
#24322 ([kernel 1/n] Introduce initial libbitcoinkernel by dongcarl)
#24185 (refactor: only use explicit reinterpret/const casts, not implicit by PastaPastaPasta)

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 Needs rebase on Jan 12, 2019

Empact force-pushed on Jan 13, 2019

DrahtBot removed the label Needs rebase on Jan 13, 2019

Empact commented at 5:48 pm on January 13, 2019: member

Rebased for #13216

DrahtBot added the label Needs rebase on Jan 13, 2019

Empact force-pushed on Jan 14, 2019

Empact commented at 8:43 am on January 14, 2019: member

Rebased for #15154

DrahtBot removed the label Needs rebase on Jan 14, 2019

benthecarman referenced this in commit 76335298f4 on Jan 14, 2019

Empact force-pushed on Jan 14, 2019

Empact commented at 6:02 am on January 16, 2019: member

AppVeyor failure is spurious, solved by #15171. Now that #15114 is in, the diff is much smaller here. Still should start with #14920.

practicalswift commented at 7:54 am on January 16, 2019: contributor

@Empact Appveyor restarted.

Empact force-pushed on Jan 16, 2019

DrahtBot added the label Needs rebase on Jan 19, 2019

Empact force-pushed on Jan 20, 2019

Empact commented at 4:58 am on January 20, 2019: member

Rebased for #15175

fanquake removed the label Needs rebase on Jan 20, 2019

Empact force-pushed on Jan 20, 2019

practicalswift commented at 8:34 am on January 20, 2019: contributor

utACK b4996079cda89711c2b8b80adbbb06a80f7b9c86

Empact force-pushed on Jan 23, 2019

Empact force-pushed on Feb 9, 2019

Empact commented at 11:49 pm on February 9, 2019: member

Rebased due to conflicts in #15377

DrahtBot added the label Needs rebase on Mar 4, 2019

Empact force-pushed on Mar 5, 2019

Empact commented at 5:39 am on March 5, 2019: member

Rebased for #15288

DrahtBot removed the label Needs rebase on Mar 5, 2019

Empact force-pushed on Mar 7, 2019

practicalswift commented at 4:41 pm on March 15, 2019: contributor

FWIW: I’ve verified that a disassembly of the bitcoind binary built with this patch applied is identical to a disassembly of the bitcoind binary built against master (as expected).

laanwj commented at 8:56 am on June 6, 2019: member

I get tons of these warnings (almost on every file):

0/home/user/src/bitcoin/src/tinyformat.h:119:32: warning: unknown warning group '-Wzero-as-null-pointer-constant', ignored [-Wunknown-pragmas]
1#pragma GCC diagnostic ignored "-Wzero-as-null-pointer-constant"
2                               ^

This is with clang version 3.8.1-24 (tags/RELEASE_381/final) (which is old as the hills but we apparently still support).

Empact force-pushed on Jun 6, 2019

Empact force-pushed on Jun 7, 2019

promag commented at 9:57 am on June 9, 2019: member

Failing in travis with

0In file included from ./index/base.h:8:0,
1                 from index/base.cpp:6:
2./dbwrapper.h:16:10: fatal error: leveldb/db.h: No such file or directory
3 #include <leveldb/db.h>

Please update OP as it seems outdated.

Empact commented at 12:02 pm on June 9, 2019: member

Yeah @promag this is wip: rebasing and addressing @laanwj’s comment

Empact force-pushed on Jun 10, 2019

Empact force-pushed on Jun 17, 2019

DrahtBot added the label Needs rebase on Jul 24, 2019

Empact force-pushed on Jan 17, 2020

DrahtBot removed the label Needs rebase on Jan 17, 2020

Empact force-pushed on Jan 17, 2020

practicalswift commented at 4:25 pm on January 18, 2020: contributor

@Empact Seems like the current version does not compile?

Empact force-pushed on Jan 19, 2020

DrahtBot added the label Needs rebase on Feb 16, 2020

Empact force-pushed on Feb 20, 2020

DrahtBot removed the label Needs rebase on Feb 20, 2020

kallewoof approved

kallewoof commented at 6:33 am on March 6, 2020: member

ACK fc19862337f0764104a918f22a97c47fb5dcc304

Reviewed code
Tested locally; managed to trigger warning when swapping a nullptr for 0. (Was unable to get a documentation warning by messing with a @param varname , though, for some reason…)

DrahtBot added the label Needs rebase on Mar 6, 2020

practicalswift commented at 12:52 pm on March 6, 2020: contributor

I’ll happily re-review and re-ACK after rebase – hopefully we can get this in soon :)

Empact force-pushed on Mar 9, 2020

DrahtBot removed the label Needs rebase on Mar 9, 2020

DrahtBot added the label Needs rebase on Mar 26, 2020

Empact force-pushed on Apr 13, 2021

DrahtBot removed the label Needs rebase on Apr 13, 2021

laanwj commented at 9:45 am on April 13, 2021: member

Fails in leveldb (shouldn’t it skip dependencies?):

0./leveldb/db/skiplist.h:328:21: error: zero as null pointer constant [-Werror=zero-as-null-pointer-constant]
1       head_(NewNode(0 /* any key will do */, kMaxHeight)),
2                     ^
3cc1plus: some warnings being treated as errors

in configure.ac:448 in 2036b7606f outdated

441@@ -442,6 +442,10 @@ if test "x$enable_werror" = "xyes"; then
442 
443   if test x$suppress_external_warnings != xno ; then
444     AX_CHECK_COMPILE_FLAG([-Werror=documentation],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=documentation"],,[[$CXXFLAG_WERROR]])
445+    AX_CHECK_COMPILE_FLAG([-Werror=zero-as-null-pointer-constant],[
446+      WARN_CXXFLAGS="$WARN_CXXFLAGS -Werror=zero-as-null-pointer-constant"
447+      AC_DEFINE(HAVE_ZERO_AS_NULL_POINTER_CONSTANT, 1, [Define this symbol if -Wzero-as-null-pointer-constant is available])
448+    ],,[[$CXXFLAG_WERROR]])

hebasto commented at 11:02 am on April 13, 2021:

One pair of [] is redundant:

0    AX_CHECK_COMPILE_FLAG([-Werror=zero-as-null-pointer-constant], [
1      WARN_CXXFLAGS="$WARN_CXXFLAGS -Werror=zero-as-null-pointer-constant"
2      AC_DEFINE(HAVE_ZERO_AS_NULL_POINTER_CONSTANT, 1, [Define this symbol if -Wzero-as-null-pointer-constant is available])
3    ], [], [$CXXFLAG_WERROR])

Empact commented at 4:30 pm on April 13, 2021:

I’m an autoconf novice - I take it we’re not concerned about macro conflicts with the CXXFLAG_WERROR as its contents are of a different form than uppercase macro definitions?

When you use the same text in a macro argument, you must therefore have an extra quotation level (since one is stripped away by the macro substitution). In general, then, it is a good idea to use double quoting for all literal string arguments, either around just the problematic portions, or over the entire argument:
AC_MSG_WARN([[AC_DC] stinks  --Iron Maiden])
AC_MSG_WARN([[AC_DC stinks  --Iron Maiden]])

https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html_node/Autoconf-Language.html#Autoconf-Language

in configure.ac:482 in 2036b7606f outdated

475@@ -472,6 +476,10 @@ if test "x$CXXFLAGS_overridden" = "xno"; then
476 
477   if test x$suppress_external_warnings != xno ; then
478     AX_CHECK_COMPILE_FLAG([-Wdocumentation],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wdocumentation"],,[[$CXXFLAG_WERROR]])
479+    AX_CHECK_COMPILE_FLAG([-Wzero-as-null-pointer-constant],[
480+      WARN_CXXFLAGS="$WARN_CXXFLAGS -Wzero-as-null-pointer-constant"
481+      AC_DEFINE(HAVE_ZERO_AS_NULL_POINTER_CONSTANT, 1, [Define this symbol if -Wzero-as-null-pointer-constant is available])
482+    ],,[[$CXXFLAG_WERROR]])

hebasto commented at 11:02 am on April 13, 2021:

One pair of [] is redundant:

0    AX_CHECK_COMPILE_FLAG([-Wzero-as-null-pointer-constant], [
1      WARN_CXXFLAGS="$WARN_CXXFLAGS -Wzero-as-null-pointer-constant"
2      AC_DEFINE(HAVE_ZERO_AS_NULL_POINTER_CONSTANT, 1, [Define this symbol if -Wzero-as-null-pointer-constant is available])
3    ], [], [$CXXFLAG_WERROR])

hebasto commented at 11:04 am on April 13, 2021: member

Approach ACK 2036b7606f16a76a300f067824168de6536c60c8

I think the following patch could help:

 0--- a/src/Makefile.leveldb.include
 1+++ b/src/Makefile.leveldb.include
 2@@ -36,7 +36,7 @@ LEVELDB_CPPFLAGS_INT += -DLEVELDB_PLATFORM_POSIX
 3 endif
 4 
 5 leveldb_libleveldb_a_CPPFLAGS = $(AM_CPPFLAGS) $(LEVELDB_CPPFLAGS_INT) $(LEVELDB_CPPFLAGS)
 6-leveldb_libleveldb_a_CXXFLAGS = $(filter-out -Wconditional-uninitialized -Werror=conditional-uninitialized -Wsuggest-override -Werror=suggest-override, $(AM_CXXFLAGS)) $(PIE_FLAGS)
 7+leveldb_libleveldb_a_CXXFLAGS = $(filter-out -Wconditional-uninitialized -Werror=conditional-uninitialized -Wsuggest-override -Werror=suggest-override -Wzero-as-null-pointer-constant -Werror=zero-as-null-pointer-constant, $(AM_CXXFLAGS)) $(PIE_FLAGS)
 8 
 9 leveldb_libleveldb_a_SOURCES=
10 leveldb_libleveldb_a_SOURCES += leveldb/port/port_stdcxx.h

In the OP could drop “Used Qt::NoItemFlags, Qt::Widget instead where applicable.” ?

In commit “Don’t use zero as null pointer constant (-Wzero-as-null-pointer-constant)” (2036b7606f16a76a300f067824168de6536c60c8) could drop “Protobuf generated files via pragmas inserted via src/Makefile.am” ?

The name HAVE_ZERO_AS_NULL_POINTER_CONSTANT seems confusing, as actually it means an activated warning, not a feature.

Empact force-pushed on Apr 13, 2021

Empact commented at 4:22 pm on April 13, 2021: member

@hebasto thanks for the review - I switched to HAVE_W_ZERO_AS_NULL_POINTER_CONSTANT - do you think that’s clearer? Open to suggestions.

Empact force-pushed on Apr 13, 2021

hebasto commented at 7:32 pm on April 13, 2021: member

https://cirrus-ci.com/task/6169333320646656?logs=ci#L2845

0test/util_tests.cpp:1805:48: error: zero as null pointer constant [-Werror,-Wzero-as-null-pointer-constant]
1    void (*old_handler)(int) = signal(SIGCHLD, SIG_DFL);
2                                               ^
3/usr/include/x86_64-linux-gnu/bits/signum-generic.h:29:37: note: expanded from macro 'SIG_DFL'
4#define SIG_DFL  ((__sighandler_t)  0)  /* Default action.  */
5                                    ^
61 error generated.

Empact force-pushed on Apr 14, 2021

Empact force-pushed on Apr 21, 2021

practicalswift commented at 6:21 am on April 22, 2021: contributor

cr ACK 7f0f2ca49f23cfd9e28892137e13f057a2152a91: patch looks correct

in src/tinyformat.h:136 in 7f0f2ca49f outdated

130+
131+#if defined(HAVE_W_ZERO_AS_NULL_POINTER_CONSTANT)
132+#pragma GCC diagnostic push
133+#pragma GCC diagnostic ignored "-Wzero-as-null-pointer-constant"
134+#pragma clang diagnostic push
135+#pragma clang diagnostic ignored "-Wzero-as-null-pointer-constant"

hebasto commented at 5:26 pm on April 29, 2021:

With gcc 9.3.0:

0./tinyformat.h:134: warning: ignoring #pragma clang diagnostic [-Wunknown-pragmas]
1  134 | #pragma clang diagnostic push
2      | 
3./tinyformat.h:135: warning: ignoring #pragma clang diagnostic [-Wunknown-pragmas]
4  135 | #pragma clang diagnostic ignored "-Wzero-as-null-pointer-constant"
5      |

maflcko commented at 9:07 am on April 23, 2022:

Might be better to do with clang-tidy or so externally instead of maiming the source code?

fanquake commented at 12:48 pm on April 25, 2022:

Might be better to do with clang-tidy

Have done this in #24971.

in src/tinyformat.h:1179 in 7f0f2ca49f outdated

1173@@ -1163,4 +1174,9 @@ std::string format(const std::string &fmt, const Args&... args)
1174 /** Format arguments and return the string or write to given std::ostream (see tinyformat::format doc for details) */
1175 #define strprintf tfm::format
1176 
1177+#if defined(HAVE_W_ZERO_AS_NULL_POINTER_CONSTANT)
1178+#pragma GCC diagnostic pop
1179+#pragma clang diagnostic pop

hebasto commented at 5:27 pm on April 29, 2021:

With gcc 9.3.0:

0./tinyformat.h:1179: warning: ignoring #pragma clang diagnostic [-Wunknown-pragmas]
1 1179 | #pragma clang diagnostic pop
2      |

Empact commented at 9:27 pm on April 22, 2022:

I made the changes in https://github.com/bitcoin/bitcoin/pull/15112/commits/0b3c3552f4ca7d0d151265adff35bdfceac5f380 in an attempt to fix this - willing to test?

in src/wallet/sqlite.cpp:374 in 7f0f2ca49f outdated

364@@ -365,6 +365,14 @@ void SQLiteBatch::Close()
365     }
366 }
367 
368+// disable -Wzero-as-null-pointer-constant due to SQLITE_STATIC

hebasto commented at 5:31 pm on April 29, 2021:

If I remove changes in this file I won’t receive warning with both gcc 9.3.0 and clang 10.0.0. What cases are guarded here?

Having system sqlite 3.31.1.

Empact commented at 1:10 pm on October 14, 2021:

Here’s the failure I get on removing that:

 0  CXX      wallet/libbitcoin_wallet_a-sqlite.o
 1wallet/sqlite.cpp:379:73: error: zero as null pointer constant [-Werror,-Wzero-as-null-pointer-constant]
 2    int res = sqlite3_bind_blob(m_read_stmt, 1, key.data(), key.size(), SQLITE_STATIC);
 3                                                                        ^
 4/usr/local/Cellar/sqlite/3.36.0/include/sqlite3.h:5666:54: note: expanded from macro 'SQLITE_STATIC'
 5#define SQLITE_STATIC      ((sqlite3_destructor_type)0)
 6                                                     ^
 7wallet/sqlite.cpp:420:66: error: zero as null pointer constant [-Werror,-Wzero-as-null-pointer-constant]
 8    int res = sqlite3_bind_blob(stmt, 1, key.data(), key.size(), SQLITE_STATIC);
 9                                                                 ^
10/usr/local/Cellar/sqlite/3.36.0/include/sqlite3.h:5666:54: note: expanded from macro 'SQLITE_STATIC'
11#define SQLITE_STATIC      ((sqlite3_destructor_type)0)
12                                                     ^
13wallet/sqlite.cpp:427:66: error: zero as null pointer constant [-Werror,-Wzero-as-null-pointer-constant]
14    res = sqlite3_bind_blob(stmt, 2, value.data(), value.size(), SQLITE_STATIC);
15                                                                 ^
16/usr/local/Cellar/sqlite/3.36.0/include/sqlite3.h:5666:54: note: expanded from macro 'SQLITE_STATIC'
17#define SQLITE_STATIC      ((sqlite3_destructor_type)0)
18                                                     ^
19wallet/sqlite.cpp:451:75: error: zero as null pointer constant [-Werror,-Wzero-as-null-pointer-constant]
20    int res = sqlite3_bind_blob(m_delete_stmt, 1, key.data(), key.size(), SQLITE_STATIC);
21                                                                          ^
22/usr/local/Cellar/sqlite/3.36.0/include/sqlite3.h:5666:54: note: expanded from macro 'SQLITE_STATIC'
23#define SQLITE_STATIC      ((sqlite3_destructor_type)0)
24                                                     ^
25wallet/sqlite.cpp:476:73: error: zero as null pointer constant [-Werror,-Wzero-as-null-pointer-constant]
26    int res = sqlite3_bind_blob(m_read_stmt, 1, key.data(), key.size(), SQLITE_STATIC);
27                                                                        ^
28/usr/local/Cellar/sqlite/3.36.0/include/sqlite3.h:5666:54: note: expanded from macro 'SQLITE_STATIC'
29#define SQLITE_STATIC      ((sqlite3_destructor_type)0)
30                                                     ^
315 errors generated.
32make[2]: *** [wallet/libbitcoin_wallet_a-sqlite.o] Error 1

BTW:

0% clang --version
1Apple clang version 12.0.5 (clang-1205.0.22.11)
2Target: x86_64-apple-darwin20.6.0
3Thread model: posix
4InstalledDir: /Library/Developer/CommandLineTools/usr/bin

Munkybooty referenced this in commit 9c8420a241 on Aug 21, 2021

Munkybooty referenced this in commit d4669e1898 on Aug 23, 2021

Munkybooty referenced this in commit 8f57bae102 on Aug 24, 2021

Munkybooty referenced this in commit 41291f2c18 on Aug 24, 2021

Munkybooty referenced this in commit eed7c1f4d4 on Aug 24, 2021

UdjinM6 referenced this in commit 485cf5ac70 on Aug 24, 2021

Munkybooty referenced this in commit f239861eba on Aug 24, 2021

DrahtBot added the label Needs rebase on Oct 13, 2021

Empact force-pushed on Oct 14, 2021

DrahtBot removed the label Needs rebase on Oct 14, 2021

DrahtBot added the label Needs rebase on Nov 13, 2021

Empact force-pushed on Feb 28, 2022

DrahtBot removed the label Needs rebase on Feb 28, 2022

Empact force-pushed on Mar 2, 2022

Don't use zero as null pointer constant (-Wzero-as-null-pointer-constant) f1bdd58cda

build: Enable -Wzero-as-null-pointer-constant under suppress_external_warnings

And -Werror=zero-as-null-pointer-constant if werror are enabled.

While disabling it in the following dependencies:
* LevelDb via build flags
* Tinyformat via pragmas due to external library
* Sqlite via pragmas due to use of SQLITE_STATIC
* util_tests via pragmas due to use of SIG_DFL

0e45825838

Empact referenced this in commit 58ca282f9b on Apr 22, 2022

Empact force-pushed on Apr 22, 2022

Empact commented at 9:25 pm on April 22, 2022: member

Rebase for the move of SQLITE_STATIC that occurred in #23732

Ignore gcc -Wunknown-pragmas in tinyformat.h

As without this, GCC warns on the following unrecognized clang pragmas.
https://github.com/bitcoin/bitcoin/pull/15112#discussion_r623251609

0b3c3552f4

Empact force-pushed on Apr 22, 2022

fanquake commented at 10:15 am on April 26, 2022: member

Going to close this in favour of #24971; where we outsource this check to external tooling, and don’t have to cover the codebase in pragmas to satisfy compilers.

fanquake closed this on Apr 26, 2022

fanquake referenced this in commit cc3877f831 on Apr 26, 2022

Empact deleted the branch on Apr 26, 2022

Empact commented at 5:57 pm on April 26, 2022: member

Sounds good, one of these days I’ll familiarize myself with clang-tidy.

sidhujag referenced this in commit 09047541df on Apr 26, 2022

gades referenced this in commit 54ed72391d on May 8, 2022

PastaPastaPasta referenced this in commit 8b56495b90 on Oct 18, 2022

Empact restored the branch on Nov 18, 2022

bitcoin locked this on Nov 18, 2023

build: Optionally enable -Wzero-as-null-pointer-constant #15112

Conflicts