public inbox for
 help / color / mirror / Atom feed
From: Andrew Burgess <>
To: Jan-Benedict Glaw <>
Subject: Re: Building with recent GCC versions: gdbsupport/gdb_assert.h:35:4: error: 'nonnull' argument 'filename' compared to NULL [-Werror=nonnull-compare]
Date: Tue, 27 Jul 2021 11:03:54 +0100	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

* Jan-Benedict Glaw <> [2021-07-26 23:11:01 +0200]:

> Hi!
> I'm running some CI builds and noticed that, when building GDB with
> quite recent GCC versions, it breaks.
> With ie. this "gcc-snapshot" GCC from Debian:
> /usr/lib/gcc-snapshot/bin/gcc --version
> gcc (Debian 20210630-1) 12.0.0 20210630 (experimental) [master revision 6bf383c37e6:93c270320bb:35da8a98026849bd20d16bbf9210ac1d0b44ea6a]
> we see:
> ./configure --target=i686-linux --prefix=/tmp/gdb-i686-linux
> [...]
> all make V=1 all-gdb
> [...]
> [all 2021-07-26 20:39:22] /usr/lib/gcc-snapshot/bin/g++ -x c++    -I. -I. -I./config -DLOCALEDIR="\"/tmp/gdb-i686-linux/share/locale\"" -DHAVE_CONFIG_H -I./../include/opcode -I./../readline/readline/.. -I./../zlib -I../bfd -I./../bfd -I./../include -I../libdecnumber -I./../libdecnumber  -I./../gnulib/import -I../gnulib/import -I./.. -I..  -DTUI=1    -I./.. -pthread  -Wall -Wpointer-arith -Wno-unused -Wunused-value -Wunused-variable -Wunused-function -Wno-switch -Wno-char-subscripts -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable -Wno-sign-compare -Wno-error=maybe-uninitialized -Wno-mismatched-tags -Wsuggest-override -Wimplicit-fallthrough=3 -Wduplicated-cond -Wshadow=local -Wdeprecated-copy -Wdeprecated-copy-dtor -Wredundant-move -Wmissing-declarations -Wstrict-null-sentinel -Wformat -Wformat-nonliteral -Werror -g -O2   -c -o compile/compile.o -MT compile/compile.o -MMD -MP -MF compile/.deps/compile.Tpo compile/compile.c
> [all 2021-07-26 20:39:26] In file included from ./../gdbsupport/common-defs.h:126,
> [all 2021-07-26 20:39:26]                  from ./defs.h:28,
> [all 2021-07-26 20:39:26]                  from compile/compile.c:20:
> [all 2021-07-26 20:39:26] ./../gdbsupport/gdb_unlinker.h: In constructor 'gdb::unlinker::unlinker(const char*)':
> [all 2021-07-26 20:39:26] ./../gdbsupport/gdb_assert.h:35:4: error: 'nonnull' argument 'filename' compared to NULL [-Werror=nonnull-compare]
> [all 2021-07-26 20:39:26]    35 |   ((void) ((expr) ? 0 :                                                       \
> [all 2021-07-26 20:39:26]       |   ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> [all 2021-07-26 20:39:26]    36 |            (gdb_assert_fail (#expr, __FILE__, __LINE__, FUNCTION_NAME), 0)))
> [all 2021-07-26 20:39:26]       |            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> [all 2021-07-26 20:39:26] ./../gdbsupport/gdb_unlinker.h:38:5: note: in expansion of macro 'gdb_assert'
> [all 2021-07-26 20:39:26]    38 |     gdb_assert (filename != NULL);
> [all 2021-07-26 20:39:26]       |     ^~~~~~~~~~
> [all 2021-07-26 20:39:27] cc1plus: all warnings being treated as errors
> [all 2021-07-26 20:39:27] make[1]: *** [Makefile:1642: compile/compile.o] Error 1
> [all 2021-07-26 20:39:27] make[1]: Leaving directory '/var/lib/laminar/run/gdb-i686-linux/4/binutils-gdb/gdb'
> [all 2021-07-26 20:39:27] make: *** [Makefile:11410: all-gdb] Error 2
> I also discussed this on the GCC patches mailing list
> (,
> where Martin suggested that this should be fixed here in GDB.
> Any thoughts about this?

As I understand it the nonnull attribute only provides compile time
protection against explicitly passing NULL, there's no compiled in
non-null check (well, maybe with -fisolate-erroneous-paths-attribute,
but the assert might give a better error message).

This means its still possible to pass NULL to a nonnull function, its
just the behaviour of the program is undefined in that case.

So, it doesn't seem crazy that we might want to both (a) have a
function declared nonnull, to prevent explicitly passing NULL, and (b)
have a NULL check inside the function to catch logic bugs that result
in NULL being passed.

We could, of course, push the assert outside of the function, but that
would really suck due to code duplication, and the risk of missing an
assert, so that seems like a non-starter.

We could drop either the assert, or the nonnull attribute, but that
would suck as both give a valuable, but different form of protection.

After some experimenting, I suspect that the assert is being optimised
away anyway, which kind of makes sense, as we're telling the compiler
it can assume that the pointer is non-null.

So, what we probably want is someway to tell (or trick) GCC into
including the null check even in the nonnull function....

... here's what I came up with, add this somewhere:

 template<typename T>
 bool __attribute__ ((noinline))
 nonnull_arg_is_really_not_null (const T *ptr)
   return ptr != nullptr;

then change the assert to:

 gdb_assert (nonnull_arg_is_really_not_null (filename));

Seems to keep the assert, and silence the warning.  Thoughts?



diff --git a/gdbsupport/gdb_assert.h b/gdbsupport/gdb_assert.h
index 00553a78613..478bf10ec24 100644
--- a/gdbsupport/gdb_assert.h
+++ b/gdbsupport/gdb_assert.h
@@ -58,4 +58,26 @@
   internal_error (__FILE__, __LINE__, _(message))
+/* If a pointer argument to a function is marked as nonnull (using the
+   nonnull attribute) then GCC will warn about any attempts to compare the
+   pointer to nullptr.  Even if you can silence the warning GCC will
+   likely optimize out the check (and any associated code block)
+   completely.
+   Normally this would be what you want, but, marking an argument nonnull
+   doesn't guarantee that nullptr can't be passed.  So it's not
+   unreasonable that we might want to add an assert that the argument is
+   not nullptr.
+   This function should be used for this purpose, like:
+   gdb_assert (nonnull_arg_is_not_nullptr (arg_name));  */
+template<typename T>
+nonnull_arg_is_not_nullptr (const T *ptr)
+  return ptr != nullptr;
 #endif /* COMMON_GDB_ASSERT_H */
diff --git a/gdbsupport/gdb_unlinker.h b/gdbsupport/gdb_unlinker.h
index bda6fe7ab54..ec159c2166a 100644
--- a/gdbsupport/gdb_unlinker.h
+++ b/gdbsupport/gdb_unlinker.h
@@ -35,7 +35,7 @@ class unlinker
   unlinker (const char *filename) ATTRIBUTE_NONNULL (2)
     : m_filename (filename)
-    gdb_assert (filename != NULL);
+    gdb_assert (nonnull_arg_is_not_nullptr (filename));
   ~unlinker ()

  reply	other threads:[~2021-07-27 10:03 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-26 21:11 Jan-Benedict Glaw
2021-07-27 10:03 ` Andrew Burgess [this message]
2021-07-27 10:44   ` Tom de Vries
2021-07-27 11:35     ` Andrew Burgess
2021-07-27 11:49       ` Tom de Vries
2021-07-27 13:38         ` Tom de Vries
2021-07-28  8:37           ` Andrew Burgess
2021-07-31 21:18 ` CI Builds (was: Building with recent GCC versions: gdbsupport/gdb_assert.h:35:4: error: 'nonnull' argument 'filename' compared to NULL [-Werror=nonnull-compare]) Jan-Benedict Glaw
2021-08-01 23:35   ` CI Builds Sergio Durigan Junior

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).