Warn when comparing nonnull arguments to NULL in a function.

The following found 14 bugs in my code base. I think it is useful to
warn about such usage since they are bugsr. If the argument is marked
as nonnull then passing in a NULL argument will produce bad results
even if the code checks against NULL.

GCC might optimize such checks away so warn the user when the function
contains such comparisions.

nn.c: In function ‘foo’:
nn.c:6:27: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
 void foo(void *bar) { if (!bar) abort(); }
                           ^
gcc/c/ChangeLog

       * c-typeck.c (build_binary_op): Check and warn when nonnull arg
       parm against NULL.

gcc/cp/ChangeLog

       * typeck.c (cp_build_binary_op): Check and warn when nonnull arg
       parm against NULL.

gcc/testsuite/ChangeLog

       * gcc.dg/nonnull-4.c: New test.
       * g++.dg/warn/nonnull3.C: Likewise.
---
 gcc/c/ChangeLog                      |  5 +++++
 gcc/c/c-typeck.c                     | 10 ++++++++++
 gcc/cp/ChangeLog                     |  5 +++++
 gcc/cp/typeck.c                      | 10 ++++++++++
 gcc/testsuite/ChangeLog              |  5 +++++
 gcc/testsuite/g++.dg/warn/nonnull3.C | 29 +++++++++++++++++++++++++++++
 gcc/testsuite/gcc.dg/nonnull-4.c     | 28 ++++++++++++++++++++++++++++
 7 files changed, 92 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/warn/nonnull3.C
 create mode 100644 gcc/testsuite/gcc.dg/nonnull-4.c

On 09/09/2015 03:44 PM, Mark Wielaard wrote:
> The following found 14 bugs in my code base. I think it is useful to
> warn about such usage since they are bugsr. If the argument is marked
> as nonnull then passing in a NULL argument will produce bad results
> even if the code checks against NULL.
>
> GCC might optimize such checks away so warn the user when the function
> contains such comparisions.
>
> nn.c: In function ‘foo’:
> nn.c:6:27: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
>   void foo(void *bar) { if (!bar) abort(); }
>                             ^
> gcc/c/ChangeLog
>
>         * c-typeck.c (build_binary_op): Check and warn when nonnull arg
>         parm against NULL.
>
> gcc/cp/ChangeLog
>
>         * typeck.c (cp_build_binary_op): Check and warn when nonnull arg
>         parm against NULL.
>
> gcc/testsuite/ChangeLog
>
>         * gcc.dg/nonnull-4.c: New test.
>         * g++.dg/warn/nonnull3.C: Likewise.
Can you also upate the -Wnonnull documentation in invoke.texi to 
indicate it also will warn if it discovers a non-null argument that is 
compared against null?

With the doc fix and a bootstrap/regression test, this patch ought to be 
fine.

jeff

On Wed, Sep 09, 2015 at 04:01:07PM -0600, Jeff Law wrote:
> >        * gcc.dg/nonnull-4.c: New test.
> >        * g++.dg/warn/nonnull3.C: Likewise.

If the tests are the same, perhaps stick just one test into
c-c++-common/nonnull-1.c instead?  Also, all the "cp1 compared to NULL"
strings mention cp1, did you mean the second one to mention cp2 and so on?

> Can you also upate the -Wnonnull documentation in invoke.texi to indicate it
> also will warn if it discovers a non-null argument that is compared against
> null?
> 
> With the doc fix and a bootstrap/regression test, this patch ought to be
> fine.

	Jakub

> +void foo(void *bar) __attribute__((nonnull(1)));
> +
> +void foo(void *bar) { if (!bar) abort(); } /* { dg-warning "null" "argument ‘bar’ compared to NULL" } */

This looks like a very useful enhancement. Since the change is limited
to build_binary_op in the two front ends I wonder if the warning also
issued for other expressions? For example, suppose I were to add to
function foo above the following:

      bool is_null = bar;

would GCC issue a warning? The same question goes for other expressions
non-binary expressions, including:

      bar ? f () : g ();

or in C++:

      bool x = static_cast<bool>(bar);

If not, I would think issuing it would make the feature even more
useful (and the diagnostics more consistent).

Martin

On Mon, 2015-09-14 at 21:37 -0600, Martin Sebor wrote:
> > +void foo(void *bar) __attribute__((nonnull(1)));
> > +
> > +void foo(void *bar) { if (!bar) abort(); } /* { dg-warning "null" "argument ‘bar’ compared to NULL" } */
> 
> This looks like a very useful enhancement. Since the change is limited
> to build_binary_op in the two front ends I wonder if the warning also
> issued for other expressions? For example, suppose I were to add to
> function foo above the following:
> 
>       bool is_null = bar;
> 
> would GCC issue a warning? The same question goes for other expressions
> non-binary expressions, including:
> 
>       bar ? f () : g ();

Yes, it will:

nt.c: In function ‘tf’:
nt.c:9:3: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
   bool is_null = bar;
   ^

nt.c:14:7: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
   bar ? f () : g ();
       ^

> or in C++:
> 
>       bool x = static_cast<bool>(bar);

Likewise for the g++ frontend:

nt.c: In function ‘bool tf(void*)’:
nt.c:12:18: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
   bool is_null = bar;
                  ^
nt.c:14:19: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
   bar ? f () : g ();
                   ^
nt.c:16:33: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
   bool x = static_cast<bool>(bar);
                                 ^

Although I now notice they differ on the placement of the carrot.
Maybe the location passed into the warning is not correct/ideal?

Cheers,

Mark

