| Message ID | 3a5ea83c-0d91-d123-f537-f8f1223df890@gmail.com |
|---|---|
| Headers | show |
| Series | v2 warning control by group and location (PR 74765) | expand |
Hi Martin! On Fri, 2021-06-04 15:27:04 -0600, Martin Sebor <msebor@gmail.com> wrote: > This is a revised patch series to add warning control by group and > location, updated based on feedback on the initial series. [...] My automated checking (in this case: Using Debian's "gcc-snapshot" package) indicates that between versions 1:20210527-1 and 1:20210630-1, building GDB breaks. Your patch is a likely candidate. It's a case where a method asks for a nonnull argument and later on checks for NULLness again. The build log is currently available at (http://wolf.lug-owl.de:8080/jobs/gdb-vax-linux/5), though obviously breaks for any target: configure --target=vax-linux --prefix=/tmp/gdb-vax-linux make all-gdb [...] [all 2021-07-16 19:19:25] CXX compile/compile.o [all 2021-07-16 19:19:30] In file included from ./../gdbsupport/common-defs.h:126, [all 2021-07-16 19:19:30] from ./defs.h:28, [all 2021-07-16 19:19:30] from compile/compile.c:20: [all 2021-07-16 19:19:30] ./../gdbsupport/gdb_unlinker.h: In constructor 'gdb::unlinker::unlinker(const char*)': [all 2021-07-16 19:19:30] ./../gdbsupport/gdb_assert.h:35:4: error: 'nonnull' argument 'filename' compared to NULL [-Werror=nonnull-compare] [all 2021-07-16 19:19:30] 35 | ((void) ((expr) ? 0 : \ [all 2021-07-16 19:19:30] | ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [all 2021-07-16 19:19:30] 36 | (gdb_assert_fail (#expr, __FILE__, __LINE__, FUNCTION_NAME), 0))) [all 2021-07-16 19:19:30] | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [all 2021-07-16 19:19:30] ./../gdbsupport/gdb_unlinker.h:38:5: note: in expansion of macro 'gdb_assert' [all 2021-07-16 19:19:30] 38 | gdb_assert (filename != NULL); [all 2021-07-16 19:19:30] | ^~~~~~~~~~ [all 2021-07-16 19:19:31] cc1plus: all warnings being treated as errors [all 2021-07-16 19:19:31] make[1]: *** [Makefile:1641: compile/compile.o] Error 1 [all 2021-07-16 19:19:31] make[1]: Leaving directory '/var/lib/laminar/run/gdb-vax-linux/5/binutils-gdb/gdb' [all 2021-07-16 19:19:31] make: *** [Makefile:11410: all-gdb] Error 2 Code is this: 31 class unlinker 32 { 33 public: 34 35 unlinker (const char *filename) ATTRIBUTE_NONNULL (2) 36 : m_filename (filename) 37 { 38 gdb_assert (filename != NULL); 39 } I'm quite undecided whether this is bad behavior of GCC or bad coding style in Binutils/GDB, or both. Thanks, Jan-Benedict --
On 7/17/21 2:36 PM, Jan-Benedict Glaw wrote: > Hi Martin! > > On Fri, 2021-06-04 15:27:04 -0600, Martin Sebor <msebor@gmail.com> wrote: >> This is a revised patch series to add warning control by group and >> location, updated based on feedback on the initial series. > [...] > > My automated checking (in this case: Using Debian's "gcc-snapshot" > package) indicates that between versions 1:20210527-1 and > 1:20210630-1, building GDB breaks. Your patch is a likely candidate. > It's a case where a method asks for a nonnull argument and later on > checks for NULLness again. The build log is currently available at > (http://wolf.lug-owl.de:8080/jobs/gdb-vax-linux/5), though obviously > breaks for any target: > > configure --target=vax-linux --prefix=/tmp/gdb-vax-linux > make all-gdb > > [...] > [all 2021-07-16 19:19:25] CXX compile/compile.o > [all 2021-07-16 19:19:30] In file included from ./../gdbsupport/common-defs.h:126, > [all 2021-07-16 19:19:30] from ./defs.h:28, > [all 2021-07-16 19:19:30] from compile/compile.c:20: > [all 2021-07-16 19:19:30] ./../gdbsupport/gdb_unlinker.h: In constructor 'gdb::unlinker::unlinker(const char*)': > [all 2021-07-16 19:19:30] ./../gdbsupport/gdb_assert.h:35:4: error: 'nonnull' argument 'filename' compared to NULL [-Werror=nonnull-compare] > [all 2021-07-16 19:19:30] 35 | ((void) ((expr) ? 0 : \ > [all 2021-07-16 19:19:30] | ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > [all 2021-07-16 19:19:30] 36 | (gdb_assert_fail (#expr, __FILE__, __LINE__, FUNCTION_NAME), 0))) > [all 2021-07-16 19:19:30] | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > [all 2021-07-16 19:19:30] ./../gdbsupport/gdb_unlinker.h:38:5: note: in expansion of macro 'gdb_assert' > [all 2021-07-16 19:19:30] 38 | gdb_assert (filename != NULL); > [all 2021-07-16 19:19:30] | ^~~~~~~~~~ > [all 2021-07-16 19:19:31] cc1plus: all warnings being treated as errors > [all 2021-07-16 19:19:31] make[1]: *** [Makefile:1641: compile/compile.o] Error 1 > [all 2021-07-16 19:19:31] make[1]: Leaving directory '/var/lib/laminar/run/gdb-vax-linux/5/binutils-gdb/gdb' > [all 2021-07-16 19:19:31] make: *** [Makefile:11410: all-gdb] Error 2 > > > Code is this: > > 31 class unlinker > 32 { > 33 public: > 34 > 35 unlinker (const char *filename) ATTRIBUTE_NONNULL (2) > 36 : m_filename (filename) > 37 { > 38 gdb_assert (filename != NULL); > 39 } > > I'm quite undecided whether this is bad behavior of GCC or bad coding > style in Binutils/GDB, or both. A warning should be expected in this case. Before the recent GCC change it was inadvertently suppressed in gdb_assert macros by its operand being enclosed in parentheses. Martin > > Thanks, > Jan-Benedict >
* Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> [2021-07-19 09:08:35 -0600]: > On 7/17/21 2:36 PM, Jan-Benedict Glaw wrote: > > Hi Martin! > > > > On Fri, 2021-06-04 15:27:04 -0600, Martin Sebor <msebor@gmail.com> wrote: > > > This is a revised patch series to add warning control by group and > > > location, updated based on feedback on the initial series. > > [...] > > > > My automated checking (in this case: Using Debian's "gcc-snapshot" > > package) indicates that between versions 1:20210527-1 and > > 1:20210630-1, building GDB breaks. Your patch is a likely candidate. > > It's a case where a method asks for a nonnull argument and later on > > checks for NULLness again. The build log is currently available at > > (http://wolf.lug-owl.de:8080/jobs/gdb-vax-linux/5), though obviously > > breaks for any target: > > > > configure --target=vax-linux --prefix=/tmp/gdb-vax-linux > > make all-gdb > > > > [...] > > [all 2021-07-16 19:19:25] CXX compile/compile.o > > [all 2021-07-16 19:19:30] In file included from ./../gdbsupport/common-defs.h:126, > > [all 2021-07-16 19:19:30] from ./defs.h:28, > > [all 2021-07-16 19:19:30] from compile/compile.c:20: > > [all 2021-07-16 19:19:30] ./../gdbsupport/gdb_unlinker.h: In constructor 'gdb::unlinker::unlinker(const char*)': > > [all 2021-07-16 19:19:30] ./../gdbsupport/gdb_assert.h:35:4: error: 'nonnull' argument 'filename' compared to NULL [-Werror=nonnull-compare] > > [all 2021-07-16 19:19:30] 35 | ((void) ((expr) ? 0 : \ > > [all 2021-07-16 19:19:30] | ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > [all 2021-07-16 19:19:30] 36 | (gdb_assert_fail (#expr, __FILE__, __LINE__, FUNCTION_NAME), 0))) > > [all 2021-07-16 19:19:30] | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > [all 2021-07-16 19:19:30] ./../gdbsupport/gdb_unlinker.h:38:5: note: in expansion of macro 'gdb_assert' > > [all 2021-07-16 19:19:30] 38 | gdb_assert (filename != NULL); > > [all 2021-07-16 19:19:30] | ^~~~~~~~~~ > > [all 2021-07-16 19:19:31] cc1plus: all warnings being treated as errors > > [all 2021-07-16 19:19:31] make[1]: *** [Makefile:1641: compile/compile.o] Error 1 > > [all 2021-07-16 19:19:31] make[1]: Leaving directory '/var/lib/laminar/run/gdb-vax-linux/5/binutils-gdb/gdb' > > [all 2021-07-16 19:19:31] make: *** [Makefile:11410: all-gdb] Error 2 > > > > > > Code is this: > > > > 31 class unlinker > > 32 { > > 33 public: > > 34 > > 35 unlinker (const char *filename) ATTRIBUTE_NONNULL (2) > > 36 : m_filename (filename) > > 37 { > > 38 gdb_assert (filename != NULL); > > 39 } > > > > I'm quite undecided whether this is bad behavior of GCC or bad coding > > style in Binutils/GDB, or both. > > A warning should be expected in this case. Before the recent GCC > change it was inadvertently suppressed in gdb_assert macros by its > operand being enclosed in parentheses. This issue was just posted to the GDB list, and I wanted to clarify my understanding a bit. I believe that (at least by default) adding the nonnull attribute allows GCC to assume (in the above case) that filename will not be NULL and generate code accordingly. Additionally, passing an explicit NULL (i.e. 'unlinker obj (NULL)') would cause a compile time error. But, there's nothing to actually stop a NULL being passed due to, say, a logic bug in the program. So, something like this would compile fine: extern const char *ptr; unlinker obj (ptr); And in a separate compilation unit: const char *ptr = NULL; Obviously, the run time behaviour of such a program would be undefined. Given the above then, it doesn't seem crazy to want to do something like the above, that is, add an assert to catch a logic bug in the program. Is there an approved mechanism through which I can tell GCC that I really do want to do a comparison to NULL, without any warning, and without the check being optimised out? Thanks, Andrew
On 7/28/21 5:14 AM, Andrew Burgess wrote: > * Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> [2021-07-19 09:08:35 -0600]: > >> On 7/17/21 2:36 PM, Jan-Benedict Glaw wrote: >>> Hi Martin! >>> >>> On Fri, 2021-06-04 15:27:04 -0600, Martin Sebor <msebor@gmail.com> wrote: >>>> This is a revised patch series to add warning control by group and >>>> location, updated based on feedback on the initial series. >>> [...] >>> >>> My automated checking (in this case: Using Debian's "gcc-snapshot" >>> package) indicates that between versions 1:20210527-1 and >>> 1:20210630-1, building GDB breaks. Your patch is a likely candidate. >>> It's a case where a method asks for a nonnull argument and later on >>> checks for NULLness again. The build log is currently available at >>> (http://wolf.lug-owl.de:8080/jobs/gdb-vax-linux/5), though obviously >>> breaks for any target: >>> >>> configure --target=vax-linux --prefix=/tmp/gdb-vax-linux >>> make all-gdb >>> >>> [...] >>> [all 2021-07-16 19:19:25] CXX compile/compile.o >>> [all 2021-07-16 19:19:30] In file included from ./../gdbsupport/common-defs.h:126, >>> [all 2021-07-16 19:19:30] from ./defs.h:28, >>> [all 2021-07-16 19:19:30] from compile/compile.c:20: >>> [all 2021-07-16 19:19:30] ./../gdbsupport/gdb_unlinker.h: In constructor 'gdb::unlinker::unlinker(const char*)': >>> [all 2021-07-16 19:19:30] ./../gdbsupport/gdb_assert.h:35:4: error: 'nonnull' argument 'filename' compared to NULL [-Werror=nonnull-compare] >>> [all 2021-07-16 19:19:30] 35 | ((void) ((expr) ? 0 : \ >>> [all 2021-07-16 19:19:30] | ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> [all 2021-07-16 19:19:30] 36 | (gdb_assert_fail (#expr, __FILE__, __LINE__, FUNCTION_NAME), 0))) >>> [all 2021-07-16 19:19:30] | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> [all 2021-07-16 19:19:30] ./../gdbsupport/gdb_unlinker.h:38:5: note: in expansion of macro 'gdb_assert' >>> [all 2021-07-16 19:19:30] 38 | gdb_assert (filename != NULL); >>> [all 2021-07-16 19:19:30] | ^~~~~~~~~~ >>> [all 2021-07-16 19:19:31] cc1plus: all warnings being treated as errors >>> [all 2021-07-16 19:19:31] make[1]: *** [Makefile:1641: compile/compile.o] Error 1 >>> [all 2021-07-16 19:19:31] make[1]: Leaving directory '/var/lib/laminar/run/gdb-vax-linux/5/binutils-gdb/gdb' >>> [all 2021-07-16 19:19:31] make: *** [Makefile:11410: all-gdb] Error 2 >>> >>> >>> Code is this: >>> >>> 31 class unlinker >>> 32 { >>> 33 public: >>> 34 >>> 35 unlinker (const char *filename) ATTRIBUTE_NONNULL (2) >>> 36 : m_filename (filename) >>> 37 { >>> 38 gdb_assert (filename != NULL); >>> 39 } >>> >>> I'm quite undecided whether this is bad behavior of GCC or bad coding >>> style in Binutils/GDB, or both. >> >> A warning should be expected in this case. Before the recent GCC >> change it was inadvertently suppressed in gdb_assert macros by its >> operand being enclosed in parentheses. > > This issue was just posted to the GDB list, and I wanted to clarify my > understanding a bit. > > I believe that (at least by default) adding the nonnull attribute > allows GCC to assume (in the above case) that filename will not be > NULL and generate code accordingly. > > Additionally, passing an explicit NULL (i.e. 'unlinker obj (NULL)') > would cause a compile time error. > > But, there's nothing to actually stop a NULL being passed due to, say, > a logic bug in the program. So, something like this would compile > fine: > > extern const char *ptr; > unlinker obj (ptr); > > And in a separate compilation unit: > > const char *ptr = NULL; > > Obviously, the run time behaviour of such a program would be > undefined. > > Given the above then, it doesn't seem crazy to want to do something > like the above, that is, add an assert to catch a logic bug in the > program. > > Is there an approved mechanism through which I can tell GCC that I > really do want to do a comparison to NULL, without any warning, and > without the check being optimised out? The manual says -fno-delete-null-pointer-checks is supposed to prevent the removal of the null function argument test so I'd expect adding attribute optimize ("no-delete-null-pointer-checks") to the definition of the function to have that effect but in my testing it didn't work (and didn't give a warning for the two attributes on the same declarataion). That seems worth filing a bug for. An alternate approach that does work is to remove the nonnull attribute from the definition while leaving it on the declaration. But the two have to be compiled separately for it to work, which can be hard to do, especially with LTO. The only other way I can think of is by playing tricks with volatile. Even that only seems to work in non-obvious ways such as: __attribute__ ((nonnull)) void f (int *p) { if (!p) abort (); // eliminated volatile int *q = p; if (!q) abort (); // eliminated volatile int i = 0; if (!&p[i]) abort (); // works } Martin
* Martin Sebor <msebor@gmail.com> [2021-07-28 10:16:59 -0600]: > On 7/28/21 5:14 AM, Andrew Burgess wrote: > > * Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> [2021-07-19 09:08:35 -0600]: > > > > > On 7/17/21 2:36 PM, Jan-Benedict Glaw wrote: > > > > Hi Martin! > > > > > > > > On Fri, 2021-06-04 15:27:04 -0600, Martin Sebor <msebor@gmail.com> wrote: > > > > > This is a revised patch series to add warning control by group and > > > > > location, updated based on feedback on the initial series. > > > > [...] > > > > > > > > My automated checking (in this case: Using Debian's "gcc-snapshot" > > > > package) indicates that between versions 1:20210527-1 and > > > > 1:20210630-1, building GDB breaks. Your patch is a likely candidate. > > > > It's a case where a method asks for a nonnull argument and later on > > > > checks for NULLness again. The build log is currently available at > > > > (http://wolf.lug-owl.de:8080/jobs/gdb-vax-linux/5), though obviously > > > > breaks for any target: > > > > > > > > configure --target=vax-linux --prefix=/tmp/gdb-vax-linux > > > > make all-gdb > > > > > > > > [...] > > > > [all 2021-07-16 19:19:25] CXX compile/compile.o > > > > [all 2021-07-16 19:19:30] In file included from ./../gdbsupport/common-defs.h:126, > > > > [all 2021-07-16 19:19:30] from ./defs.h:28, > > > > [all 2021-07-16 19:19:30] from compile/compile.c:20: > > > > [all 2021-07-16 19:19:30] ./../gdbsupport/gdb_unlinker.h: In constructor 'gdb::unlinker::unlinker(const char*)': > > > > [all 2021-07-16 19:19:30] ./../gdbsupport/gdb_assert.h:35:4: error: 'nonnull' argument 'filename' compared to NULL [-Werror=nonnull-compare] > > > > [all 2021-07-16 19:19:30] 35 | ((void) ((expr) ? 0 : \ > > > > [all 2021-07-16 19:19:30] | ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > [all 2021-07-16 19:19:30] 36 | (gdb_assert_fail (#expr, __FILE__, __LINE__, FUNCTION_NAME), 0))) > > > > [all 2021-07-16 19:19:30] | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > [all 2021-07-16 19:19:30] ./../gdbsupport/gdb_unlinker.h:38:5: note: in expansion of macro 'gdb_assert' > > > > [all 2021-07-16 19:19:30] 38 | gdb_assert (filename != NULL); > > > > [all 2021-07-16 19:19:30] | ^~~~~~~~~~ > > > > [all 2021-07-16 19:19:31] cc1plus: all warnings being treated as errors > > > > [all 2021-07-16 19:19:31] make[1]: *** [Makefile:1641: compile/compile.o] Error 1 > > > > [all 2021-07-16 19:19:31] make[1]: Leaving directory '/var/lib/laminar/run/gdb-vax-linux/5/binutils-gdb/gdb' > > > > [all 2021-07-16 19:19:31] make: *** [Makefile:11410: all-gdb] Error 2 > > > > > > > > > > > > Code is this: > > > > > > > > 31 class unlinker > > > > 32 { > > > > 33 public: > > > > 34 > > > > 35 unlinker (const char *filename) ATTRIBUTE_NONNULL (2) > > > > 36 : m_filename (filename) > > > > 37 { > > > > 38 gdb_assert (filename != NULL); > > > > 39 } > > > > > > > > I'm quite undecided whether this is bad behavior of GCC or bad coding > > > > style in Binutils/GDB, or both. > > > > > > A warning should be expected in this case. Before the recent GCC > > > change it was inadvertently suppressed in gdb_assert macros by its > > > operand being enclosed in parentheses. > > > > This issue was just posted to the GDB list, and I wanted to clarify my > > understanding a bit. > > > > I believe that (at least by default) adding the nonnull attribute > > allows GCC to assume (in the above case) that filename will not be > > NULL and generate code accordingly. > > > > Additionally, passing an explicit NULL (i.e. 'unlinker obj (NULL)') > > would cause a compile time error. > > > > But, there's nothing to actually stop a NULL being passed due to, say, > > a logic bug in the program. So, something like this would compile > > fine: > > > > extern const char *ptr; > > unlinker obj (ptr); > > > > And in a separate compilation unit: > > > > const char *ptr = NULL; > > > > Obviously, the run time behaviour of such a program would be > > undefined. > > > > Given the above then, it doesn't seem crazy to want to do something > > like the above, that is, add an assert to catch a logic bug in the > > program. > > > > Is there an approved mechanism through which I can tell GCC that I > > really do want to do a comparison to NULL, without any warning, and > > without the check being optimised out? > Thanks for your feedback. > The manual says -fno-delete-null-pointer-checks is supposed to > prevent the removal of the null function argument test so I'd > expect adding attribute optimize ("no-delete-null-pointer-checks") > to the definition of the function to have that effect but in my > testing it didn't work (and didn't give a warning for the two > attributes on the same declarataion). That seems worth filing > a bug for. I've since been pointed at this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100404 Comment #3 of which discusses exactly this issue. Thanks, Andrew
On 7/29/21 2:26 AM, Andrew Burgess wrote: > * Martin Sebor <msebor@gmail.com> [2021-07-28 10:16:59 -0600]: > >> On 7/28/21 5:14 AM, Andrew Burgess wrote: >>> * Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> [2021-07-19 09:08:35 -0600]: >>> >>>> On 7/17/21 2:36 PM, Jan-Benedict Glaw wrote: >>>>> Hi Martin! >>>>> >>>>> On Fri, 2021-06-04 15:27:04 -0600, Martin Sebor <msebor@gmail.com> wrote: >>>>>> This is a revised patch series to add warning control by group and >>>>>> location, updated based on feedback on the initial series. >>>>> [...] >>>>> >>>>> My automated checking (in this case: Using Debian's "gcc-snapshot" >>>>> package) indicates that between versions 1:20210527-1 and >>>>> 1:20210630-1, building GDB breaks. Your patch is a likely candidate. >>>>> It's a case where a method asks for a nonnull argument and later on >>>>> checks for NULLness again. The build log is currently available at >>>>> (http://wolf.lug-owl.de:8080/jobs/gdb-vax-linux/5), though obviously >>>>> breaks for any target: >>>>> >>>>> configure --target=vax-linux --prefix=/tmp/gdb-vax-linux >>>>> make all-gdb >>>>> >>>>> [...] >>>>> [all 2021-07-16 19:19:25] CXX compile/compile.o >>>>> [all 2021-07-16 19:19:30] In file included from ./../gdbsupport/common-defs.h:126, >>>>> [all 2021-07-16 19:19:30] from ./defs.h:28, >>>>> [all 2021-07-16 19:19:30] from compile/compile.c:20: >>>>> [all 2021-07-16 19:19:30] ./../gdbsupport/gdb_unlinker.h: In constructor 'gdb::unlinker::unlinker(const char*)': >>>>> [all 2021-07-16 19:19:30] ./../gdbsupport/gdb_assert.h:35:4: error: 'nonnull' argument 'filename' compared to NULL [-Werror=nonnull-compare] >>>>> [all 2021-07-16 19:19:30] 35 | ((void) ((expr) ? 0 : \ >>>>> [all 2021-07-16 19:19:30] | ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>> [all 2021-07-16 19:19:30] 36 | (gdb_assert_fail (#expr, __FILE__, __LINE__, FUNCTION_NAME), 0))) >>>>> [all 2021-07-16 19:19:30] | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>> [all 2021-07-16 19:19:30] ./../gdbsupport/gdb_unlinker.h:38:5: note: in expansion of macro 'gdb_assert' >>>>> [all 2021-07-16 19:19:30] 38 | gdb_assert (filename != NULL); >>>>> [all 2021-07-16 19:19:30] | ^~~~~~~~~~ >>>>> [all 2021-07-16 19:19:31] cc1plus: all warnings being treated as errors >>>>> [all 2021-07-16 19:19:31] make[1]: *** [Makefile:1641: compile/compile.o] Error 1 >>>>> [all 2021-07-16 19:19:31] make[1]: Leaving directory '/var/lib/laminar/run/gdb-vax-linux/5/binutils-gdb/gdb' >>>>> [all 2021-07-16 19:19:31] make: *** [Makefile:11410: all-gdb] Error 2 >>>>> >>>>> >>>>> Code is this: >>>>> >>>>> 31 class unlinker >>>>> 32 { >>>>> 33 public: >>>>> 34 >>>>> 35 unlinker (const char *filename) ATTRIBUTE_NONNULL (2) >>>>> 36 : m_filename (filename) >>>>> 37 { >>>>> 38 gdb_assert (filename != NULL); >>>>> 39 } >>>>> >>>>> I'm quite undecided whether this is bad behavior of GCC or bad coding >>>>> style in Binutils/GDB, or both. >>>> >>>> A warning should be expected in this case. Before the recent GCC >>>> change it was inadvertently suppressed in gdb_assert macros by its >>>> operand being enclosed in parentheses. >>> >>> This issue was just posted to the GDB list, and I wanted to clarify my >>> understanding a bit. >>> >>> I believe that (at least by default) adding the nonnull attribute >>> allows GCC to assume (in the above case) that filename will not be >>> NULL and generate code accordingly. >>> >>> Additionally, passing an explicit NULL (i.e. 'unlinker obj (NULL)') >>> would cause a compile time error. >>> >>> But, there's nothing to actually stop a NULL being passed due to, say, >>> a logic bug in the program. So, something like this would compile >>> fine: >>> >>> extern const char *ptr; >>> unlinker obj (ptr); >>> >>> And in a separate compilation unit: >>> >>> const char *ptr = NULL; >>> >>> Obviously, the run time behaviour of such a program would be >>> undefined. >>> >>> Given the above then, it doesn't seem crazy to want to do something >>> like the above, that is, add an assert to catch a logic bug in the >>> program. >>> >>> Is there an approved mechanism through which I can tell GCC that I >>> really do want to do a comparison to NULL, without any warning, and >>> without the check being optimised out? >> > > Thanks for your feedback. > >> The manual says -fno-delete-null-pointer-checks is supposed to >> prevent the removal of the null function argument test so I'd >> expect adding attribute optimize ("no-delete-null-pointer-checks") >> to the definition of the function to have that effect but in my >> testing it didn't work (and didn't give a warning for the two >> attributes on the same declarataion). That seems worth filing >> a bug for. > > I've since been pointed at this: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100404 > > Comment #3 of which discusses exactly this issue. I had forgotten about that discussion and opened PR 101665. Thanks for the pointer! Let me link the two and see about updating the manual and maybe also issuing a warning when both attributes are set on the same function. Martin
This is a revised patch series to add warning control by group and location, updated based on feedback on the initial series. v2 changes include: 1) Use opt_code rather than int for the option argument to the new APIs. This let me find and fix a bug in the original Ada change. 2) Use suppress_warning() and warning_suppressed_p() instead of get/set_no_warning, and also instead of warning_enabled/disabled for the names of the new functions (as requested/clarified offline by David). 3) Make the removal of the TREE_NO_WARNING macro and the gimple_get_no_warning_p() and gimple_set_no_warning() functions a standalone patch. 4) Include tests for PR 74765 and 74762 fixed by these changes. I have retested the whole patch series on x86_64-linux.