Re: [google] Check if the nonnull attribute is applied to 'this' (issue4446070)
On 05/04/2011 05:08 AM, Richard Guenther wrote: On Tue, May 3, 2011 at 10:10 PM,jason.merr...@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)
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
[google] Check if the nonnull attribute is applied to 'this' (issue4446070)
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 l...@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 l...@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 000..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)
On Fri, Apr 29, 2011 at 5:08 PM, Diego Novillo dnovi...@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 l...@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 l...@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 000..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)
On 2011/04/29 15:12:52, richard.guenther_gmail.com wrote: + spurious white-space change. Thanks. Fixed. Diego. http://codereview.appspot.com/4446070/
Re: [google] Check if the nonnull attribute is applied to 'this' (issue4446070)
On Fri, 29 Apr 2011 11:08:24 -0400 (EDT) dnovi...@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: basileatstarynkevitchdotnet 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)
On Fri, Apr 29, 2011 at 14:46, Basile Starynkevitch bas...@starynkevitch.net wrote: On Fri, 29 Apr 2011 11:08:24 -0400 (EDT) dnovi...@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)
On Apr 29, 2011, at 11:46 AM, Basile Starynkevitch wrote: On Fri, 29 Apr 2011 11:08:24 -0400 (EDT) dnovi...@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)
On Fri, 29 Apr 2011 12:32:24 -0700 Mike Stump mikest...@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: basileatstarynkevitchdotnet 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)
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 ? :-)