Re: PATCH RFA: C++ frontend warning: comparing function pointer to NULL

Re: PATCH RFA: C++ frontend warning: comparing function pointer to NULL

[Manuel López-Ibáñez]

(Sorry for taking so much time to bring this up.)

Ian added 2 testcases for Walways-true in revision 120493. There are
at least 3 existing testcases that also deal with Walways-true:

* gcc.dg/20031012-1.c
* gcc.dg/weak/weak-3.c
* g++.old-deja/g++.mike/warn8.C

They were added in revision 108689.

Wouldn't it be appropriate to integrate them into the testcases added
by Ian and remove any duplicated tests?

Cheers,

Manuel.

Re: PATCH RFA: C++ frontend warning: comparing function pointer to NULL

[Gabriel Dos Reis]

"Manuel López-Ibáñez" <lopezibanez@gmail.com> writes:

| (Sorry for taking so much time to bring this up.)
| 
| Ian added 2 testcases for Walways-true in revision 120493. There are
| at least 3 existing testcases that also deal with Walways-true:
| 
| * gcc.dg/20031012-1.c
| * gcc.dg/weak/weak-3.c
| * g++.old-deja/g++.mike/warn8.C
| 
| They were added in revision 108689.
| 
| Wouldn't it be appropriate to integrate them into the testcases added
| by Ian and remove any duplicated tests?

Yes, that sounds good to me.

-- Gaby

Re: PATCH RFA: C++ frontend warning: comparing function pointer to NULL

[Ian Lance Taylor]

"Manuel López-Ibáñez" <lopezibanez@gmail.com> writes:

> Ian added 2 testcases for Walways-true in revision 120493. There are
> at least 3 existing testcases that also deal with Walways-true:
> 
> * gcc.dg/20031012-1.c
> * gcc.dg/weak/weak-3.c
> * g++.old-deja/g++.mike/warn8.C
> 
> They were added in revision 108689.
> 
> Wouldn't it be appropriate to integrate them into the testcases added
> by Ian and remove any duplicated tests?

Sure.

Ian

Re: PATCH RFA: C++ frontend warning: comparing function pointer to NULL

[Manuel López-Ibáñez]

On 07 Jan 2007 11:27:59 -0800, Ian Lance Taylor <iant@google.com> wrote:
> "Manuel López-Ibáñez" <lopezibanez@gmail.com> writes:
>
> > Ian added 2 testcases for Walways-true in revision 120493. There are
> > at least 3 existing testcases that also deal with Walways-true:
> >
> > * gcc.dg/20031012-1.c
> > * gcc.dg/weak/weak-3.c
> > * g++.old-deja/g++.mike/warn8.C
> >
> > They were added in revision 108689.
> >
> > Wouldn't it be appropriate to integrate them into the testcases added
> > by Ian and remove any duplicated tests?
>
> Sure.

Sorry for being such a bother but does this mean "Sure, I will do it"
or "Sure, go ahead and do it" ?

Also, perhaps we could consider this something to take into account
before sending patches that add new testcases. Most of the time, a
simple grep on the option name or a representative part of the
expected output will bring up similar (and thus redundant) testcases.
Maybe this could be commented somewhere...

Cheers,

Manuel.

Re: PATCH RFA: C++ frontend warning: comparing function pointer to NULL

[Joseph S. Myers]

On Sun, 7 Jan 2007, Manuel Lopez-Ibanez wrote:

> Also, perhaps we could consider this something to take into account
> before sending patches that add new testcases. Most of the time, a
> simple grep on the option name or a representative part of the
> expected output will bring up similar (and thus redundant) testcases.
> Maybe this could be commented somewhere...

It's generally a mistake to consider testcases redundant unless actually 
identical.  While two testcases may appear to test the same thing with the 
current implementation while the compiler is working correctly, a 
reimplementation of a feature or diagnostic could cause them to take 
different code paths, and a regression could break one similar case but 
not the other.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: PATCH RFA: C++ frontend warning: comparing function pointer to NULL

[Manuel López-Ibáñez]

On 07/01/07, Joseph S. Myers <joseph@codesourcery.com> wrote:
> On Sun, 7 Jan 2007, Manuel Lopez-Ibanez wrote:
>
> > Also, perhaps we could consider this something to take into account
> > before sending patches that add new testcases. Most of the time, a
> > simple grep on the option name or a representative part of the
> > expected output will bring up similar (and thus redundant) testcases.
> > Maybe this could be commented somewhere...
>
> It's generally a mistake to consider testcases redundant unless actually
> identical.  While two testcases may appear to test the same thing with the
> current implementation while the compiler is working correctly, a
> reimplementation of a feature or diagnostic could cause them to take
> different code paths, and a regression could break one similar case but
> not the other.

Of course, being strict, unless the two testcase files are byte-wise
identical, anything may happen. But I guess that there may be some
practical assumptions about the equivalence of testcases that allows
to avoid testing every possible variation of any random testcase.

For example, I would consider that it is generally safe to assume from
a practical point of view that if two testcases vary only in
whitespace, then they may be considered equivalent. I would go a bit
further and consider that if two testcases vary only on the names of
identifiers, then they may be considered equivalent. I guess we could
find more practical assumptions.

