[google] Check if the nonnull attribute is applied to 'this' (issue4446070)

[google] Check if the nonnull attribute is applied to 'this' (issue4446070)

[Diego Novillo]

This patch from Le-Chun Wu adds support to check whether a nonnull
attribute is applied to 'this' pointer for non-static methods.

OK for trunk?  Applied to google/main

2011-04-27  Le-Chun Wu  <lcwu@google.com>

	Google ref 45339.

	* c-common.c (handle_nonnull_attribute): Check whether the nonnull
	attribute is applied to the 'this' pointer for non-static methods.

testsuite/ChangeLog.google-main
2011-04-27  Le-Chun Wu  <lcwu@google.com>

	* g++.dg/warn/nonnull2.C: New.

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index c6dc649..a1702f8 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -7434,7 +7434,7 @@ handle_nonnull_attribute (tree *node, tree ARG_UNUSED (name),
 
   /* Argument list specified.  Verify that each argument number references
      a pointer argument.  */
-  for (attr_arg_num = 1; args; args = TREE_CHAIN (args))
+  for (attr_arg_num = 1; args; args = TREE_CHAIN (args), attr_arg_num++)
     {
       tree argument;
       unsigned HOST_WIDE_INT arg_num = 0, ck_num;
@@ -7466,6 +7466,7 @@ handle_nonnull_attribute (tree *node, tree ARG_UNUSED (name),
 	      return NULL_TREE;
 	    }
 
+
 	  if (TREE_CODE (TREE_VALUE (argument)) != POINTER_TYPE)
 	    {
 	      error ("nonnull argument references non-pointer operand (argument %lu, operand %lu)",
@@ -7473,6 +7474,11 @@ handle_nonnull_attribute (tree *node, tree ARG_UNUSED (name),
 	      *no_add_attrs = true;
 	      return NULL_TREE;
 	    }
+
+          if (TREE_CODE (type) == METHOD_TYPE && arg_num == 1)
+            warning (OPT_Wattributes,
+                     "nonnull argument references 'this' pointer (argument %lu, operand %lu)",
+                     (unsigned long) attr_arg_num, (unsigned long) arg_num);
 	}
     }
 
diff --git a/gcc/testsuite/g++.dg/warn/nonnull2.C b/gcc/testsuite/g++.dg/warn/nonnull2.C
new file mode 100644
index 0000000..03006b1
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/nonnull2.C
@@ -0,0 +1,14 @@
+// Test that "nonnull" attribute should not be applied to 'this' pointer.
+// { dg-do compile }
+
+#define NULL 0
+
+class Foo {
+ public:
+  void method1(const int *ptr) __attribute__((nonnull(1, 2))); // { dg-warning "nonnull argument references 'this' pointer" }
+  void method2(int *ptr1, int a, int *ptr2) __attribute__((nonnull(2, 3, 4))); // { dg-error "nonnull argument references non-pointer operand" }
+  static void func3(int *ptr) __attribute__((nonnull(1))); // should not warn
+  Foo(char *str) __attribute__((nonnull())) {}
+};
+
+int func4(int *ptr1, int a) __attribute__((nonnull(1))); // should not warn
-- 
1.7.3.1


--
This patch is available for review at http://codereview.appspot.com/4446070

Re: [google] Check if the nonnull attribute is applied to 'this' (issue4446070)

[Richard Guenther]

On Fri, Apr 29, 2011 at 5:08 PM, Diego Novillo <dnovillo@google.com> wrote:
> This patch from Le-Chun Wu adds support to check whether a nonnull
> attribute is applied to 'this' pointer for non-static methods.
>
> OK for trunk?  Applied to google/main
>
> 2011-04-27  Le-Chun Wu  <lcwu@google.com>
>
>        Google ref 45339.
>
>        * c-common.c (handle_nonnull_attribute): Check whether the nonnull
>        attribute is applied to the 'this' pointer for non-static methods.
>
> testsuite/ChangeLog.google-main
> 2011-04-27  Le-Chun Wu  <lcwu@google.com>
>
>        * g++.dg/warn/nonnull2.C: New.
>
> diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
> index c6dc649..a1702f8 100644
> --- a/gcc/c-family/c-common.c
> +++ b/gcc/c-family/c-common.c
> @@ -7434,7 +7434,7 @@ handle_nonnull_attribute (tree *node, tree ARG_UNUSED (name),
>
>   /* Argument list specified.  Verify that each argument number references
>      a pointer argument.  */
> -  for (attr_arg_num = 1; args; args = TREE_CHAIN (args))
> +  for (attr_arg_num = 1; args; args = TREE_CHAIN (args), attr_arg_num++)
>     {
>       tree argument;
>       unsigned HOST_WIDE_INT arg_num = 0, ck_num;
> @@ -7466,6 +7466,7 @@ handle_nonnull_attribute (tree *node, tree ARG_UNUSED (name),
>              return NULL_TREE;
>            }
>
> +

spurious white-space change.

>          if (TREE_CODE (TREE_VALUE (argument)) != POINTER_TYPE)
>            {
>              error ("nonnull argument references non-pointer operand (argument %lu, operand %lu)",
> @@ -7473,6 +7474,11 @@ handle_nonnull_attribute (tree *node, tree ARG_UNUSED (name),
>              *no_add_attrs = true;
>              return NULL_TREE;
>            }
> +
> +          if (TREE_CODE (type) == METHOD_TYPE && arg_num == 1)
> +            warning (OPT_Wattributes,
> +                     "nonnull argument references 'this' pointer (argument %lu, operand %lu)",
> +                     (unsigned long) attr_arg_num, (unsigned long) arg_num);
>        }
>     }
>
> diff --git a/gcc/testsuite/g++.dg/warn/nonnull2.C b/gcc/testsuite/g++.dg/warn/nonnull2.C
> new file mode 100644
> index 0000000..03006b1
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/nonnull2.C
> @@ -0,0 +1,14 @@
> +// Test that "nonnull" attribute should not be applied to 'this' pointer.
> +// { dg-do compile }
> +
> +#define NULL 0
> +
> +class Foo {
> + public:
> +  void method1(const int *ptr) __attribute__((nonnull(1, 2))); // { dg-warning "nonnull argument references 'this' pointer" }
> +  void method2(int *ptr1, int a, int *ptr2) __attribute__((nonnull(2, 3, 4))); // { dg-error "nonnull argument references non-pointer operand" }
> +  static void func3(int *ptr) __attribute__((nonnull(1))); // should not warn
> +  Foo(char *str) __attribute__((nonnull())) {}
> +};
> +
> +int func4(int *ptr1, int a) __attribute__((nonnull(1))); // should not warn
> --
> 1.7.3.1
>
>
> --
> This patch is available for review at http://codereview.appspot.com/4446070
>

Re: [google] Check if the nonnull attribute is applied to 'this' (issue4446070)

[Basile Starynkevitch]

On Fri, 29 Apr 2011 11:08:24 -0400 (EDT)
dnovillo@google.com (Diego Novillo) wrote:

> This patch from Le-Chun Wu adds support to check whether a nonnull
> attribute is applied to 'this' pointer for non-static methods.

This bring me to a question. Does the C++ standard imply that this is
never a null pointer? (Last time I looked into a C++ standard, I was
not able to give a firm answer).

In other words, with 
  class Myclass {
     int x;
   public:
     Myclass(int z=0): x(z) {};
     int get(void) const { if (this) return x; else return 0; };
     ~Myclass() { };
  };
can a C++ standard conforming compiler optimize the get () member
function to 
  inline int Myclass::get(void) { return x; };
(I tend to believe that such an optimization is incorrect, but I am not
sure). can a C++ program call
   Myclass *t = null_ptr;
   return t->get();
(I might believe this is not conforming to standards)

I feel that testing that this is not null is occasionnally useful. For
instance, if I coded a trivial interpreter (e.g. of a Lua or Python or
Lisp-like language) I would be tempted, if that language has a nil
value, to implement values as pointers to objects and to represent nil
as a C++ null_ptr. So I might even want to code something like

// my abstract top superclass for values
class Value {
  virtual ~Value() {};
  Value () {};
  virtual void do_print(void) const = 0;
public:
  void print(void) const { if (this) this->do_print(); };
};

class IntegerValue : public Value {
  int x;
public:
  IntegerValue (int z) : Value(), x(z) {}:
  virtual void do_print (void) { cout << x; };
};

But I am not sure that such code is standard C++. However, GCC seems to
compile it as I expect! At least I still hope that future versions of
GCC would accept it (perhaps in some permissive mode, or with warnings)

Regards.

PS: By standard C++, I mean some recent or future C++ ISO standard (eg C++1x or C++0x).

-- 
Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mine, sont seulement les miennes} ***

Re: [google] Check if the nonnull attribute is applied to 'this' (issue4446070)

[Diego Novillo]

On Fri, Apr 29, 2011 at 14:46, Basile Starynkevitch
<basile@starynkevitch.net> wrote:
> On Fri, 29 Apr 2011 11:08:24 -0400 (EDT)
> dnovillo@google.com (Diego Novillo) wrote:
>
>> This patch from Le-Chun Wu adds support to check whether a nonnull
>> attribute is applied to 'this' pointer for non-static methods.
>
> This bring me to a question. Does the C++ standard imply that this is
> never a null pointer? (Last time I looked into a C++ standard, I was
> not able to give a firm answer).

I didn't find a specific section either, but that's my understanding.
The this pointer can never be NULL.  Jason, Lawrence?


Diego.

Re: [google] Check if the nonnull attribute is applied to 'this' (issue4446070)

[Mike Stump]

On Apr 29, 2011, at 11:46 AM, Basile Starynkevitch wrote:
> On Fri, 29 Apr 2011 11:08:24 -0400 (EDT)
> dnovillo@google.com (Diego Novillo) wrote:
> 
>> This patch from Le-Chun Wu adds support to check whether a nonnull
>> attribute is applied to 'this' pointer for non-static methods.
> 
> This bring me to a question. Does the C++ standard imply that this is
> never a null pointer?

Does this:

4 Certain other operations are described in this International  Standard
  as  undefined  (for  example,  the  effect  of  dereferencing the null
  pointer).  [Note: this International Standard imposes no  requirements
  on the behavior of programs that contain undefined behavior.  ]

answer your question?  Note, this is exactly the same as C.  You can't do it in C either:

struct A {
  int x;
} *pa = 0;

int main() {
  pa->x = 1;
}

If you run it, it will crash on all good OSes.  In C++, it will also crash.

Re: [google] Check if the nonnull attribute is applied to 'this' (issue4446070)

[Basile Starynkevitch]

On Fri, 29 Apr 2011 12:32:24 -0700
Mike Stump <mikestump@comcast.net> wrote:

> On Apr 29, 2011, at 11:46 AM, Basile Starynkevitch wrote:
> > 
> > This bring me to a question. Does the C++ standard imply that this is
> > never a null pointer?
> 
> Does this:
> 
> 4 Certain other operations are described in this International  Standard
>   as  undefined  (for  example,  the  effect  of  dereferencing the null
>   pointer).  [Note: this International Standard imposes no  requirements
>   on the behavior of programs that contain undefined behavior.  ]
> 
> answer your question?  
Not really.

> Note, this is exactly the same as C.  You can't do it in C either:
> 
> struct A {
>   int x;
> } *pa = 0;
> 
> int main() {
>   pa->x = 1;
> }
> 
> If you run it, it will crash on all good OSes.  In C++, it will also crash.

The examples I gave did not crash, because they are non-virtual member
functions. I gave as example

>   class Myclass {
>      int x;
>    public:
>      Myclass(int z=0): x(z) {};
>      int get(void) const { if (this) return x; else return 0; };
>      ~Myclass() { };
>   };

Please notice that get above is a non-virtual member *function*

In the old days when C++ was a translator to C, the
generated C code would be something like

int Myclass__get (MyClass* this)
{ if (this) return this->x; else return 0; }

And such C code won't crash and is conforming to (old and current) C
standard[s].

My example did not dereference a null pointer! I did test some similar
code and on my Linux machine it did work as I expect.

So I still don't know if this can be null or not. After all, it is 
(in C++ parlance) a pointer, not a reference, and C++ pointers can be
null.

with gcc version 4.6.1 20110421 (prerelease) (Debian 4.6.0-5)
(Debian/Experimental on AMD64) the following file

/// file nullthis.cc
  class Myclass {
     int x;
   public:
     Myclass(int z=0): x(z) {};
     int get(void) const { if (this) return x; else return 0; };
     ~Myclass() { };
  };

extern "C" int myget (Myclass *c) { return c->get(); };
//// eof nullthis.cc

is compiled with g++ -Wall -O3 -fverbose-asm -S nullthis.cc
without warnings, and the only produced function in nullthis.s is
	.globl	myget
	.type	myget, @function
myget:
.LFB7:
	.cfi_startproc
	xorl	%eax, %eax	# D.2112
	testq	%rdi, %rdi	# c
	je	.L2	#,
	movl	(%rdi), %eax	# MEM[(const struct Myclass
*)c_1(D)].x, D.2112 .L2:
	rep
	ret
	.cfi_endproc
.LFE7:
	.size	myget, .-myget
	.ident	"GCC: (Debian 4.6.0-5) 4.6.1 20110421
(prerelease)"

This is exactly what I expect. But I don't know if it conforms to
standard.

When compiling with g++ -Wall -O3 -fdump-tree-all -fverbose-asm -S
nullthis.cc I have (among others) a dump file  
% cat nullthis.cc.131t.phiopt3                      

;; Function int myget(Myclass*) (myget)

int myget(Myclass*) (struct Myclass * c)
{
  int D.2112;

<bb 2>:
  if (c_1(D) != 0B)
    goto <bb 3>;
  else
    goto <bb 4>;

<bb 3>:
  D.2112_4 = MEM[(const struct Myclass *)c_1(D)].x;

<bb 4>:
  # D.2112_5 = PHI <0(2), D.2112_4(3)>
  return D.2112_5;

}
which is also what I expect. I have no idea if it is conforming to
standards. But I notice that the test about this is remaining in the
code. So apparently GCC 4.6 does not make the hypothesis that this is
never null, otherwise it would have optimized the test by removing it.
What I don't know (and I am asking) is if such an hypothetical
optimization is conforming to standards. My biased feeling is that it
is not (because I found no explicit & unambigous mention in standard
legal text that this is never null).

Of course, calling a virtual member function on null is obviously an
undefined behavior (and SIGSEGV under Linux).

Regards.
-- 
Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mine, sont seulement les miennes} ***

