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

[Andrew Burgess <andrew.burgess@embecosm.com>]

* 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