[PATCH] Further -Wnonnull-compare fixes (PR c++/69922)

[Jakub Jelinek <jakub at redhat dot com>]

Hi!

This patch adds TREE_NO_WARNING to two places that didn't have it before
and avoids folding those conditions at those spots (partly to match
C++ delayed folding intent, partly to avoid duplicating TREE_NO_WARNING
propagation).

The more controversial stuff is that we need to preserve TREE_NO_WARNING on
artificial comparisons with NULL, but some fold-const.c optimizations break
that, e.g. fold_binary_op_with_conditional_arg which changes
(cond ? a : b) != NULL to
(cond ? a != NULL : b != NULL)
but can go arbitrarily deep, we'd want to mark those NULL comparisons added
because of that as TREE_NO_WARNING, but this is called from
fold_binary_loc, which doesn't know it is for a TREE_NO_WARNING comparison.
Richi suggested on IRC just punt on folding in that case, which is what the
patch below implements (basically, it will try to fold the comparison,
if we get error_mark_node or if the comparison folds to constant, we are
fine, if we get a comparison, mark it TREE_NO_WARNING, otherwise don't
fold the comparison itself (hopefully GIMPLE optimizations will optimize
it), but still make sure that at least the operands are folded).
Perhaps we can pattern recognize some important cases, or could invoke
e.g. fold_binary_op_with_conditional_arg with some extra argument from
cp_fold directly, whatever.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2016-02-24  Jakub Jelinek  <jakub@redhat.com>

	PR c++/69922
	* class.c (build_base_path): Set TREE_NO_WARNING on the null_test.
	Avoid folding it.
	* init.c (build_vec_delete_1, build_delete): Don't fold the non-NULL
	tests.
	* cp-gimplify.c (cp_fold): For TREE_NO_WARNING comparisons with NULL,
	unless they are folded into INTEGER_CST, error_mark_node or some
	comparison with NULL, avoid folding them and use either the original
	comparison or non-folded comparison of folded arguments.
	* cp-ubsan.c (cp_ubsan_instrument_vptr): Set TREE_NO_WARNING on the
	comparison, don't fold the comparison right away.

	* g++.dg/warn/Wnonnull-compare-6.C: New test.
	* g++.dg/warn/Wnonnull-compare-7.C: New test.
	* g++.dg/ubsan/pr69922.C: New test.