Of course, equivalence should be decided for each testcase. What I
wished to say is that it should be taken into account. Or perhaps not,
if we don't care about the testsuite size, regression testing time and
testcases getting silently obsolete. It was more a subtle question
that a firm proposal.

Here is another question, if I find a testcase in g++.old-deja that is
relevant for a patch that I am going to submit, would it be
appropriate to move it to g++.dg/X and give it an apter name?

For example, if I am preparing testcases such Wsuperwarning-1.C,
Wsuperwarning-2.C and I find that g++.old-deja/g++.mike/warn19.C is
relevant for -Wsuperwarning, should I do "mv
g++.old-deja/g++.mike/warn19.C g++.dg/warn/Wsuperwarning-3.C ?

Cheers,

Manuel.

Re: PATCH RFA: C++ frontend warning: comparing function pointer to NULL

[Ian Lance Taylor]

"Manuel López-Ibáñez" <lopezibanez@gmail.com> writes:

> On 07 Jan 2007 11:27:59 -0800, Ian Lance Taylor <iant@google.com> wrote:
> > "Manuel López-Ibáñez" <lopezibanez@gmail.com> writes:
> >
> > > Ian added 2 testcases for Walways-true in revision 120493. There are
> > > at least 3 existing testcases that also deal with Walways-true:
> > >
> > > * gcc.dg/20031012-1.c
> > > * gcc.dg/weak/weak-3.c
> > > * g++.old-deja/g++.mike/warn8.C
> > >
> > > They were added in revision 108689.
> > >
> > > Wouldn't it be appropriate to integrate them into the testcases added
> > > by Ian and remove any duplicated tests?
> >
> > Sure.
> 
> Sorry for being such a bother but does this mean "Sure, I will do it"
> or "Sure, go ahead and do it" ?

Well, I meant "sure, go ahead and do it."

