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

[Tom de Vries <tdevries@suse.de>]

[-- 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