Bug 79961 - Should diagnose when '__nonnull__' attribute is applied to implicit this argument
Summary: Should diagnose when '__nonnull__' attribute is applied to implicit this argu...
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 7.1.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on:
Blocks: Wnonnull
  Show dependency treegraph
 
Reported: 2017-03-08 15:42 UTC by Pedro Alves
Modified: 2020-06-03 22:49 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 10.1.0, 11.0, 7.3.0, 8.2.0, 9.1.0
Last reconfirmed: 2020-06-03 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Alves 2017-03-08 15:42:56 UTC
struct A
{
  A (const char *arg) __attribute__ ((__nonnull__ (1)));
  void foo (const char *arg) __attribute__ ((__nonnull__ (1)));
};

Above, nonnull(1) should have been nonnull(2), since argument 1 is the implicit "this" pointer (which is always non-NULL anyway).

Unfortunately, G++ fails to help find the bug.

With GNU C++14 (GCC) version 7.0.1 20170307 (experimental):

 $ /opt/gcc/bin/g++ nonnull.cc -o nonnull -g3 -O2 -c -Wall -Wextra
 (no warnings)

Note Clang 3.7:

 $ clang++ nonnull.cc -o nonnull -c
 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)));
                                              ^            ~
 2 errors generated.
Comment 1 Jonathan Wakely 2017-03-08 15:56:05 UTC
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.
Comment 2 Pedro Alves 2017-03-08 16:07:29 UTC
Yeah, I think it's too late, and it'd cause trouble with compatibility with clang.

Note that the documentation for the attribute(format) <https://gcc.gnu.org/onlinedocs/gcc-6.3.0/gcc/Common-Function-Attributes.html> 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.
Comment 3 Jonathan Wakely 2017-03-08 16:11:05 UTC
Yes, improving the docs would be a start. Issuing a warning like Clang does would be ideal.
Comment 4 Jakub Jelinek 2017-03-08 18:56:29 UTC
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).
Comment 5 Pedro Alves 2017-03-08 19:10:27 UTC
> 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.
Comment 6 Pedro Alves 2017-03-08 23:13:58 UTC
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.
Comment 7 Pedro Alves 2017-03-08 23:14:26 UTC
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.  :-)
Comment 8 Martin Sebor 2017-03-09 00:21:54 UTC
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);
          ^
Comment 9 Pedro Alves 2017-03-09 00:50:33 UTC
>  ((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.
Comment 10 Martin Sebor 2017-03-09 01:20:11 UTC
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, &b, &c);
}
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;
            ^
Comment 11 Pedro Alves 2017-03-09 02:01:58 UTC
> 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);
  ((A *) 0)->foo (1, 0);
  ((A *) 0)->foo (0, 0);
  a.foo (0, &a);
  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.
Comment 12 Pedro Alves 2017-03-09 02:19:06 UTC
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.
Comment 13 Martin Sebor 2020-06-03 20:28:41 UTC
No improvement in GCC 10 or 11 (trunk).
Comment 14 Pedro Alves 2020-06-03 22:48:44 UTC
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.
Comment 15 Pedro Alves 2020-06-03 22:49:40 UTC
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)));