--- gcc/cp/class.c.jj	2016-02-04 12:01:38.000000000 +0100
+++ gcc/cp/class.c	2016-02-24 11:21:59.490438431 +0100
@@ -392,8 +392,11 @@ build_base_path (enum tree_code code,
   if (null_test)
     {
       tree zero = cp_convert (TREE_TYPE (expr), nullptr_node, complain);
-      null_test = fold_build2_loc (input_location, NE_EXPR, boolean_type_node,
-			       expr, zero);
+      null_test = build2_loc (input_location, NE_EXPR, boolean_type_node,
+			      expr, zero);
+      /* This is a compiler generated comparison, don't emit
+	 e.g. -Wnonnull-compare warning for it.  */
+      TREE_NO_WARNING (null_test) = 1;
     }
 
   /* If this is a simple base reference, express it as a COMPONENT_REF.  */
--- gcc/cp/init.c.jj	2016-02-19 17:02:14.000000000 +0100
+++ gcc/cp/init.c	2016-02-24 12:12:49.889583371 +0100
@@ -3678,15 +3678,13 @@ build_vec_delete_1 (tree base, tree maxi
     body = integer_zero_node;
 
   /* Outermost wrapper: If pointer is null, punt.  */
-  tree cond
-    = fold_build2_loc (input_location, NE_EXPR, boolean_type_node, base,
-		       fold_convert (TREE_TYPE (base), nullptr_node));
+  tree cond = build2_loc (input_location, NE_EXPR, boolean_type_node, base,
+			  fold_convert (TREE_TYPE (base), nullptr_node));
   /* This is a compiler generated comparison, don't emit
      e.g. -Wnonnull-compare warning for it.  */
-  if (TREE_CODE (cond) == NE_EXPR)
-    TREE_NO_WARNING (cond) = 1;
-  body = fold_build3_loc (input_location, COND_EXPR, void_type_node,
-			  cond, body, integer_zero_node);
+  TREE_NO_WARNING (cond) = 1;
+  body = build3_loc (input_location, COND_EXPR, void_type_node,
+		     cond, body, integer_zero_node);
   body = build1 (NOP_EXPR, void_type_node, body);
 
   if (controller)
@@ -4523,9 +4521,8 @@ build_delete (tree otype, tree addr, spe
 	{
 	  /* Handle deleting a null pointer.  */
 	  warning_sentinel s (warn_address);
-	  ifexp = fold (cp_build_binary_op (input_location,
-					    NE_EXPR, addr, nullptr_node,
-					    complain));
+	  ifexp = cp_build_binary_op (input_location, NE_EXPR, addr,
+				      nullptr_node, complain);
 	  if (ifexp == error_mark_node)
 	    return error_mark_node;
 	  /* This is a compiler generated comparison, don't emit
--- gcc/cp/cp-gimplify.c.jj	2016-02-24 12:06:15.000000000 +0100
+++ gcc/cp/cp-gimplify.c	2016-02-24 14:12:24.063438023 +0100
@@ -2069,8 +2069,28 @@ cp_fold (tree x)
 	x = fold (x);
 
       if (TREE_NO_WARNING (org_x)
-	  && TREE_CODE (x) == TREE_CODE (org_x))
-	TREE_NO_WARNING (x) = 1;
+	  && warn_nonnull_compare
+	  && COMPARISON_CLASS_P (org_x)
+	  && integer_zerop (tree_strip_nop_conversions (TREE_OPERAND (org_x,
+								      1))))
+	{
+	  if (x == error_mark_node || TREE_CODE (x) == INTEGER_CST)
+	    ;
+	  else if (COMPARISON_CLASS_P (x)
+		   && integer_zerop (tree_strip_nop_conversions
+						(TREE_OPERAND (x, 1))))
+	    TREE_NO_WARNING (x) = 1;
+	  /* Otherwise give up on optimizing these, let GIMPLE folders
+	     optimize those later on.  */
+	  else if (op0 != TREE_OPERAND (org_x, 0)
+		   || op1 != TREE_OPERAND (org_x, 1))
+	    {
+	      x = build2_loc (loc, code, TREE_TYPE (org_x), op0, op1);
+	      TREE_NO_WARNING (x) = 1;
+	    }
+	  else
+	    x = org_x;
+	}
       break;
 
     case VEC_COND_EXPR:
--- gcc/cp/cp-ubsan.c.jj	2016-01-04 14:55:57.000000000 +0100
+++ gcc/cp/cp-ubsan.c	2016-02-24 12:57:13.335137369 +0100
@@ -70,10 +70,15 @@ cp_ubsan_instrument_vptr (location_t loc
   vptr = fold_convert_loc (loc, pointer_sized_int_node, vptr);
   vptr = fold_convert_loc (loc, uint64_type_node, vptr);
   if (ckind == UBSAN_DOWNCAST_POINTER)
-    vptr = fold_build3 (COND_EXPR, uint64_type_node,
-			fold_build2 (NE_EXPR, boolean_type_node, op,
-				     build_zero_cst (TREE_TYPE (op))),
-			vptr, build_int_cst (uint64_type_node, 0));
+    {
+      tree cond = build2_loc (loc, NE_EXPR, boolean_type_node, op,
+			      build_zero_cst (TREE_TYPE (op)));
+      /* This is a compiler generated comparison, don't emit
+	 e.g. -Wnonnull-compare warning for it.  */
+      TREE_NO_WARNING (cond) = 1;
+      vptr = build3_loc (loc, COND_EXPR, uint64_type_node, cond,
+			 vptr, build_int_cst (uint64_type_node, 0));
+    }
   tree ti_decl = get_tinfo_decl (type);
   mark_used (ti_decl);
   tree ptype = build_pointer_type (type);
--- gcc/testsuite/g++.dg/warn/Wnonnull-compare-6.C.jj	2016-02-24 12:36:40.979450607 +0100
+++ gcc/testsuite/g++.dg/warn/Wnonnull-compare-6.C	2016-02-24 12:39:50.922779766 +0100
@@ -0,0 +1,13 @@
+// PR c++/69922
+// { dg-do compile }
+// { dg-options "-Wnonnull-compare" }
+
+struct T { virtual ~T (); };
+struct S { virtual ~S (); T *f (bool); };
+struct U : S, T {};
+
+T *
+S::f (bool b)
+{
+  return b ? static_cast<U *> (this) : (U *) 0;	// { dg-bogus "nonnull argument" }
+}
--- gcc/testsuite/g++.dg/warn/Wnonnull-compare-7.C.jj	2016-02-24 12:36:44.313403726 +0100
+++ gcc/testsuite/g++.dg/warn/Wnonnull-compare-7.C	2016-02-24 12:39:09.968355579 +0100
@@ -0,0 +1,19 @@
+// PR c++/69922
+// { dg-do compile }
+// { dg-options "-Wnonnull-compare" }
+
+struct S { virtual ~S (); };
+struct T { virtual ~T (); };
+bool b, c;
+S *p;
+T *q, *r;
+
+S::~S ()
+{
+  delete (b ? this : p);		// { dg-bogus "nonnull argument" }
+}
+
+T::~T ()
+{
+  delete (b ? (c ? this : q) : r);	// { dg-bogus "nonnull argument" }
+}
--- gcc/testsuite/g++.dg/ubsan/pr69922.C.jj	2016-02-24 12:51:21.730075909 +0100
+++ gcc/testsuite/g++.dg/ubsan/pr69922.C	2016-02-24 13:00:23.973474407 +0100
@@ -0,0 +1,18 @@
+// PR c++/69922
+// { dg-do compile }
+// { dg-options "-fsanitize=vptr -Wnonnull-compare" }
+
+struct S { virtual ~S (); };
+struct T : S { T *bar (); T *baz (); T *q; bool b; };
+
+T *
+T::bar ()
+{
+  return static_cast<T*>(reinterpret_cast<S*>(this));	// { dg-bogus "nonnull argument" }
+}
+
+T *
+T::baz ()
+{
+  return static_cast<T*>(reinterpret_cast<S*>(b ? this : q));	// { dg-bogus "nonnull argument" }
+}

	Jakub