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

[Ian Lance Taylor <iant at google dot com>]

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);
+}