[Bug c++/79961] Should diagnose when '__nonnull__' attribute is applied to implicit this argument

[palves at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79961

--- Comment #15 from Pedro Alves  ---
Darn typos.  Should be obvious that I meant:

Make it possible to write:

  void foo (int, const char *arg) __attribute__ ((__nonnull__ (arg)));

instead of this when foo is a class method:

  void foo (int, const char *arg) __attribute__ ((__nonnull__ (3)));

and this when it's a free function:

  void foo (int, const char *arg) __attribute__ ((__nonnull__ (2)));

[Bug c++/79961] Should diagnose when '__nonnull__' attribute is applied to implicit this argument

[palves at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79961

--- Comment #14 from Pedro Alves  ---
I had forgotten about this bug, and when I re-read it, the idea of letting
the user refer to the parameter by name crossed my mind.

Like, make it possible to write:

  void foo (int, const char *arg) __attribute__ ((__nonnull__ (arg)));

instead of this when foo is a class method:

  void foo (int, const char *arg) __attribute__ ((__nonnull__ (3)));

and this when it's a free function:

  void foo (int, const char *arg) __attribute__ ((__nonnull__ (3)));

Thus, removing the ambiguity, and probably making it easier for humans to
grok the intention, removing the need to count parameters.

Slightly off topic, but I thought I'd record it.

[Bug c++/79961] Should diagnose when '__nonnull__' attribute is applied to implicit this argument

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79961

Martin Sebor  changed:

   What|Removed |Added

Version|unknown |7.1.0
  Known to fail||10.1.0, 11.0, 7.3.0, 8.2.0,
   ||9.1.0
 Blocks||95507
   Last reconfirmed|2017-03-08 00:00:00 |2020-6-3

--- Comment #13 from Martin Sebor  ---
No improvement in GCC 10 or 11 (trunk).


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95507
[Bug 95507] [meta-bug] bogus/missing -Wnonnull

[Bug c++/79961] Should diagnose when '__nonnull__' attribute is applied to implicit this argument

[palves at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79961

--- Comment #12 from Pedro Alves  ---
TBC, the reason I filed this, is because GDB had an incorrect use like that
that survived for a few months:

 https://sourceware.org/ml/gdb-patches/2016-11/msg00933.html

until someone compiled GDB with clang:

 https://sourceware.org/bugzilla/show_bug.cgi?id=21206#c6

I can only imagine how many more incorrect uses are out there.

[Bug c++/79961] Should diagnose when '__nonnull__' attribute is applied to implicit this argument

[palves at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79961

--- Comment #11 from Pedro Alves  ---
> All the interesting calls here are undefined.

I meant that the one pointed out is undefined even without the nonnull
attribute.  I.e., it's not a use case that justifies supporting nonnull(1) on
non-members.  The others are only undefined due to the nonnull attribute.
Actually, I'd think that G++ should warn/error for that "((A*)0)->f();" case as
if the user that specified nonnull(1), even if the user hadn't.  I.e.,
implicitly add the nonnull(1) on all non-static methods.

> 
> The point of the example is to highlight that the nonnull attribute on the
> typedef
> 
>   typedef void F (void*) __attribute__ ((__nonnull__ (1)));
> 
> is interpreted differently depending on how the typedef is used.  While in
> most cases the number refers to the first void* argument, when it's used to
> declare a member function it refers to the implicit this argument.  If
> attribute((nonnull(1))) is to be rejected on non-static member function
> declarations as you suggest this seems like a case worth keeping in mind.

Yes, but that's not specific to the implicit this argument.  That's true for
_all_ arguments.  Change the prototype to:

  typedef void F (int, void*) __attribute__ ((__nonnull__ (2)));

and now when F is used as type of a class method, if the nonnull argument
number is reevaluated, nonnull(2) applies to the "int", which would be an
error, because that's not a pointer parameter.  Interestingly, seems like
neither G++ nor clang error in that case:

$ cat nonnull2.cc
typedef void F (int, void*) __attribute__ ((__nonnull__ (2)));

struct A
{
  F foo;
};

void foo ()
{
  A a;

  ((A *) 0)->foo (0, );
  ((A *) 0)->foo (1, 0);
  ((A *) 0)->foo (0, 0);
  a.foo (0, );
  a.foo (0, 0);
}
$ /opt/gcc/bin/g++ -O2 -c -Wall -Wextra -Wpedantic nonnull2.cc 
(nothing)
$ clang++ -O2 -c -Wall -Wextra -Wpedantic nonnull2.cc 
(nothing)

So I'm not sure what's the logic that dictates when and how are nonnull
argument numbers reevaluated in the method typedef case.  Looks like the
nonnull attribute is discarded above.  But in your case with nonnull(1), it
isn't.  ??!

Note that adding back the nonnull attribute like this:

struct A
{
- F foo;
+ F __attribute__ ((__nonnull__ (3))) foo;
};

makes both G++ and clang complain again as expected:

nonnull2.cc:13:23: warning: null argument where non-null required (argument 3)
[-Wnonnull]
   ((A *) 0)->foo (1, 0);
   ^
nonnull2.cc:14:23: warning: null argument where non-null required (argument 3)
[-Wnonnull]
   ((A *) 0)->foo (0, 0);
   ^
nonnull2.cc:16:14: warning: null argument where non-null required (argument 3)
[-Wnonnull]
   a.foo (0, 0);
  ^

> 
> The call to A::g in the test case is meant to demonstrate that simply
> printing "null argument where non-null required (argument 1)" in the
> diagnostic may not be sufficient to identify which argument is being
> referred to.  

I agree.

> The fact that the caret doesn't point at the argument only
> makes it that much more difficult (and underscores the importance of
> referring to the argument more clearly, e.g., by printing "the this pointer"
> instead of "argument 1" or something like that).

Definitely agreed.  For the other arguments, underlining the parameter in
question, as in clang's output shown in comment 7 makes it much simpler to
grok, IMO:

nonnull.cc:5:51: warning: '__nonnull__' attribute only applies to pointer
arguments [-Wignored-attributes]
  void foo (int, const char *arg) __attribute__ ((__nonnull__ (2)));
~~~

> Here's a slightly different example that should make that clear:

Understood and agreed.

[Bug c++/79961] Should diagnose when '__nonnull__' attribute is applied to implicit this argument

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79961

--- Comment #10 from Martin Sebor  ---
All the interesting calls here are undefined.

The point of the example is to highlight that the nonnull attribute on the
typedef

  typedef void F (void*) __attribute__ ((__nonnull__ (1)));

is interpreted differently depending on how the typedef is used.  While in most
cases the number refers to the first void* argument, when it's used to declare
a member function it refers to the implicit this argument.  If
attribute((nonnull(1))) is to be rejected on non-static member function
declarations as you suggest this seems like a case worth keeping in mind.

The call to A::g in the test case is meant to demonstrate that simply printing
"null argument where non-null required (argument 1)" in the diagnostic may not
be sufficient to identify which argument is being referred to.  The fact that
the caret doesn't point at the argument only makes it that much more difficult
(and underscores the importance of referring to the argument more clearly,
e.g., by printing "the this pointer" instead of "argument 1" or something like
that).

Here's a slightly different example that should make that clear:

$ cat t.C && gcc -O2 -S -Wall -Wextra -Wpedantic t.C
typedef void F (void*, void*) __attribute__ ((nonnull));

struct A {
  F f;
  static F g;
};

void foo (A *p, A *q, A *r)
{
  p->f (q, r);   // argument 1 is the implicit this here...
  r->g (p, r);   // ...but the first argument here
}

void bar ()
{
  A *a = 0, b, c;
  foo (a, , );
}
t.C: In function ‘void bar()’:
t.C:10:8: warning: argument 1 null where non-null expected [-Wnonnull]
   p->f (q, r);   // argument 1 is the implicit this here...
   ~^~
t.C:4:5: note: in a call to function ‘void A::f(void*, void*)’ declared here
   F f;
 ^
t.C:11:8: warning: argument 1 null where non-null expected [-Wnonnull]
   r->g (p, r);   // ...but the first argument here
   ~^~
t.C:5:12: note: in a call to function ‘static void A::g(void*, void*)’ declared
here
   static F g;
^

[Bug c++/79961] Should diagnose when '__nonnull__' attribute is applied to implicit this argument

[palves at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79961

--- Comment #9 from Pedro Alves  ---
>  ((A*)0)->g (p)

This is undefined behavior.  We forced the world to fix code like that in the
GCC 6 release cycle: https://gcc.gnu.org/gcc-6/changes.html

At best, I'd suggest degrading the error on implicit this nonnull(1) to a
warning iff compiling with -fno-delete-null-pointer-checks.  But I don't think
we really should need to cater to that.  FWIW.

[Bug c++/79961] Should diagnose when '__nonnull__' attribute is applied to implicit this argument

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79961

Martin Sebor  changed:

   What|Removed |Added

 CC||msebor at gcc dot gnu.org

--- Comment #8 from Martin Sebor  ---
Consider the following test case.  If the decision is to allow A::g, it would
be helpful if the warning on calls to it made it clear which argument it's
referring to: the implicit this or the same argument 1 as that in the calls to
the other two functions.

$ cat t.C && gcc -S -Wall -Wextra -Wpedantic t.C
typedef void F (void*) __attribute__ ((__nonnull__ (1)));

F f;

struct A
{
  F g;
  static F h;
};

void foo (void *p)
{
  f (0);

  ((A*)0)->g (p);

  A::h (0);
}
t.C: In function ‘void foo(void*)’:
t.C:13:7: warning: null argument where non-null required (argument 1)
[-Wnonnul]
   f (0);
   ^
t.C:15:16: warning: null argument where non-null required (argument 1)
[-Wnonnull]
   ((A*)0)->g (p);
^
t.C:17:10: warning: null argument where non-null required (argument 1)
[-Wnonnull]
   A::h (0);
  ^

[Bug c++/79961] Should diagnose when '__nonnull__' attribute is applied to implicit this argument

[palves at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79961

--- Comment #7 from Pedro Alves  ---
Funny enough, clang 3.7 (don't have more recent handy), warns in that case,
while it errors on the "this" arg:

nonnull.cc:3:39: error: '__nonnull__' attribute is invalid for the implicit
this argument
  A (const char *arg) __attribute__ ((__nonnull__ (1)));
  ^~
nonnull.cc:4:46: error: '__nonnull__' attribute is invalid for the implicit
this argument
  void foo (const char *arg) __attribute__ ((__nonnull__ (1)));
 ^~
nonnull.cc:5:51: warning: '__nonnull__' attribute only applies to pointer
arguments [-Wignored-attributes]
  void foo (int, const char *arg) __attribute__ ((__nonnull__ (2)));
~~~   ^~
1 warning and 2 errors generated.

So nobody's consistent.  :-)

[Bug c++/79961] Should diagnose when '__nonnull__' attribute is applied to implicit this argument

[palves at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79961

--- Comment #6 from Pedro Alves  ---
I remembered to check what does G++ say when you apply the nonnull to a
non-pointer argument.  We get a hard error:

$ /opt/gcc/bin/g++ nonnull.cc -o nonnull -c
nonnull.cc:5:67: error: nonnull argument references non-pointer operand
(argument 1, operand 2)
   void foo (int, const char *arg) __attribute__ ((__nonnull__ (2)));
   ^

I think this sets precedent for a hard error on the implicit "this" argument.

[Bug c++/79961] Should diagnose when '__nonnull__' attribute is applied to implicit this argument

[palves at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79961

--- Comment #5 from Pedro Alves  ---
> We certainly should allow __attribute__((nonnull)) on methods, even when that 
> > includes nonnull (implicit) also for this. 

Yes, agreed, with implicit nonnull with no specified argument.

For the case of specifying an argument number with nonnull(N), I don't find the
error weird at all, because "this" can never be NULL.  Even without the
attribute, the compiler is already free to assume that.  Calling a method via a
null pointer is undefined behavior.  So __attribute__((nonnull(1))) on a
non-static method is _always_ a bug.

[Bug c++/79961] Should diagnose when '__nonnull__' attribute is applied to implicit this argument

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79961

Jakub Jelinek  changed:

   What|Removed |Added

 CC||jakub at gcc dot gnu.org

--- Comment #4 from Jakub Jelinek  ---
We certainly should allow __attribute__((nonnull)) on methods, even when that
includes nonnull (implicit) also for this.  I find error on that weird, there
is nothing wrong specifying that this is non-null, so perhaps just warning (but
question is how to call it and under which of -Wall/-W or none of that enable
it).

[Bug c++/79961] Should diagnose when '__nonnull__' attribute is applied to implicit this argument

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79961

Jonathan Wakely  changed:

   What|Removed |Added

   Keywords||diagnostic
 Status|UNCONFIRMED |NEW
   Last reconfirmed||2017-03-08
 Ever confirmed|0   |1

--- Comment #3 from Jonathan Wakely  ---
Yes, improving the docs would be a start. Issuing a warning like Clang does
would be ideal.

[Bug c++/79961] Should diagnose when '__nonnull__' attribute is applied to implicit this argument

[palves at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79961

--- Comment #2 from Pedro Alves  ---
Yeah, I think it's too late, and it'd cause trouble with compatibility with
clang.

Note that the documentation for the attribute(format)

says:

"Since non-static C++ methods have an implicit this argument, the arguments of
such methods should be counted from two, not one, when giving values for
string-index and first-to-check."

but the documentation for the nonnull attribute doesn't say anything about it. 
Maybe that bit could be factored out somehow, or copied, or xrefed.

[Bug c++/79961] Should diagnose when '__nonnull__' attribute is applied to implicit this argument

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79961

--- Comment #1 from Jonathan Wakely  ---
IMHO it would be much, much better if the implicit 'this' wasn't counted and
nonnull(1) referred to 'arg' but maybe it's too late to change that.