On 15/09/15 10:32, Mark Wielaard wrote:
> On Mon, 2015-09-14 at 21:37 -0600, Martin Sebor wrote:
> Although I now notice they differ on the placement of the carrot.
> Maybe the location passed into the warning is not correct/ideal?

The caret is placed at the location given by expand_location(loc), with loc 
being the location passed to warning_at(). As far as I am aware, there are no 
bugs on that. If the caret is wrong, it is definitely because the location 
passed is the wrong one. You need to find the correct one (which may appear up 
in the call stack and may need to be passed down) and pass that one instead.

You can test the location with { dg-warning "18:nonnull argument" } where 18 is 
the column number expected. I wish it was used more frequently, in particular 
in those cases where we have the perfect location and it would be pity to regress.

Cheers,

Manuel.

On 09/15/2015 02:32 AM, Mark Wielaard wrote:
> On Mon, 2015-09-14 at 21:37 -0600, Martin Sebor wrote:
>>> +void foo(void *bar) __attribute__((nonnull(1)));
>>> +
>>> +void foo(void *bar) { if (!bar) abort(); } /* { dg-warning "null" "argument ‘bar’ compared to NULL" } */
>>
>> This looks like a very useful enhancement. Since the change is limited
>> to build_binary_op in the two front ends I wonder if the warning also
>> issued for other expressions? For example, suppose I were to add to
>> function foo above the following:
>>
>>        bool is_null = bar;
>>
>> would GCC issue a warning? The same question goes for other expressions
>> non-binary expressions, including:
>>
>>        bar ? f () : g ();
>
> Yes, it will:
>
> nt.c: In function ‘tf’:
> nt.c:9:3: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
>     bool is_null = bar;
>     ^
>
> nt.c:14:7: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
>     bar ? f () : g ();
>         ^
>
>> or in C++:
>>
>>        bool x = static_cast<bool>(bar);
>
> Likewise for the g++ frontend:

Great!

>
> nt.c: In function ‘bool tf(void*)’:
> nt.c:12:18: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
>     bool is_null = bar;
>                    ^
> nt.c:14:19: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
>     bar ? f () : g ();
>                     ^

IME, the C++ diagnostics aren't entirely consistent in where they
put the location but this one seems worse than I would expect. It
should point at bar.

OTOH, here are a couple of examples of the inconsistency, one with
the same problem as yours, so it's probably not something you did:

   $ cat t.c && g++ -c t.c
   int bar (void *p) {
       return p < 0.0 ? 0 : 1;
       struct A { } a;
       return a ? 0 : 1;
   }
   t.c: In function ‘int bar(void*)’:
   t.c:2:16: error: invalid operands of types ‘void*’ and ‘double’ to 
binary ‘operator<’
        return p < 0.0 ? 0 : 1;
                   ^
   t.c:4:20: error: could not convert ‘a’ from ‘bar(void*)::A’ to ‘bool’
        return a ? 0 : 1;
                       ^

> nt.c:16:33: warning: nonnull argument ‘bar’ compared to NULL [-Wnonnull]
>     bool x = static_cast<bool>(bar);
>                                   ^
>
> Although I now notice they differ on the placement of the carrot.
> Maybe the location passed into the warning is not correct/ideal?

I noticed the same problem in other g++ diagnostics. For instance,
in the messages below, the column is that of the closing parenthesis
rather than that of the operand:

   $ cat t.c && g++ -c t.c
   int foo (int, int);

   int bar (void *p) {
       foo (p, 0);
       return static_cast<int>(p);
   }
   t.c: In function ‘int bar(void*)’:
   t.c:4:14: error: invalid conversion from ‘void*’ to ‘int’ [-fpermissive]
        foo (p, 0);
                 ^
   t.c:1:5: note:   initializing argument 1 of ‘int foo(int, int)’
    int foo (int, int);
        ^
   t.c:5:30: error: invalid static_cast from type ‘void*’ to type ‘int’
        return static_cast<int>(p);
                                 ^

Martin

On Tue, 2015-09-15 at 14:21 +0200, Manuel López-Ibáñez wrote:
> On 15/09/15 10:32, Mark Wielaard wrote:
> > On Mon, 2015-09-14 at 21:37 -0600, Martin Sebor wrote:
> > Although I now notice they differ on the placement of the carrot.
> > Maybe the location passed into the warning is not correct/ideal?
> 
> The caret is placed at the location given by expand_location(loc), with loc 
> being the location passed to warning_at(). As far as I am aware, there are no 
> bugs on that. If the caret is wrong, it is definitely because the location 
> passed is the wrong one. You need to find the correct one (which may appear up 
> in the call stack and may need to be passed down) and pass that one instead.
> 
> You can test the location with { dg-warning "18:nonnull argument" } where 18 is 
> the column number expected. I wish it was used more frequently, in particular 
> in those cases where we have the perfect location and it would be pity to regress.

As Marin pointed out, this is not a new issue. Since build_binary_op
effectively only has one (input) location (and possibly the
DECL_SOURCE_LOCATIONS of the expressions, which are irrelevant in this
case) it is hard to get the location for the diagnostic precise. We
really need the locations of the expressions, not just the input
location of the binary op (which already seems a little inconsistent).
As you say we would have to audit each call to build_binary_op and
somehow pass through the location of op1 and op2 (if we actually have
them in the first place). Given the large number of ways build_binary_op
is called this doesn't seem feasible.

The real solution is probably something like David's "Add source-ranges
for trees" https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00739.html
David mentions several alternatives. I am not sure which one would be
the most acceptable.

Cheers,

Mark