[gdb/build] Fix Werror=nonnull-compare build breaker with gcc 12

[gdb/build] Fix Werror=nonnull-compare build breaker with gcc 12

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 8150 bytes --]

[ was : Re: Building with recent GCC versions:
gdbsupport/gdb_assert.h:35:4: error: 'nonnull' argument 'filename'
compared to NULL [-Werror=nonnull-compare] ]

[ Moving discussion to gdb-patches ]

On 7/27/21 3:38 PM, Tom de Vries wrote:
> On 7/27/21 1:49 PM, Tom de Vries wrote:
>> On 7/27/21 1:35 PM, Andrew Burgess wrote:
>>> * Tom de Vries <tdevries@suse.de> [2021-07-27 12:44:10 +0200]:
>>>
>>>> On 7/27/21 12:03 PM, Andrew Burgess wrote:
>>>>> * Jan-Benedict Glaw <jbglaw@lug-owl.de> [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
>>>>>> (https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575568.html),
>>>>>> 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.
>>>>>
>>>>
>>>> Yes, in fact that's what the nonnull-compare warning specifically warns
>>>> against: there's some code that may be optimized away, due to the
>>>> nonnull attribute.
>>>>
>>>>> 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?
>>>>>
>>>>
>>>> I understand why it works, but it seems fragile to me.  At some point
>>>> some compiler may get smart enough to also optimize this case, and then
>>>> we're back in the same situation.
>>>
>>> Good point.
>>>
>>> The GCC documentation for noinline[1] suggests we can avoid the call
>>> being removed by adding 'asm ("");' into the function:
>>>
>>>   template<typename T>
>>>   bool __attribute__ ((noinline))
>>>   nonnull_arg_is_really_not_null (const T *ptr)
>>>   {
>>>     asm ("");
>>>     return ptr != nullptr;
>>>   }
>>>
>>> I'm not really arguing for this approach over any other, just sharing
>>> what I discovered.
>>>
>>
>> Ack, understood.  Note that the added asm doesn't stop a compiler from
>> doing:
>> ...
>> gdb_assert (nonnull_arg_is_really_not_null (filename));
>> ...
>> ->
>> ...
>> nonnull_arg_is_really_not_null (filename);
>> gdb_assert (true);
>> ...
>>
>> Thanks,
>> - Tom
>>
>>> Thanks,
>>> Andrew
>>>
>>> [1] https://gcc.gnu.org/onlinedocs/gcc-11.1.0/gcc/Common-Function-Attributes.html#Common-Function-Attributes
>>>
>>>>
>>>> I wonder whether using volatile is a better idea (can't try this out
>>>> right now).
>>>>
> 
> I was thinking of something like this:
> ...
> diff --git a/gdbsupport/gdb_unlinker.h b/gdbsupport/gdb_unlinker.h
> index bda6fe7ab54..3d99b41e7ad 100644
> --- a/gdbsupport/gdb_unlinker.h
> +++ b/gdbsupport/gdb_unlinker.h
> @@ -20,6 +20,13 @@
>  #ifndef COMMON_GDB_UNLINKER_H
>  #define COMMON_GDB_UNLINKER_H
> 
> +template<typename T>
> +const T *volatile
> +ignore_nonnull (const T *ptr)
> +{
> +  return ptr;
> +}
> +
>  namespace gdb
>  {
> 
> @@ -35,7 +42,7 @@ class unlinker
>    unlinker (const char *filename) ATTRIBUTE_NONNULL (2)
>      : m_filename (filename)
>    {
> -    gdb_assert (filename != NULL);
> +    gdb_assert (ignore_nonnull (filename) != NULL);
>    }
> 
>    ~unlinker ()
> ...
> 
> This builds for me, though I haven't got a setup yet where the warning
> reproduces, so I can't check whether it actually fixes things.

I managed now to reproduce, and wrote a patch along these lines.

Any comments?

In particular, any suggestion where to put ignore_nonnull?

Or, is it perhaps a better idea to have a gdb_assert_nonnull and
implement things there?

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-build-Fix-Werror-nonnull-compare-build-breaker-with-gcc-12.patch --]
[-- Type: text/x-patch, Size: 4018 bytes --]

[gdb/build] Fix Werror=nonnull-compare build breaker with gcc 12

When building gdb using current gcc trunk, we run into:
...
gdbsupport/gdb_unlinker.h: \
  In constructor 'gdb::unlinker::unlinker(const char*)':
gdbsupport/gdb_assert.h:35:4: error: \
  'nonnull' argument 'filename' compared to NULL [-Werror=nonnull-compare]
35| ((void) ((expr) ? 0 :                                                    \
  | ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
36|          (gdb_assert_fail (#expr, __FILE__, __LINE__, FUNCTION_NAME), 0)))
  |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
gdbsupport/gdb_unlinker.h:38:5: note: in expansion of macro 'gdb_assert'
38|   gdb_assert (filename != NULL);
  |   ^~~~~~~~~~
cc1plus: all warnings being treated as errors
make[1]: *** [compile/compile.o] Error 1
...

The warning triggers in this code:
...
  unlinker (const char *filename) ATTRIBUTE_NONNULL (2)
    : m_filename (filename)
  {
    gdb_assert (filename != NULL);
  }
...

The attribute nonnull (applied here to the filename parameter) has two
effects:
- if the compiler determines that a null pointer is passed in argument
  filename, and the -Wnonnull option is enabled, a warning is issued.
- the compiler may perform optimizations based on the knowledge that
  filename != NULL (unless disabled by the -fno-delete-null-pointer-checks
  option).

The warning Werror=nonnull-compare warns that the compiler may perform the
optimization:
....
    gdb_assert (filename != NULL);
...
->
...
    gdb_assert (true);
...
in which case "unlinker (obfuscated_NULL)" no longer will trigger the assert.
And we want to keep the gdb_assert to detect cases that -Wnonnull doesn't
detect.

We could simply fix this by dropping the attribute, but that means that we no
longer get the -Wnonnull warning.

Fix this by ignoring the nonnull attribute using a function:
...
template<typename T>
const T *volatile
ignore_nonnull (const T *ptr)
{
  return ptr;
}
...
such that we can do:
...
    gdb_assert (ignore_nonnull (filename) != NULL);
...

Build on x86_64-linux using "gcc version 12.0.0 20210727 (experimental) (GCC)"
build from gcc commit 7ffba77d01a.

Reported-By: Jan-Benedict Glaw <jbglaw@lug-owl.de>

gdb/ChangeLog:

2021-07-27  Andrew Burgess  <andrew.burgess@embecosm.com>
	    Tom de Vries  <tdevries@suse.de>

	* gdbsupport/gdb_assert.h (ignore_nonnull): New function.
	* gdb/gdb_regex.c (compiled_regex::compiled_regex): Use ignore_nonnull
	to ignore nonnull attribute on function parameter.
	* gdbsupport/gdb_unlinker.h (unlinker::unlinker): Same.

---
 gdb/gdb_regex.c           | 4 ++--
 gdbsupport/gdb_assert.h   | 6 ++++++
 gdbsupport/gdb_unlinker.h | 2 +-
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/gdb/gdb_regex.c b/gdb/gdb_regex.c
index 17fbfd28ee8..22473f57e26 100644
--- a/gdb/gdb_regex.c
+++ b/gdb/gdb_regex.c
@@ -22,8 +22,8 @@
 compiled_regex::compiled_regex (const char *regex, int cflags,
 				const char *message)
 {
-  gdb_assert (regex != NULL);
-  gdb_assert (message != NULL);
+  gdb_assert (ignore_nonnull (regex) != NULL);
+  gdb_assert (ignore_nonnull (message) != NULL);
 
   int code = regcomp (&m_pattern, regex, cflags);
   if (code != 0)
diff --git a/gdbsupport/gdb_assert.h b/gdbsupport/gdb_assert.h
index 00553a78613..26a9594e7f8 100644
--- a/gdbsupport/gdb_assert.h
+++ b/gdbsupport/gdb_assert.h
@@ -58,4 +58,10 @@
   internal_error (__FILE__, __LINE__, _(message))
 #endif
 
+template<typename T>
+const T *volatile
+ignore_nonnull (const T *ptr)
+{
+  return ptr;
+}
 #endif /* COMMON_GDB_ASSERT_H */
diff --git a/gdbsupport/gdb_unlinker.h b/gdbsupport/gdb_unlinker.h
index bda6fe7ab54..4aa4f3fc3e9 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 (ignore_nonnull (filename) != NULL);
   }
 
   ~unlinker ()

Re: [gdb/build] Fix Werror=nonnull-compare build breaker with gcc 12

[Andrew Burgess]

* Tom de Vries <tdevries@suse.de> [2021-07-27 18:28:30 +0200]:

> [ was : Re: Building with recent GCC versions:
> gdbsupport/gdb_assert.h:35:4: error: 'nonnull' argument 'filename'
> compared to NULL [-Werror=nonnull-compare] ]
> 
> [ Moving discussion to gdb-patches ]
> 
> On 7/27/21 3:38 PM, Tom de Vries wrote:
> > On 7/27/21 1:49 PM, Tom de Vries wrote:
> >> On 7/27/21 1:35 PM, Andrew Burgess wrote:
> >>> * Tom de Vries <tdevries@suse.de> [2021-07-27 12:44:10 +0200]:
> >>>
> >>>> On 7/27/21 12:03 PM, Andrew Burgess wrote:
> >>>>> * Jan-Benedict Glaw <jbglaw@lug-owl.de> [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
> >>>>>> (https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575568.html),
> >>>>>> 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.
> >>>>>
> >>>>
> >>>> Yes, in fact that's what the nonnull-compare warning specifically warns
> >>>> against: there's some code that may be optimized away, due to the
> >>>> nonnull attribute.
> >>>>
> >>>>> 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?
> >>>>>
> >>>>
> >>>> I understand why it works, but it seems fragile to me.  At some point
> >>>> some compiler may get smart enough to also optimize this case, and then
> >>>> we're back in the same situation.
> >>>
> >>> Good point.
> >>>
> >>> The GCC documentation for noinline[1] suggests we can avoid the call
> >>> being removed by adding 'asm ("");' into the function:
> >>>
> >>>   template<typename T>
> >>>   bool __attribute__ ((noinline))
> >>>   nonnull_arg_is_really_not_null (const T *ptr)
> >>>   {
> >>>     asm ("");
> >>>     return ptr != nullptr;
> >>>   }
> >>>
> >>> I'm not really arguing for this approach over any other, just sharing
> >>> what I discovered.
> >>>
> >>
> >> Ack, understood.  Note that the added asm doesn't stop a compiler from
> >> doing:
> >> ...
> >> gdb_assert (nonnull_arg_is_really_not_null (filename));
> >> ...
> >> ->
> >> ...
> >> nonnull_arg_is_really_not_null (filename);
> >> gdb_assert (true);
> >> ...
> >>
> >> Thanks,
> >> - Tom
> >>
> >>> Thanks,
> >>> Andrew
> >>>
> >>> [1] https://gcc.gnu.org/onlinedocs/gcc-11.1.0/gcc/Common-Function-Attributes.html#Common-Function-Attributes
> >>>
> >>>>
> >>>> I wonder whether using volatile is a better idea (can't try this out
> >>>> right now).
> >>>>
> > 
> > I was thinking of something like this:
> > ...
> > diff --git a/gdbsupport/gdb_unlinker.h b/gdbsupport/gdb_unlinker.h
> > index bda6fe7ab54..3d99b41e7ad 100644
> > --- a/gdbsupport/gdb_unlinker.h
> > +++ b/gdbsupport/gdb_unlinker.h
> > @@ -20,6 +20,13 @@
> >  #ifndef COMMON_GDB_UNLINKER_H
> >  #define COMMON_GDB_UNLINKER_H
> > 
> > +template<typename T>
> > +const T *volatile
> > +ignore_nonnull (const T *ptr)
> > +{
> > +  return ptr;
> > +}
> > +
> >  namespace gdb
> >  {
> > 
> > @@ -35,7 +42,7 @@ class unlinker
> >    unlinker (const char *filename) ATTRIBUTE_NONNULL (2)
> >      : m_filename (filename)
> >    {
> > -    gdb_assert (filename != NULL);
> > +    gdb_assert (ignore_nonnull (filename) != NULL);
> >    }
> > 
> >    ~unlinker ()
> > ...
> > 
> > This builds for me, though I haven't got a setup yet where the warning
> > reproduces, so I can't check whether it actually fixes things.
> 
> I managed now to reproduce, and wrote a patch along these lines.
> 
> Any comments?
> 
> In particular, any suggestion where to put ignore_nonnull?
> 
> Or, is it perhaps a better idea to have a gdb_assert_nonnull and
> implement things there?
> 
> Thanks,
> - Tom

> [gdb/build] Fix Werror=nonnull-compare build breaker with gcc 12
> 
> When building gdb using current gcc trunk, we run into:
> ...
> gdbsupport/gdb_unlinker.h: \
>   In constructor 'gdb::unlinker::unlinker(const char*)':
> gdbsupport/gdb_assert.h:35:4: error: \
>   'nonnull' argument 'filename' compared to NULL [-Werror=nonnull-compare]
> 35| ((void) ((expr) ? 0 :                                                    \
>   | ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 36|          (gdb_assert_fail (#expr, __FILE__, __LINE__, FUNCTION_NAME), 0)))
>   |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> gdbsupport/gdb_unlinker.h:38:5: note: in expansion of macro 'gdb_assert'
> 38|   gdb_assert (filename != NULL);
>   |   ^~~~~~~~~~
> cc1plus: all warnings being treated as errors
> make[1]: *** [compile/compile.o] Error 1
> ...
> 
> The warning triggers in this code:
> ...
>   unlinker (const char *filename) ATTRIBUTE_NONNULL (2)
>     : m_filename (filename)
>   {
>     gdb_assert (filename != NULL);
>   }
> ...
> 
> The attribute nonnull (applied here to the filename parameter) has two
> effects:
> - if the compiler determines that a null pointer is passed in argument
>   filename, and the -Wnonnull option is enabled, a warning is issued.
> - the compiler may perform optimizations based on the knowledge that
>   filename != NULL (unless disabled by the -fno-delete-null-pointer-checks
>   option).
> 
> The warning Werror=nonnull-compare warns that the compiler may perform the
> optimization:
> ....
>     gdb_assert (filename != NULL);
> ...
> ->
> ...
>     gdb_assert (true);
> ...
> in which case "unlinker (obfuscated_NULL)" no longer will trigger the assert.
> And we want to keep the gdb_assert to detect cases that -Wnonnull doesn't
> detect.
> 
> We could simply fix this by dropping the attribute, but that means that we no
> longer get the -Wnonnull warning.
> 
> Fix this by ignoring the nonnull attribute using a function:
> ...
> template<typename T>
> const T *volatile
> ignore_nonnull (const T *ptr)
> {
>   return ptr;
> }
> ...
> such that we can do:
> ...
>     gdb_assert (ignore_nonnull (filename) != NULL);
> ...
> 
> Build on x86_64-linux using "gcc version 12.0.0 20210727 (experimental) (GCC)"
> build from gcc commit 7ffba77d01a.
> 
> Reported-By: Jan-Benedict Glaw <jbglaw@lug-owl.de>
> 
> gdb/ChangeLog:
> 
> 2021-07-27  Andrew Burgess  <andrew.burgess@embecosm.com>
> 	    Tom de Vries  <tdevries@suse.de>
> 
> 	* gdbsupport/gdb_assert.h (ignore_nonnull): New function.
> 	* gdb/gdb_regex.c (compiled_regex::compiled_regex): Use ignore_nonnull
> 	to ignore nonnull attribute on function parameter.
> 	* gdbsupport/gdb_unlinker.h (unlinker::unlinker): Same.
> 
> ---
>  gdb/gdb_regex.c           | 4 ++--
>  gdbsupport/gdb_assert.h   | 6 ++++++
>  gdbsupport/gdb_unlinker.h | 2 +-
>  3 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/gdb_regex.c b/gdb/gdb_regex.c
> index 17fbfd28ee8..22473f57e26 100644
> --- a/gdb/gdb_regex.c
> +++ b/gdb/gdb_regex.c
> @@ -22,8 +22,8 @@
>  compiled_regex::compiled_regex (const char *regex, int cflags,
>  				const char *message)
>  {
> -  gdb_assert (regex != NULL);
> -  gdb_assert (message != NULL);
> +  gdb_assert (ignore_nonnull (regex) != NULL);
> +  gdb_assert (ignore_nonnull (message) != NULL);
>  
>    int code = regcomp (&m_pattern, regex, cflags);
>    if (code != 0)
> diff --git a/gdbsupport/gdb_assert.h b/gdbsupport/gdb_assert.h
> index 00553a78613..26a9594e7f8 100644
> --- a/gdbsupport/gdb_assert.h
> +++ b/gdbsupport/gdb_assert.h
> @@ -58,4 +58,10 @@
>    internal_error (__FILE__, __LINE__, _(message))
>  #endif
>  
> +template<typename T>
> +const T *volatile
> +ignore_nonnull (const T *ptr)
> +{
> +  return ptr;
> +}

Sorry, I missed that the thread had moved to the patches list.

Using trunk GCC I see a warning like this:

  error: 'volatile'-qualified return type is deprecated [-Werror=volatile]
     21 | const T * volatile
        | ^~~~~
  cc1plus: all warnings being treated as errors
  Compiler returned: 1

See: https://godbolt.org/z/nfhq6zb7q

Thanks,
Andrew

>  #endif /* COMMON_GDB_ASSERT_H */
> diff --git a/gdbsupport/gdb_unlinker.h b/gdbsupport/gdb_unlinker.h
> index bda6fe7ab54..4aa4f3fc3e9 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 (ignore_nonnull (filename) != NULL);
>    }
>  
>    ~unlinker ()

Re: [gdb/build] Fix Werror=nonnull-compare build breaker with gcc 12

[Tom de Vries]

On 7/28/21 11:28 AM, Andrew Burgess wrote:
> Sorry, I missed that the thread had moved to the patches list.
> 

Np, happens to me all the time :)

> Using trunk GCC I see a warning like this:
> 
>   error: 'volatile'-qualified return type is deprecated [-Werror=volatile]
>      21 | const T * volatile
>         | ^~~~~
>   cc1plus: all warnings being treated as errors
>   Compiler returned: 1
> 
> See: https://godbolt.org/z/nfhq6zb7q
> 

I could reproduce that after adding -std=c++20.  Thanks for finding
that, I had no idea about certain uses of volatile being deprecated.

I've looked a bit further into things today, and submitted
"[PATCH][gcc/doc] Improve nonnull attribute documentation" (
https://gcc.gnu.org/pipermail/gcc-patches/2021-July/576218.html ) .

Let's see if we get any useful feedback there.

Thanks,
- Tom

Re: [gdb/build] Fix Werror=nonnull-compare build breaker with gcc 12

[Tom Tromey]

Tom> I managed now to reproduce, and wrote a patch along these lines.

Tom> Any comments?

Tom> In particular, any suggestion where to put ignore_nonnull?

Tom> Or, is it perhaps a better idea to have a gdb_assert_nonnull and
Tom> implement things there?

How about we either drop the nonnull attribute or we use
-fno-delete-null-pointer-checks?

I personally feel that the gcc approach in this area is
counter-productive, at least for our purposes.  My view is that the
point of this stuff is to help us detect programming errors -- and we're
uninterested in using non-null-ness as some kind of optimization hint.
gcc seems to want it both ways, which seems bizarre.  But, given that
this is how the compiler works, IMO we should choose reliability
whichever way we best can.

thanks,
Tom

Re: [gdb/build] Fix Werror=nonnull-compare build breaker with gcc 12

[Tom de Vries]

[ quote-copied from
https://sourceware.org/pipermail/gdb-patches/2021-July/181173.html. I
didn't get this email in either my inbox or gdb-patches folder. ]

> Tom> I managed now to reproduce, and wrote a patch along these lines.
> 
> Tom> Any comments?
> 
> Tom> In particular, any suggestion where to put ignore_nonnull?
> 
> Tom> Or, is it perhaps a better idea to have a gdb_assert_nonnull and
> Tom> implement things there?
> 

Thanks for giving your take on this.

> How about we either drop the nonnull attribute

That's a possibility indeed.

> or we use
> -fno-delete-null-pointer-checks?
> 

Unfortunately, that doesn't work (and it took me some hours today to
realize and understand).  I've submitted a patch to improved the nonnull
documentation (
https://gcc.gnu.org/pipermail/gcc-patches/2021-July/576218.html ),
hopefully it should be clear from there that
-fno-delete-null-pointer-checks doesn't disable the optimization we're
having trouble with.

> I personally feel that the gcc approach in this area is
> counter-productive, at least for our purposes.  My view is that the
> point of this stuff is to help us detect programming errors -- and we're
> uninterested in using non-null-ness as some kind of optimization hint.
> gcc seems to want it both ways, which seems bizarre.

I think they just implemented two attributes: assume_nonnull and
verify_nonnull as a single attribute nonnull.  Then still that could be
workable, but eventually the problem is that you can't switch the two
interpretations on and off in a reasonable manner.

> But, given that
> this is how the compiler works, IMO we should choose reliability
> whichever way we best can.

Yes, I hope to get some reasonable feedback, but I'm not holding my
breath.  So we could possibly end up with dropping nonnull.

Thanks,
- Tom

[master + 11][gdb/build] Disable attribute nonnull

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 2248 bytes --]

[ was: Re: [gdb/build] Fix Werror=nonnull-compare build breaker with gcc
12 ]

On 7/29/21 12:32 AM, Tom de Vries wrote:
> [ quote-copied from
> https://sourceware.org/pipermail/gdb-patches/2021-July/181173.html. I
> didn't get this email in either my inbox or gdb-patches folder. ]
> 
>> Tom> I managed now to reproduce, and wrote a patch along these lines.
>>
>> Tom> Any comments?
>>
>> Tom> In particular, any suggestion where to put ignore_nonnull?
>>
>> Tom> Or, is it perhaps a better idea to have a gdb_assert_nonnull and
>> Tom> implement things there?
>>
> 
> Thanks for giving your take on this.
> 
>> How about we either drop the nonnull attribute
> 
> That's a possibility indeed.
> 
>> or we use
>> -fno-delete-null-pointer-checks?
>>
> 
> Unfortunately, that doesn't work (and it took me some hours today to
> realize and understand).  I've submitted a patch to improved the nonnull
> documentation (
> https://gcc.gnu.org/pipermail/gcc-patches/2021-July/576218.html ),
> hopefully it should be clear from there that
> -fno-delete-null-pointer-checks doesn't disable the optimization we're
> having trouble with.
> 
>> I personally feel that the gcc approach in this area is
>> counter-productive, at least for our purposes.  My view is that the
>> point of this stuff is to help us detect programming errors -- and we're
>> uninterested in using non-null-ness as some kind of optimization hint.
>> gcc seems to want it both ways, which seems bizarre.
> 
> I think they just implemented two attributes: assume_nonnull and
> verify_nonnull as a single attribute nonnull.  Then still that could be
> workable, but eventually the problem is that you can't switch the two
> interpretations on and off in a reasonable manner.
> 
>> But, given that
>> this is how the compiler works, IMO we should choose reliability
>> whichever way we best can.
> 
> Yes, I hope to get some reasonable feedback, but I'm not holding my
> breath.  So we could possibly end up with dropping nonnull.
> 

How about this patch (currently building) for master and gdb-11-branch,
to have a reliable solution right now for both master and gdb-11-branch,
and if and when the discussion at gcc-patches brings further insight, we
can improve master?

Thanks,
- Tom


[-- Attachment #2: 0001-gdb-build-Disable-attribute-nonnull.patch --]
[-- Type: text/x-patch, Size: 3434 bytes --]

[gdb/build] Disable attribute nonnull

With trunk gcc (12.0) we're running into a -Werror=nonnull-compare build
breaker in gdb, which caused a broader review of the usage of the nonnull
attribute.

The current conclusion is that it's best to disable this.  This is explained
at length in the gdbsupport/common-defs.h comment.

Tested by building with trunk gcc.

gdb/ChangeLog:

2021-07-29  Tom de Vries  <tdevries@suse.de>

	* gdbsupport/common-defs.h (ATTRIBUTE_NONNULL): Disable.

---
 gdbsupport/common-defs.h | 76 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/gdbsupport/common-defs.h b/gdbsupport/common-defs.h
index 5b644010cd9..3ced5371846 100644
--- a/gdbsupport/common-defs.h
+++ b/gdbsupport/common-defs.h
@@ -110,6 +110,82 @@
 #undef ATTRIBUTE_PRINTF
 #define ATTRIBUTE_PRINTF _GL_ATTRIBUTE_FORMAT_PRINTF_STANDARD
 
+/* This is defined by ansidecl.h, but we disable the attribute.
+
+   Say a developer starts out with:
+   ...
+   extern void foo (void *ptr) __atttribute__((nonnull (1)))
+   void foo (void *ptr) {
+   }
+   ...
+   with the idea in mind to catch:
+   ...
+   foo (nullptr);
+   ...
+   at compile time with -Werror=nonnull, and then adds:
+   ...
+    void foo (void *ptr) {
+   +  gdb_assert (ptr != nullptr);
+    }
+   ...
+   to catch:
+   ...
+   foo (variable_with_nullptr_value);
+   ...
+   at runtime as well.
+
+   Said developer then verifies that the assert works (using -O0), and commits
+   the code.
+
+   Some other developer then checks out the code and accidentally writes some
+   variant of:
+   ...
+   foo (variable_with_nullptr_value);
+   ...
+   and builds with -O2, and ... the assert doesn't trigger, because it's
+   optimized away by gcc.
+
+   There's no suppported recipe to prevent the assertion from being optimized
+   away (other than: build with -O0, or remove the nonnull attribute).  Note
+   that -fno-delete-null-pointer-checks does not help.  A patch was submitted
+   to improve gcc documentation to point his out more clearly (
+   https://gcc.gnu.org/pipermail/gcc-patches/2021-July/576218.html ).  The
+   patch also mentions a possible workaround that obfuscated the pointer
+   using:
+   ...
+    void foo (void *ptr) {
+   +  asm ("" : "+r"(ptr));
+      gdb_assert (ptr != nullptr);
+    }
+   ...
+   but that still requires the developer to manually add this in all cases
+   where that's necessary.
+
+   A warning was added to detect the situation: -Wnonnull-compare, which does
+   help in detecting those cases, but each new gcc release may indicate a new
+   batch of locations that needs fixing, which means we've added a maintenance
+   burden.
+
+   We could try to deal with the problem more proactively by introducing a
+   gdb_assert variant like:
+   ...
+   void gdb_assert_non_null (void *ptr) {
+      asm ("" : "+r"(ptr));
+      gdb_assert (ptr != nullptr);
+    }
+    void foo (void *ptr) {
+      gdb_assert_nonnull (ptr);
+    }
+   ...
+   and make it a coding style to use it everywhere, but again, maintenance
+   burden.
+
+   With all these things considered, for now we go with the solution with the
+   least maintenance burden: disable the attribute, such that we reliably deal
+   with it everywhere.  */
+#undef ATTRIBUTE_NONNULL
+#define ATTRIBUTE_NONNULL(m)
+
 #if GCC_VERSION >= 3004
 #define ATTRIBUTE_UNUSED_RESULT __attribute__ ((__warn_unused_result__))
 #else

Re: [master + 11][gdb/build] Disable attribute nonnull

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> The current conclusion is that it's best to disable this.  This is explained
Tom> at length in the gdbsupport/common-defs.h comment.

FWIW this looks reasonable to me.
I would recommend waiting a little to see if anyone else disagrees.

Normally I'd say let's simply not use ATTRIBUTE_NONNULL, but one thing I
like about your approach is that it provides a spot to put this great
comment you wrote.

Tom

Re: [master + 11][gdb/build] Disable attribute nonnull

[Andrew Burgess]

* Tom de Vries <tdevries@suse.de> [2021-07-29 13:42:15 +0200]:

> [ was: Re: [gdb/build] Fix Werror=nonnull-compare build breaker with gcc
> 12 ]
> 
> On 7/29/21 12:32 AM, Tom de Vries wrote:
> > [ quote-copied from
> > https://sourceware.org/pipermail/gdb-patches/2021-July/181173.html. I
> > didn't get this email in either my inbox or gdb-patches folder. ]
> > 
> >> Tom> I managed now to reproduce, and wrote a patch along these lines.
> >>
> >> Tom> Any comments?
> >>
> >> Tom> In particular, any suggestion where to put ignore_nonnull?
> >>
> >> Tom> Or, is it perhaps a better idea to have a gdb_assert_nonnull and
> >> Tom> implement things there?
> >>
> > 
> > Thanks for giving your take on this.
> > 
> >> How about we either drop the nonnull attribute
> > 
> > That's a possibility indeed.
> > 
> >> or we use
> >> -fno-delete-null-pointer-checks?
> >>
> > 
> > Unfortunately, that doesn't work (and it took me some hours today to
> > realize and understand).  I've submitted a patch to improved the nonnull
> > documentation (
> > https://gcc.gnu.org/pipermail/gcc-patches/2021-July/576218.html ),
> > hopefully it should be clear from there that
> > -fno-delete-null-pointer-checks doesn't disable the optimization we're
> > having trouble with.
> > 
> >> I personally feel that the gcc approach in this area is
> >> counter-productive, at least for our purposes.  My view is that the
> >> point of this stuff is to help us detect programming errors -- and we're
> >> uninterested in using non-null-ness as some kind of optimization hint.
> >> gcc seems to want it both ways, which seems bizarre.
> > 
> > I think they just implemented two attributes: assume_nonnull and
> > verify_nonnull as a single attribute nonnull.  Then still that could be
> > workable, but eventually the problem is that you can't switch the two
> > interpretations on and off in a reasonable manner.
> > 
> >> But, given that
> >> this is how the compiler works, IMO we should choose reliability
> >> whichever way we best can.
> > 
> > Yes, I hope to get some reasonable feedback, but I'm not holding my
> > breath.  So we could possibly end up with dropping nonnull.
> > 
> 
> How about this patch (currently building) for master and gdb-11-branch,
> to have a reliable solution right now for both master and gdb-11-branch,
> and if and when the discussion at gcc-patches brings further insight, we
> can improve master?
> 
> Thanks,
> - Tom
> 

> [gdb/build] Disable attribute nonnull
> 
> With trunk gcc (12.0) we're running into a -Werror=nonnull-compare build
> breaker in gdb, which caused a broader review of the usage of the nonnull
> attribute.
> 
> The current conclusion is that it's best to disable this.  This is explained
> at length in the gdbsupport/common-defs.h comment.
> 
> Tested by building with trunk gcc.
> 
> gdb/ChangeLog:
> 
> 2021-07-29  Tom de Vries  <tdevries@suse.de>
> 
> 	* gdbsupport/common-defs.h (ATTRIBUTE_NONNULL): Disable.
> 
> ---
>  gdbsupport/common-defs.h | 76 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/gdbsupport/common-defs.h b/gdbsupport/common-defs.h
> index 5b644010cd9..3ced5371846 100644
> --- a/gdbsupport/common-defs.h
> +++ b/gdbsupport/common-defs.h
> @@ -110,6 +110,82 @@
>  #undef ATTRIBUTE_PRINTF
>  #define ATTRIBUTE_PRINTF _GL_ATTRIBUTE_FORMAT_PRINTF_STANDARD
>  
> +/* This is defined by ansidecl.h, but we disable the attribute.
> +
> +   Say a developer starts out with:
> +   ...
> +   extern void foo (void *ptr) __atttribute__((nonnull (1)))
> +   void foo (void *ptr) {
> +   }
> +   ...
> +   with the idea in mind to catch:
> +   ...
> +   foo (nullptr);
> +   ...
> +   at compile time with -Werror=nonnull, and then adds:
> +   ...
> +    void foo (void *ptr) {
> +   +  gdb_assert (ptr != nullptr);
> +    }
> +   ...
> +   to catch:
> +   ...
> +   foo (variable_with_nullptr_value);
> +   ...
> +   at runtime as well.
> +
> +   Said developer then verifies that the assert works (using -O0), and commits
> +   the code.
> +
> +   Some other developer then checks out the code and accidentally writes some
> +   variant of:
> +   ...
> +   foo (variable_with_nullptr_value);
> +   ...
> +   and builds with -O2, and ... the assert doesn't trigger, because it's
> +   optimized away by gcc.
> +
> +   There's no suppported recipe to prevent the assertion from being optimized
> +   away (other than: build with -O0, or remove the nonnull attribute).  Note
> +   that -fno-delete-null-pointer-checks does not help.  A patch was submitted
> +   to improve gcc documentation to point his out more clearly (
> +   https://gcc.gnu.org/pipermail/gcc-patches/2021-July/576218.html ).  The
> +   patch also mentions a possible workaround that obfuscated the pointer
> +   using:
> +   ...
> +    void foo (void *ptr) {
> +   +  asm ("" : "+r"(ptr));
> +      gdb_assert (ptr != nullptr);
> +    }
> +   ...
> +   but that still requires the developer to manually add this in all cases
> +   where that's necessary.
> +
> +   A warning was added to detect the situation: -Wnonnull-compare, which does
> +   help in detecting those cases, but each new gcc release may indicate a new
> +   batch of locations that needs fixing, which means we've added a maintenance
> +   burden.
> +
> +   We could try to deal with the problem more proactively by introducing a
> +   gdb_assert variant like:
> +   ...
> +   void gdb_assert_non_null (void *ptr) {
> +      asm ("" : "+r"(ptr));
> +      gdb_assert (ptr != nullptr);
> +    }
> +    void foo (void *ptr) {
> +      gdb_assert_nonnull (ptr);
> +    }
> +   ...
> +   and make it a coding style to use it everywhere, but again, maintenance
> +   burden.
> +
> +   With all these things considered, for now we go with the solution with the
> +   least maintenance burden: disable the attribute, such that we reliably deal
> +   with it everywhere.  */
> +#undef ATTRIBUTE_NONNULL
> +#define ATTRIBUTE_NONNULL(m)

I have no problem with this patch going in.

I did wonder if something like this would be better:

  #define ATTRIBUTE_NONNULL(m)  __do_not_use_attribute_nonull__

We'd then have to remove all uses of ATTRIBUTE_NONNULL.   But maybe
this would cause problems if any other sub-projects (readline/gnulib?)
used ATTRIBUTE_NONNULL, we don't want to have to edit those source
files.  So your patch is probably preferred.

Thanks,
Andrew