[PATCH 0/13] v2 warning control by group and location (PR 74765)

[Martin Sebor]

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