But it's also true, as Mark alluded to, that it's inadvisable to
modify an existing test (it's OK to modify the tests I just added).  I
personally think it's OK to actually remove an old test.  But I also
think you need to run the idea past Janis, the testsuite maintainer.

I did look a bit for existing tests, but I managed to miss the ones
you mentioned.  I didn't even realize there was a gcc.dg/weak
subdirectory.

Ian

Re: PATCH RFA: C++ frontend warning: comparing function pointer to NULL

[Mark Mitchell]

Ian Lance Taylor wrote:
> Mark Mitchell <mark@codesourcery.com> writes:
> 
>> I hate to be niggly, but it looks like you've introduced two copies of
>> the same test here (modulo op0 vs. op1), and it sounds like the C front
>> end has at least one more copy.  Would you mind creating a function,
>> shared as widely as possible?
> 
> How about this version?  Since I changed the C frontend, I added C
> test cases too (they are actually exactly the same as the C++ test
> cases, except for comment syntax).

OK, and thanks for going along with the niggle.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: PATCH RFA: C++ frontend warning: comparing function pointer to NULL

[Ian Lance Taylor]

Mark Mitchell <mark@codesourcery.com> writes:

> I hate to be niggly, but it looks like you've introduced two copies of
> the same test here (modulo op0 vs. op1), and it sounds like the C front
> end has at least one more copy.  Would you mind creating a function,
> shared as widely as possible?

How about this version?  Since I changed the C frontend, I added C
test cases too (they are actually exactly the same as the C++ test
cases, except for comment syntax).

Bootstrapped on i686-pc-linux-gnu, testsuite run in progress.

Ian

Index: gcc/c-common.c
===================================================================
--- gcc/c-common.c	(revision 120376)
+++ gcc/c-common.c	(working copy)
@@ -2591,6 +2591,18 @@ pointer_int_sum (enum tree_code resultco
   return fold_build2 (resultcode, result_type, ptrop, intop);
 }
 \f
+/* Return whether EXPR is a declaration whose address can never be
+   NULL.  */
+
+bool
+decl_with_nonnull_addr_p (tree expr)
+{
+  return (DECL_P (expr)
+	  && (TREE_CODE (expr) == PARM_DECL
+	      || TREE_CODE (expr) == LABEL_DECL
+	      || !DECL_WEAK (expr)));
+}
+
 /* Prepare expr to be an argument of a TRUTH_NOT_EXPR,
    or for an `if' or `while' statement or ?..: exp.  It should already
    have been validated to be of suitable type; otherwise, a bad
@@ -2656,23 +2668,22 @@ c_common_truthvalue_conversion (tree exp
     case ADDR_EXPR:
       {
  	tree inner = TREE_OPERAND (expr, 0);
-	if (DECL_P (inner)
-	    && (TREE_CODE (inner) == PARM_DECL
-		|| TREE_CODE (inner) == LABEL_DECL
-		|| !DECL_WEAK (inner)))
+	if (decl_with_nonnull_addr_p (inner))
 	  {
-	    /* Common Ada/Pascal programmer's mistake.  We always warn
-	       about this since it is so bad.  */
-	    warning (OPT_Walways_true, "the address of %qD will always evaluate as %<true%>",
+	    /* Common Ada/Pascal programmer's mistake.  */
+	    warning (OPT_Walways_true,
+		     "the address of %qD will always evaluate as %<true%>",
 		     inner);
 	    return truthvalue_true_node;
 	  }
 
-	/* If we are taking the address of an external decl, it might be
-	   zero if it is weak, so we cannot optimize.  */
-	if (DECL_P (inner)
-	    && DECL_EXTERNAL (inner))
-	  break;
+	/* If we still have a decl, it is possible for its address to
+	   be NULL, so we cannot optimize.  */
+	if (DECL_P (inner))
+	  {
+	    gcc_assert (DECL_WEAK (inner));
+	    break;
+	  }
 
 	if (TREE_SIDE_EFFECTS (inner))
 	  return build2 (COMPOUND_EXPR, truthvalue_type_node,
Index: gcc/c-typeck.c
===================================================================
--- gcc/c-typeck.c	(revision 120376)
+++ gcc/c-typeck.c	(working copy)
@@ -8013,10 +8013,7 @@ build_binary_op (enum tree_code code, tr
       else if (code0 == POINTER_TYPE && null_pointer_constant_p (orig_op1))
 	{
 	  if (TREE_CODE (op0) == ADDR_EXPR
-	      && DECL_P (TREE_OPERAND (op0, 0))
-	      && (TREE_CODE (TREE_OPERAND (op0, 0)) == PARM_DECL
-		  || TREE_CODE (TREE_OPERAND (op0, 0)) == LABEL_DECL
-		  || !DECL_WEAK (TREE_OPERAND (op0, 0))))
+	      && decl_with_nonnull_addr_p (TREE_OPERAND (op0, 0)))
 	    warning (OPT_Walways_true, "the address of %qD will never be NULL",
 		     TREE_OPERAND (op0, 0));
 	  result_type = type0;
@@ -8024,10 +8021,7 @@ build_binary_op (enum tree_code code, tr
       else if (code1 == POINTER_TYPE && null_pointer_constant_p (orig_op0))
 	{
 	  if (TREE_CODE (op1) == ADDR_EXPR
-	      && DECL_P (TREE_OPERAND (op1, 0))
-	      && (TREE_CODE (TREE_OPERAND (op1, 0)) == PARM_DECL
-		  || TREE_CODE (TREE_OPERAND (op1, 0)) == LABEL_DECL
-		  || !DECL_WEAK (TREE_OPERAND (op1, 0))))
+	      && decl_with_nonnull_addr_p (TREE_OPERAND (op1, 0)))
 	    warning (OPT_Walways_true, "the address of %qD will never be NULL",
 		     TREE_OPERAND (op1, 0));
 	  result_type = type1;
Index: gcc/c-common.h
===================================================================
--- gcc/c-common.h	(revision 120376)
+++ gcc/c-common.h	(working copy)
@@ -652,6 +652,7 @@ extern tree c_common_unsigned_type (tree
 extern tree c_common_signed_type (tree);
 extern tree c_common_signed_or_unsigned_type (int, tree);
 extern tree c_build_bitfield_integer_type (unsigned HOST_WIDE_INT, int);
+extern bool decl_with_nonnull_addr_p (tree);
 extern tree c_common_truthvalue_conversion (tree);
 extern void c_apply_type_quals_to_decl (int, tree);
 extern tree c_sizeof_or_alignof_type (tree, bool, int);
Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c	(revision 120376)
+++ gcc/cp/typeck.c	(working copy)
@@ -3334,10 +3334,22 @@ build_binary_op (enum tree_code code, tr
 					      "comparison");
       else if ((code0 == POINTER_TYPE || TYPE_PTRMEM_P (type0))
 	       && null_ptr_cst_p (op1))
-	result_type = type0;
+	{
+	  if (TREE_CODE (op0) == ADDR_EXPR
+	      && decl_with_nonnull_addr_p (TREE_OPERAND (op0, 0)))
+	    warning (OPT_Walways_true, "the address of %qD will never be NULL",
+		     TREE_OPERAND (op0, 0));
+	  result_type = type0;
+	}
       else if ((code1 == POINTER_TYPE || TYPE_PTRMEM_P (type1))
 	       && null_ptr_cst_p (op0))
-	result_type = type1;
+	{
+	  if (TREE_CODE (op1) == ADDR_EXPR 
+	      && decl_with_nonnull_addr_p (TREE_OPERAND (op1, 0)))
+	    warning (OPT_Walways_true, "the address of %qD will never be NULL",
+		     TREE_OPERAND (op1, 0));
+	  result_type = type1;
+	}
       else if (code0 == POINTER_TYPE && code1 == INTEGER_TYPE)
 	{
 	  result_type = type0;
Index: gcc/testsuite/gcc.dg/Walways-true-1.c
===================================================================
--- gcc/testsuite/gcc.dg/Walways-true-1.c	(revision 0)
+++ gcc/testsuite/gcc.dg/Walways-true-1.c	(revision 0)
@@ -0,0 +1,57 @@
+/* Test -Walways-true for testing an address against NULL.
+   Origin: Ian Lance Taylor <iant@google.com>.  */
+
+/* { dg-do compile} */
+/* { dg-options "-Walways-true" } */
+
+extern int foo (int);
+
+int i;
+
+void
+bar (int a)
+{
+ lab:
+  if (foo)	/* { dg-warning "always evaluate as" "correct warning" } */
+    foo (0);
+  if (foo (1))
+    ;
+  if (&i)	/* { dg-warning "always evaluate as" "correct warning" } */
+    foo (2);
+  if (i)
+    foo (3);
+  if (&a)	/* { dg-warning "always evaluate as" "correct warning" } */
+    foo (4);
+  if (a)
+    foo (5);
+  if (&&lab)	/* { dg-warning "always evaluate as" "correct warning" } */
+    foo (6);
+  if (foo == 0)	/* { dg-warning "never be NULL" "correct warning" } */
+    foo (7);
+  if (foo (1) == 0)
+    foo (8);
+  if (&i == 0)	/* { dg-warning "never be NULL" "correct warning" } */
+    foo (9);
+  if (i == 0)
+    foo (10);
+  if (&a == 0)	/* { dg-warning "never be NULL" "correct warning" } */
+    foo (11);
+  if (a == 0)
+    foo (12);
+  if (&&lab == 0) /* { dg-warning "never be NULL" "correct warning" } */
+    foo (13);
+  if (0 == foo)	/* { dg-warning "never be NULL" "correct warning" } */
+    foo (14);
+  if (0 == foo (1))
+    foo (15);
+  if (0 == &i)	/* { dg-warning "never be NULL" "correct warning" } */
+    foo (16);
+  if (0 == i)
+    foo (17);
+  if (0 == &a)	/* { dg-warning "never be NULL" "correct warning" } */
+    foo (18);
+  if (0 == a)
+    foo (19);
+  if (0 == &&lab) /* { dg-warning "never be NULL" "correct warning" } */
+    foo (20);
+}
Index: gcc/testsuite/gcc.dg/Walways-true-2.c
===================================================================
--- gcc/testsuite/gcc.dg/Walways-true-2.c	(revision 0)
+++ gcc/testsuite/gcc.dg/Walways-true-2.c	(revision 0)
@@ -0,0 +1,60 @@
+/* Make sure we don't assume that a weak symbol is always non-NULL.
+   This is just like Walways-true-1.C, except that it uses a weak
+   symbol.
+   Origin: Ian Lance Taylor <iant@google.com>.  */
+
+/* { dg-do compile} */
+/* { dg-options "-Walways-true" } */
+/* { dg-require-weak "" } */
+
+extern int foo (int) __attribute__ ((weak));
+
+int i __attribute__ ((weak));
+
+void
+bar (int a)
+{
+ lab:
+  if (foo)
+    foo (0);
+  if (foo (1))
+    ;
+  if (&i)
+    foo (2);
+  if (i)
+    foo (3);
+  if (&a)	/* { dg-warning "always evaluate as" "correct warning" } */
+    foo (4);
+  if (a)
+    foo (5);
+  if (&&lab)	/* { dg-warning "always evaluate as" "correct warning" } */
+    foo (6);
+  if (foo == 0)
+    foo (7);
+  if (foo (1) == 0)
+    foo (8);
+  if (&i == 0)
+    foo (9);
+  if (i == 0)
+    foo (10);
+  if (&a == 0)	/* { dg-warning "never be NULL" "correct warning" } */
+    foo (11);
+  if (a == 0)
+    foo (12);
+  if (&&lab == 0) /* { dg-warning "never be NULL" "correct warning" } */
+    foo (13);
+  if (0 == foo)
+    foo (14);
+  if (0 == foo (1))
+    foo (15);
+  if (0 == &i)
+    foo (16);
+  if (0 == i)
+    foo (17);
+  if (0 == &a)	/* { dg-warning "never be NULL" "correct warning" } */
+    foo (18);
+  if (0 == a)
+    foo (19);
+  if (0 == &&lab) /* { dg-warning "never be NULL" "correct warning" } */
+    foo (20);
+}
Index: gcc/testsuite/g++.dg/warn/Walways-true-1.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Walways-true-1.C	(revision 0)
+++ gcc/testsuite/g++.dg/warn/Walways-true-1.C	(revision 0)
@@ -0,0 +1,57 @@
+// Test -Walways-true for testing an address against NULL.
+// Origin: Ian Lance Taylor <iant@google.com>
+
+// { dg-do compile}
+// { dg-options "-Walways-true" }
+
+extern int foo (int);
+
+int i;
+
+void
+bar (int a)
+{
+ lab:
+  if (foo)	// { dg-warning "always evaluate as" "correct warning" }
+    foo (0);
+  if (foo (1))
+    ;
+  if (&i)	// { dg-warning "always evaluate as" "correct warning" }
+    foo (2);
+  if (i)
+    foo (3);
+  if (&a)	// { dg-warning "always evaluate as" "correct warning" }
+    foo (4);
+  if (a)
+    foo (5);
+  if (&&lab)	// { dg-warning "always evaluate as" "correct warning" }
+    foo (6);
+  if (foo == 0)	// { dg-warning "never be NULL" "correct warning" }
+    foo (7);
+  if (foo (1) == 0)
+    foo (8);
+  if (&i == 0)	// { dg-warning "never be NULL" "correct warning" }
+    foo (9);
+  if (i == 0)
+    foo (10);
+  if (&a == 0)	// { dg-warning "never be NULL" "correct warning" }
+    foo (11);
+  if (a == 0)
+    foo (12);
+  if (&&lab == 0) // { dg-warning "never be NULL" "correct warning" }
+    foo (13);
+  if (0 == foo)	// { dg-warning "never be NULL" "correct warning" }
+    foo (14);
+  if (0 == foo (1))
+    foo (15);
+  if (0 == &i)	// { dg-warning "never be NULL" "correct warning" }
+    foo (16);
+  if (0 == i)
+    foo (17);
+  if (0 == &a)	// { dg-warning "never be NULL" "correct warning" }
+    foo (18);
+  if (0 == a)
+    foo (19);
+  if (0 == &&lab) // { dg-warning "never be NULL" "correct warning" }
+    foo (20);
+}
Index: gcc/testsuite/g++.dg/warn/Walways-true-2.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Walways-true-2.C	(revision 0)
+++ gcc/testsuite/g++.dg/warn/Walways-true-2.C	(revision 0)
@@ -0,0 +1,60 @@
+// Make sure we don't assume that a weak symbol is always non-NULL.
+// This is just like Walways-true-1.C, except that it uses a weak
+// symbol.
+// Origin: Ian Lance Taylor <iant@google.com>
+
+// { dg-do compile}
+// { dg-options "-Walways-true" }
+// { dg-require-weak "" }
+
+extern int foo (int) __attribute__ ((weak));
+
+int i __attribute__ ((weak));
+
+void
+bar (int a)
+{
+ lab:
+  if (foo)
+    foo (0);
+  if (foo (1))
+    ;
+  if (&i)
+    foo (2);
+  if (i)
+    foo (3);
+  if (&a)	// { dg-warning "always evaluate as" "correct warning" }
+    foo (4);
+  if (a)
+    foo (5);
+  if (&&lab)	// { dg-warning "always evaluate as" "correct warning" }
+    foo (6);
+  if (foo == 0)
+    foo (7);
+  if (foo (1) == 0)
+    foo (8);
+  if (&i == 0)
+    foo (9);
+  if (i == 0)
+    foo (10);
+  if (&a == 0)	// { dg-warning "never be NULL" "correct warning" }
+    foo (11);
+  if (a == 0)
+    foo (12);
+  if (&&lab == 0) // { dg-warning "never be NULL" "correct warning" }
+    foo (13);
+  if (0 == foo)
+    foo (14);
+  if (0 == foo (1))
+    foo (15);
+  if (0 == &i)
+    foo (16);
+  if (0 == i)
+    foo (17);
+  if (0 == &a)	// { dg-warning "never be NULL" "correct warning" }
+    foo (18);
+  if (0 == a)
+    foo (19);
+  if (0 == &&lab) // { dg-warning "never be NULL" "correct warning" }
+    foo (20);
+}

Re: PATCH RFA: C++ frontend warning: comparing function pointer to NULL

[Gabriel Dos Reis]

Ian Lance Taylor <iant@google.com> writes:

| Andrew Pinski <pinskia@physics.uc.edu> writes:
| 
| > I remember when the C code was added for this warning (which looked almost
| > the same), we would ICE for code like:
| > 
| > int f(int a)
| > {
| >   return (&a) == 0;
| > }
| > 
| > Looking at the above code looks like we would ICE in the C++ code with the
| > above testcase.
| > The C front-end checks for PARM and LABEL decls too:
| >           if (TREE_CODE (op0) == ADDR_EXPR 
| >               && DECL_P (TREE_OPERAND (op0, 0))
| >               && (TREE_CODE (TREE_OPERAND (op0, 0)) == PARM_DECL
| >                   || TREE_CODE (TREE_OPERAND (op0, 0)) == LABEL_DECL
| >                   || !DECL_WEAK (TREE_OPERAND (op0, 0))))
| > 
| > Could you add a testcase for the argument and label cases and make sure
| > we don't ICE on them?
| 
| You're absolutely right.  Thanks a lot for the pointer.
| 
| Here is the updated, retested, patch.
| 
| Ian
| 
| Index: gcc/cp/typeck.c
| ===================================================================
| --- gcc/cp/typeck.c	(revision 120376)
| +++ gcc/cp/typeck.c	(working copy)
| @@ -3334,10 +3334,28 @@ build_binary_op (enum tree_code code, tr
|  					      "comparison");
|        else if ((code0 == POINTER_TYPE || TYPE_PTRMEM_P (type0))
|  	       && null_ptr_cst_p (op1))
| -	result_type = type0;
| +	{
| +	  if (TREE_CODE (op0) == ADDR_EXPR
| +	      && DECL_P (TREE_OPERAND (op0, 0)) 
| +	      && (TREE_CODE (TREE_OPERAND (op0, 0)) == PARM_DECL
| +		  || TREE_CODE (TREE_OPERAND (op0, 0)) == LABEL_DECL
| +		  || !DECL_WEAK (TREE_OPERAND (op0, 0))))
| +	    warning (OPT_Walways_true, "the address of %qD will never be NULL",
| +		     TREE_OPERAND (op0, 0));
| +	  result_type = type0;
| +	}
|        else if ((code1 == POINTER_TYPE || TYPE_PTRMEM_P (type1))
|  	       && null_ptr_cst_p (op0))
| -	result_type = type1;
| +	{
| +	  if (TREE_CODE (op1) == ADDR_EXPR 
| +	      && DECL_P (TREE_OPERAND (op1, 0))
| +	      && (TREE_CODE (TREE_OPERAND (op0, 0)) == PARM_DECL
| +		  || TREE_CODE (TREE_OPERAND (op0, 0)) == LABEL_DECL
| +		  || !DECL_WEAK (TREE_OPERAND (op0, 0))))
| +	    warning (OPT_Walways_true, "the address of %qD will never be NULL",
| +		     TREE_OPERAND (op1, 0));
| +	  result_type = type1;


You get brownie point, if you could abstract that check into a
separate (static inline) function.  Otherwise, OK.

-- Gaby

Re: PATCH RFA: C++ frontend warning: comparing function pointer to NULL

[Mark Mitchell]

Ian Lance Taylor wrote:

> You're absolutely right.  Thanks a lot for the pointer.
> 
> Here is the updated, retested, patch.
> 
> Ian
> 
> Index: gcc/cp/typeck.c
> ===================================================================
> --- gcc/cp/typeck.c	(revision 120376)
> +++ gcc/cp/typeck.c	(working copy)
> @@ -3334,10 +3334,28 @@ build_binary_op (enum tree_code code, tr
>  					      "comparison");
>        else if ((code0 == POINTER_TYPE || TYPE_PTRMEM_P (type0))
>  	       && null_ptr_cst_p (op1))
> -	result_type = type0;
> +	{
> +	  if (TREE_CODE (op0) == ADDR_EXPR
> +	      && DECL_P (TREE_OPERAND (op0, 0)) 
> +	      && (TREE_CODE (TREE_OPERAND (op0, 0)) == PARM_DECL
> +		  || TREE_CODE (TREE_OPERAND (op0, 0)) == LABEL_DECL
> +		  || !DECL_WEAK (TREE_OPERAND (op0, 0))))
> +	    warning (OPT_Walways_true, "the address of %qD will never be NULL",
> +		     TREE_OPERAND (op0, 0));
> +	  result_type = type0;
> +	}

I hate to be niggly, but it looks like you've introduced two copies of
the same test here (modulo op0 vs. op1), and it sounds like the C front
end has at least one more copy.  Would you mind creating a function,
shared as widely as possible?

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: PATCH RFA: C++ frontend warning: comparing function pointer to NULL

[Ian Lance Taylor]

Andrew Pinski <pinskia@physics.uc.edu> writes:

> I remember when the C code was added for this warning (which looked almost
> the same), we would ICE for code like:
> 
> int f(int a)
> {
>   return (&a) == 0;
> }
> 
> Looking at the above code looks like we would ICE in the C++ code with the
> above testcase.
> The C front-end checks for PARM and LABEL decls too:
>           if (TREE_CODE (op0) == ADDR_EXPR 
>               && DECL_P (TREE_OPERAND (op0, 0))
>               && (TREE_CODE (TREE_OPERAND (op0, 0)) == PARM_DECL
>                   || TREE_CODE (TREE_OPERAND (op0, 0)) == LABEL_DECL
>                   || !DECL_WEAK (TREE_OPERAND (op0, 0))))
> 
> Could you add a testcase for the argument and label cases and make sure
> we don't ICE on them?

You're absolutely right.  Thanks a lot for the pointer.

Here is the updated, retested, patch.

Ian

Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c	(revision 120376)
+++ gcc/cp/typeck.c	(working copy)
@@ -3334,10 +3334,28 @@ build_binary_op (enum tree_code code, tr
 					      "comparison");
       else if ((code0 == POINTER_TYPE || TYPE_PTRMEM_P (type0))
 	       && null_ptr_cst_p (op1))
-	result_type = type0;
+	{
+	  if (TREE_CODE (op0) == ADDR_EXPR
+	      && DECL_P (TREE_OPERAND (op0, 0)) 
+	      && (TREE_CODE (TREE_OPERAND (op0, 0)) == PARM_DECL
+		  || TREE_CODE (TREE_OPERAND (op0, 0)) == LABEL_DECL
+		  || !DECL_WEAK (TREE_OPERAND (op0, 0))))
+	    warning (OPT_Walways_true, "the address of %qD will never be NULL",
+		     TREE_OPERAND (op0, 0));
+	  result_type = type0;
+	}
       else if ((code1 == POINTER_TYPE || TYPE_PTRMEM_P (type1))
 	       && null_ptr_cst_p (op0))
-	result_type = type1;
+	{
+	  if (TREE_CODE (op1) == ADDR_EXPR 
+	      && DECL_P (TREE_OPERAND (op1, 0))
+	      && (TREE_CODE (TREE_OPERAND (op0, 0)) == PARM_DECL
+		  || TREE_CODE (TREE_OPERAND (op0, 0)) == LABEL_DECL
+		  || !DECL_WEAK (TREE_OPERAND (op0, 0))))
+	    warning (OPT_Walways_true, "the address of %qD will never be NULL",
+		     TREE_OPERAND (op1, 0));
+	  result_type = type1;
+	}
       else if (code0 == POINTER_TYPE && code1 == INTEGER_TYPE)
 	{
 	  result_type = type0;
Index: gcc/testsuite/g++.dg/warn/Walways-true-1.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Walways-true-1.C	(revision 0)
+++ gcc/testsuite/g++.dg/warn/Walways-true-1.C	(revision 0)
@@ -0,0 +1,43 @@
+// Test -Walways-true for testing an address against NULL.
+// Origin: Ian Lance Taylor <iant@google.com>
+
+// { dg-do compile}
+// { dg-options "-Walways-true" }
+
+extern int foo (int);
+
+int i;
+
+void
+bar (int a)
+{
+ lab:
+  if (foo)	// { dg-warning "always evaluate as" "correct warning" }
+    foo (0);
+  if (foo (1))
+    foo (1);
+  if (&i)	// { dg-warning "always evaluate as" "correct warning" }
+    foo (2);
+  if (i)
+    foo (3);
+  if (&a)	// { dg-warning "always evaluate as" "correct warning" }
+    foo (4);
+  if (a)
+    foo (5);
+  if (&&lab)	// { dg-warning "always evaluate as" "correct warning" }
+    foo (6);
+  if (foo == 0)	// { dg-warning "never be NULL" "correct warning" }
+    foo (7);
+  if (foo (1) == 0)
+    foo (9);
+  if (&i == 0)	// { dg-warning "never be NULL" "correct warning" }
+    foo (9);
+  if (i == 0)
+    foo (10);
+  if (&a == 0)	// { dg-warning "never be NULL" "correct warning" }
+    foo (11);
+  if (a == 0)
+    foo (12);
+  if (&&lab == 0) // { dg-warning "never be NULL" "correct warning" }
+    foo (13);
+}
Index: gcc/testsuite/g++.dg/warn/Walways-true-2.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Walways-true-2.C	(revision 0)
+++ gcc/testsuite/g++.dg/warn/Walways-true-2.C	(revision 0)
@@ -0,0 +1,46 @@
+// Make sure we don't assume that a weak symbol is always non-NULL.
+// This is just like Walways-true-1.C, except that it uses a weak
+// symbol.
+// Origin: Ian Lance Taylor <iant@google.com>
+
+// { dg-do compile}
+// { dg-options "-Walways-true" }
+// { dg-require-weak "" }
+
+extern int foo (int) __attribute__ ((weak));
+
+int i __attribute__ ((weak));
+
+void
+bar (int a)
+{
+ lab:
+  if (foo)
+    foo (0);
+  if (foo (1))
+    foo (1);
+  if (&i)
+    foo (2);
+  if (i)
+    foo (3);
+  if (&a)	// { dg-warning "always evaluate as" "correct warning" }
+    foo (4);
+  if (a)
+    foo (5);
+  if (&&lab)	// { dg-warning "always evaluate as" "correct warning" }
+    foo (6);
+  if (foo == 0)
+    foo (7);
+  if (foo (1) == 0)
+    foo (9);
+  if (&i == 0)
+    foo (9);
+  if (i == 0)
+    foo (10);
+  if (&a == 0)	// { dg-warning "never be NULL" "correct warning" }
+    foo (11);
+  if (a == 0)
+    foo (12);
+  if (&&lab == 0) // { dg-warning "never be NULL" "correct warning" }
+    foo (13);
+}

Re: PATCH RFA: C++ frontend warning: comparing function pointer to NULL

[Andrew Pinski]

> 
> Ian
> 
> 
> cp/ChangeLog:
> 2007-01-04  Ian Lance Taylor  <iant@google.com>
> 
> 	* typeck.c (build_binary_op): Warn about comparing a non-weak
> 	address to NULL.
> 
> testsuite/ChangeLog:
> 2007-01-04  Ian Lance Taylor  <iant@google.com>
> 
> 	* g++.dg/warn/Walways-true-1.C: New test.
> 	* g++.dg/warn/Walways-true-2.C: New test.
> 
> 
> Index: gcc/cp/typeck.c
> ===================================================================
> --- gcc/cp/typeck.c	(revision 120376)
> +++ gcc/cp/typeck.c	(working copy)
> @@ -3334,10 +3334,24 @@ build_binary_op (enum tree_code code, tr
>  					      "comparison");
>        else if ((code0 == POINTER_TYPE || TYPE_PTRMEM_P (type0))
>  	       && null_ptr_cst_p (op1))
> -	result_type = type0;
> +	{
> +	  if (TREE_CODE (op0) == ADDR_EXPR
> +	      && DECL_P (TREE_OPERAND (op0, 0)) 
> +	      && !DECL_WEAK (TREE_OPERAND (op0, 0)))
> +	    warning (OPT_Walways_true, "the address of %qD will never be NULL",
> +		     TREE_OPERAND (op0, 0));

I remember when the C code was added for this warning (which looked almost
the same), we would ICE for code like:

int f(int a)
{
  return (&a) == 0;
}

Looking at the above code looks like we would ICE in the C++ code with the
above testcase.
The C front-end checks for PARM and LABEL decls too:
          if (TREE_CODE (op0) == ADDR_EXPR 
              && DECL_P (TREE_OPERAND (op0, 0))
              && (TREE_CODE (TREE_OPERAND (op0, 0)) == PARM_DECL
                  || TREE_CODE (TREE_OPERAND (op0, 0)) == LABEL_DECL
                  || !DECL_WEAK (TREE_OPERAND (op0, 0))))

Could you add a testcase for the argument and label cases and make sure
we don't ICE on them?

Thanks,
Andrew Pinski

PATCH RFA: C++ frontend warning: comparing function pointer to NULL

[Ian Lance Taylor]

If you write code like this:
    extern int foo();
    if (foo == 0)
      ...
the C frontend will issue a warning:
  the address of 'foo' will never be NULL

This is useful for the case where somebody wrote "foo" instead of
"foo()".

The C++ frontend does not issue this warning.  This patch adds the
warning to the C++ frontend.

In the testsuite I added a couple of tests for
   if (foo)
which is a warning which the C++ frontend already generates.

Tested with bootstrap and testsuite run on i686-pc-linux-gnu.

OK for mainline?

Ian


cp/ChangeLog:
2007-01-04  Ian Lance Taylor  <iant@google.com>

	* typeck.c (build_binary_op): Warn about comparing a non-weak
	address to NULL.

testsuite/ChangeLog:
2007-01-04  Ian Lance Taylor  <iant@google.com>

	* g++.dg/warn/Walways-true-1.C: New test.
	* g++.dg/warn/Walways-true-2.C: New test.


Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c	(revision 120376)
+++ gcc/cp/typeck.c	(working copy)
@@ -3334,10 +3334,24 @@ build_binary_op (enum tree_code code, tr
 					      "comparison");
       else if ((code0 == POINTER_TYPE || TYPE_PTRMEM_P (type0))
 	       && null_ptr_cst_p (op1))
-	result_type = type0;
+	{
+	  if (TREE_CODE (op0) == ADDR_EXPR
+	      && DECL_P (TREE_OPERAND (op0, 0)) 
+	      && !DECL_WEAK (TREE_OPERAND (op0, 0)))
+	    warning (OPT_Walways_true, "the address of %qD will never be NULL",
+		     TREE_OPERAND (op0, 0));
+	  result_type = type0;
+	}
       else if ((code1 == POINTER_TYPE || TYPE_PTRMEM_P (type1))
 	       && null_ptr_cst_p (op0))
-	result_type = type1;
+	{
+	  if (TREE_CODE (op1) == ADDR_EXPR 
+	      && DECL_P (TREE_OPERAND (op1, 0))
+	      && !DECL_WEAK (TREE_OPERAND (op1, 0)))
+	    warning (OPT_Walways_true, "the address of %qD will never be NULL",
+		     TREE_OPERAND (op1, 0));
+	  result_type = type1;
+	}
       else if (code0 == POINTER_TYPE && code1 == INTEGER_TYPE)
 	{
 	  result_type = type0;
Index: gcc/testsuite/g++.dg/warn/Walways-true-1.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Walways-true-1.C	(revision 0)
+++ gcc/testsuite/g++.dg/warn/Walways-true-1.C	(revision 0)
@@ -0,0 +1,30 @@
+// Test -Walways-true for testing an address against NULL.
+// Origin: Ian Lance Taylor <iant@google.com>
+
+// { dg-do compile}
+// { dg-options "-Walways-true" }
+
+extern int foo (int);
+
+int i;
+
+void
+bar ()
+{
+  if (foo)	// { dg-warning "always evaluate as" "correct warning" }
+    foo (0);
+  if (foo (1))
+    foo (1);
+  if (&i)	// { dg-warning "always evaluate as" "correct warning" }
+    foo (2);
+  if (i)
+    foo (3);
+  if (foo == 0)	// { dg-warning "never be NULL" "correct warning" }
+    foo (4);
+  if (foo (1) == 0)
+    foo (5);
+  if (&i == 0)	// { dg-warning "never be NULL" "correct warning" }
+    foo (6);
+  if (i == 0)
+    foo (7);
+}
Index: gcc/testsuite/g++.dg/warn/Walways-true-2.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Walways-true-2.C	(revision 0)
+++ gcc/testsuite/g++.dg/warn/Walways-true-2.C	(revision 0)
@@ -0,0 +1,33 @@
+// Make sure we don't assume that a weak symbol is always non-NULL.
+// This is just like Walways-true-1.C, except that it uses a weak
+// symbol.
+// Origin: Ian Lance Taylor <iant@google.com>
+
+// { dg-do compile}
+// { dg-options "-Walways-true" }
+// { dg-require-weak "" }
+
+extern int foo (int) __attribute__ ((weak));
+
+int i __attribute__ ((weak));
+
+void
+bar ()
+{
+  if (foo)
+    foo (0);
+  if (foo (1))
+    foo (1);
+  if (&i)
+    foo (2);
+  if (i)
+    foo (3);
+  if (foo == 0)
+    foo (4);
+  if (foo (1) == 0)
+    foo (5);
+  if (&i == 0)
+    foo (6);
+  if (i == 0)
+    foo (7);
+}