* Building with recent GCC versions: gdbsupport/gdb_assert.h:35:4: error: 'nonnull' argument 'filename' compared to NULL [-Werror=nonnull-compare] @ 2021-07-26 21:11 Jan-Benedict Glaw 2021-07-27 10:03 ` 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 0 siblings, 2 replies; 9+ messages in thread From: Jan-Benedict Glaw @ 2021-07-26 21:11 UTC (permalink / raw) To: gdb [-- Attachment #1: Type: text/plain, Size: 3153 bytes --] 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? Thanks, Jan-Benedict -- [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Building with recent GCC versions: gdbsupport/gdb_assert.h:35:4: error: 'nonnull' argument 'filename' compared to NULL [-Werror=nonnull-compare] 2021-07-26 21:11 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-07-27 10:03 ` Andrew Burgess 2021-07-27 10:44 ` Tom de Vries 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 1 sibling, 1 reply; 9+ messages in thread From: Andrew Burgess @ 2021-07-27 10:03 UTC (permalink / raw) To: Jan-Benedict Glaw; +Cc: gdb * 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. 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? Thanks, Andrew --- 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)) #endif +/* 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> +bool _GL_ATTRIBUTE_NOINLINE +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 () ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Building with recent GCC versions: gdbsupport/gdb_assert.h:35:4: error: 'nonnull' argument 'filename' compared to NULL [-Werror=nonnull-compare] 2021-07-27 10:03 ` Andrew Burgess @ 2021-07-27 10:44 ` Tom de Vries 2021-07-27 11:35 ` Andrew Burgess 0 siblings, 1 reply; 9+ messages in thread From: Tom de Vries @ 2021-07-27 10:44 UTC (permalink / raw) To: Andrew Burgess, Jan-Benedict Glaw; +Cc: gdb 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. I wonder whether using volatile is a better idea (can't try this out right now). Thanks, - Tom ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Building with recent GCC versions: gdbsupport/gdb_assert.h:35:4: error: 'nonnull' argument 'filename' compared to NULL [-Werror=nonnull-compare] 2021-07-27 10:44 ` Tom de Vries @ 2021-07-27 11:35 ` Andrew Burgess 2021-07-27 11:49 ` Tom de Vries 0 siblings, 1 reply; 9+ messages in thread From: Andrew Burgess @ 2021-07-27 11:35 UTC (permalink / raw) To: Tom de Vries; +Cc: Jan-Benedict Glaw, gdb * 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. 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). > > Thanks, > - Tom ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Building with recent GCC versions: gdbsupport/gdb_assert.h:35:4: error: 'nonnull' argument 'filename' compared to NULL [-Werror=nonnull-compare] 2021-07-27 11:35 ` Andrew Burgess @ 2021-07-27 11:49 ` Tom de Vries 2021-07-27 13:38 ` Tom de Vries 0 siblings, 1 reply; 9+ messages in thread From: Tom de Vries @ 2021-07-27 11:49 UTC (permalink / raw) To: Andrew Burgess; +Cc: Jan-Benedict Glaw, gdb 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). >> >> Thanks, >> - Tom ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Building with recent GCC versions: gdbsupport/gdb_assert.h:35:4: error: 'nonnull' argument 'filename' compared to NULL [-Werror=nonnull-compare] 2021-07-27 11:49 ` Tom de Vries @ 2021-07-27 13:38 ` Tom de Vries 2021-07-28 8:37 ` Andrew Burgess 0 siblings, 1 reply; 9+ messages in thread From: Tom de Vries @ 2021-07-27 13:38 UTC (permalink / raw) To: Andrew Burgess; +Cc: Jan-Benedict Glaw, gdb 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. Thanks, - Tom ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Building with recent GCC versions: gdbsupport/gdb_assert.h:35:4: error: 'nonnull' argument 'filename' compared to NULL [-Werror=nonnull-compare] 2021-07-27 13:38 ` Tom de Vries @ 2021-07-28 8:37 ` Andrew Burgess 0 siblings, 0 replies; 9+ messages in thread From: Andrew Burgess @ 2021-07-28 8:37 UTC (permalink / raw) To: Tom de Vries; +Cc: Jan-Benedict Glaw, gdb * Tom de Vries <tdevries@suse.de> [2021-07-27 15:38:19 +0200]: > 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've been testing issues like this using: https://godbolt.org/z/nfhq6zb7q Your suggestion gives this error: error: 'volatile'-qualified return type is deprecated [-Werror=volatile] 21 | const T * volatile | ^~~~~ cc1plus: all warnings being treated as errors Compiler returned: 1 If we remove the volatile return type then of course GCC inlines and optimises out the assert. We could make the 'ignore_nonnull' noinline, but then we're basically back at my original suggestion. I suspect the only choice (right now) might be to do: template<typename T> void __attribute__ ((noinline)) assert_nonnull (const T *ptr) { asm (""); gdb_assert (ptr != nullptr); } Then replace gdb_assert with 'assert_nonnull (filename)'. What we'd actually want is for 'assert_nonnull' to be a macro that passes through the file/function/line just like the existing assert does so that the failed assert can be reported in the correct place. The more I look at this the more it feels like this is something GCC should be able to help us with... Thanks, Andrew ^ permalink raw reply [flat|nested] 9+ messages in thread
* CI Builds (was: Building with recent GCC versions: gdbsupport/gdb_assert.h:35:4: error: 'nonnull' argument 'filename' compared to NULL [-Werror=nonnull-compare]) 2021-07-26 21:11 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-07-27 10:03 ` Andrew Burgess @ 2021-07-31 21:18 ` Jan-Benedict Glaw 2021-08-01 23:35 ` CI Builds Sergio Durigan Junior 1 sibling, 1 reply; 9+ messages in thread From: Jan-Benedict Glaw @ 2021-07-31 21:18 UTC (permalink / raw) To: gdb [-- Attachment #1: Type: text/plain, Size: 1233 bytes --] Hi! On Mon, 2021-07-26 23:11:01 +0200, Jan-Benedict Glaw <jbglaw@lug-owl.de> wrote: > 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] [...] I just wanted to add a few thoughts. There's this machine running, doing test compiles for Binutils/GAS/GDB as well as GCC, Linux kernel, SIMH simulator and NetBSD (cross-compiled from Linux and from a NetBSD host running in KVM.) I'm usually building with gcc-snapshot (except Linux kernel, which cross-compiles from my last matching GCC build for that target, so it's nearly master.) When I detect issues like this (either individual targets or global breakages like this non-null attribution issue), shall I report them to the GDB list? Open a ticket? Most of the time, I think I'd be able to pin-point a causing commit. Thanks, Jan-Benedict PS: I've seen that there was a CI build thread these days, maybe I'd also comment on that? -- [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: CI Builds 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 ` Sergio Durigan Junior 0 siblings, 0 replies; 9+ messages in thread From: Sergio Durigan Junior @ 2021-08-01 23:35 UTC (permalink / raw) To: Jan-Benedict Glaw; +Cc: gdb [-- Attachment #1: Type: text/plain, Size: 1638 bytes --] On Saturday, July 31 2021, Jan-Benedict Glaw wrote: > Hi! > > On Mon, 2021-07-26 23:11:01 +0200, Jan-Benedict Glaw <jbglaw@lug-owl.de> wrote: >> 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] > > [...] > > I just wanted to add a few thoughts. > > There's this machine running, doing test compiles for > Binutils/GAS/GDB as well as GCC, Linux kernel, SIMH simulator and > NetBSD (cross-compiled from Linux and from a NetBSD host running in > KVM.) I'm usually building with gcc-snapshot (except Linux kernel, > which cross-compiles from my last matching GCC build for that target, > so it's nearly master.) > > When I detect issues like this (either individual targets or global > breakages like this non-null attribution issue), shall I report them > to the GDB list? Open a ticket? Most of the time, I think I'd be able > to pin-point a causing commit. If you're able to pinpoint a commit, then the best IMO would be to reply to the email thread where the patch was discussed (on gdb-patches@). This is what I was doing when I was reporting regressions on behalf of the GDB Buildbot. Not all developers pay attention to gdb@. Thanks, -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible https://sergiodj.net/ [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-08-01 23:35 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-26 21:11 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-07-27 10:03 ` Andrew Burgess 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
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).