Re: [google] Check if the nonnull attribute is applied to 'this' (issue4446070)

[Mike Stump]

On Apr 29, 2011, at 1:08 PM, Basile Starynkevitch wrote:
> Not really.

How's this then:

http://stackoverflow.com/questions/2474018/when-does-invoking-a-member-function-on-a-null-instance-result-in-undefined-behav

?  :-)

Re: [google] Check if the nonnull attribute is applied to 'this' (issue4446070)

[Jason Merrill]

On 04/29/2011 04:08 PM, Basile Starynkevitch wrote:
[...]
> which is also what I expect. I have no idea if it is conforming to
> standards. But I notice that the test about this is remaining in the
> code. So apparently GCC 4.6 does not make the hypothesis that this is
> never null, otherwise it would have optimized the test by removing it.
> What I don't know (and I am asking) is if such an hypothetical
> optimization is conforming to standards. My biased feeling is that it
> is not (because I found no explicit&  unambiguous mention in standard
> legal text that this is never null).

9.3.1 seems pretty clear:

> If a non-static member function of a class X is called for an object that is not of type X, or of a type derived
> from X, the behavior is undefined.

A null pointer does not point to an object of type X, so the 
optimization would be conforming.

Jason

Re: [google] Check if the nonnull attribute is applied to 'this' (issue4446070)

[Jason Merrill]

On 05/04/2011 05:08 AM, Richard Guenther wrote:
> On Tue, May 3, 2011 at 10:10 PM,<jason.merrill@gmail.com>  wrote:
>> As I commented on the -Wnonnull patch, rather than complain about people
>> getting the argument number wrong we should ignore 'this' (and other
>> artificial parms) for attribute argument numbers.
>
> I don't think we can reasonably change that now for existing attributes ...

Ah, no, I suppose not.

Jason

Re: [google] Check if the nonnull attribute is applied to 'this' (issue4446070)

[Richard Guenther]

On Tue, May 3, 2011 at 10:10 PM,  <jason.merrill@gmail.com> wrote:
> As I commented on the -Wnonnull patch, rather than complain about people
> getting the argument number wrong we should ignore 'this' (and other
> artificial parms) for attribute argument numbers.

I don't think we can reasonably change that now for existing attributes ...

Richard.

Re: [google] Check if the nonnull attribute is applied to 'this' (issue4446070)

[dnovillo]

On 2011/04/29 15:12:52, richard.guenther_gmail.com wrote:

> >
> > +

> spurious white-space change.

Thanks.  Fixed.


Diego.

http://codereview.appspot.com/4446070/