[C++ Patch/RFC] PR 64877

[C++ Patch/RFC] PR 64877

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 673 bytes --]

Hi,

Manuel did most of the work on this [5 Regression], caused by my fix for 
c++/43906, which extended a lot the functionality of -Waddress: a 
spurious warning is emitted with -Waddress for an expression internally 
generated in cp_build_binary_op. Manuel suggested that when safe we 
could completely avoid generating the relevant recursive calls to 
cp_build_binary_op. Alternately, for these internally generated 
expressions we could probably use tf_none instead of passing down 
complain. Note: I'm not 100% sure the ptrmemfunc_vbit_in_delta bits are 
correct, for sure avoid the spurious warning for those targets too.

Thanks,
Paolo.

///////////////////////////

[-- Attachment #2: patch_64877 --]
[-- Type: text/plain, Size: 4308 bytes --]

Index: cp/typeck.c
===================================================================
--- cp/typeck.c	(revision 220366)
+++ cp/typeck.c	(working copy)
@@ -4552,35 +4552,44 @@ cp_build_binary_op (location_t location,
 
 	         The reason for the `!op0.pfn' bit is that a NULL
 	         pointer-to-member is any member with a zero PFN and
-	         LSB of the DELTA field is 0.  */
+	         LSB of the DELTA field is 0.  Note: avoid generating the
+		 '|| (!op0.pfn && ...)' if !op0.pfn is known to be false.  */
 
-	      e1 = cp_build_binary_op (location, BIT_AND_EXPR,
-				       delta0, 
-				       integer_one_node,
-				       complain);
-	      e1 = cp_build_binary_op (location,
-				       EQ_EXPR, e1, integer_zero_node,
-				       complain);
-	      e2 = cp_build_binary_op (location, BIT_AND_EXPR,
-				       delta1,
-				       integer_one_node,
-				       complain);
-	      e2 = cp_build_binary_op (location,
-				       EQ_EXPR, e2, integer_zero_node,
-				       complain);
-	      e1 = cp_build_binary_op (location,
-				       TRUTH_ANDIF_EXPR, e2, e1,
-				       complain);
-	      e2 = cp_build_binary_op (location, EQ_EXPR,
-				       pfn0,
-				       build_zero_cst (TREE_TYPE (pfn0)),
-				       complain);
-	      e2 = cp_build_binary_op (location,
-				       TRUTH_ANDIF_EXPR, e2, e1, complain);
-	      e1 = cp_build_binary_op (location,
-				       EQ_EXPR, delta0, delta1, complain);
-	      e1 = cp_build_binary_op (location,
-				       TRUTH_ORIF_EXPR, e1, e2, complain);
+	      if (TREE_CODE (pfn0) != ADDR_EXPR
+		  || !decl_with_nonnull_addr_p (TREE_OPERAND (pfn0, 0)))
+		{
+		  e1 = cp_build_binary_op (location, BIT_AND_EXPR,
+					   delta0, 
+					   integer_one_node,
+					   complain);
+		  e1 = cp_build_binary_op (location,
+					   EQ_EXPR, e1, integer_zero_node,
+					   complain);
+		  e2 = cp_build_binary_op (location, BIT_AND_EXPR,
+					   delta1,
+					   integer_one_node,
+					   complain);
+		  e2 = cp_build_binary_op (location,
+					   EQ_EXPR, e2, integer_zero_node,
+					   complain);
+		  e1 = cp_build_binary_op (location,
+					   TRUTH_ANDIF_EXPR, e2, e1,
+					   complain);
+		  e2 = cp_build_binary_op (location, EQ_EXPR,
+					   pfn0,
+					   build_zero_cst (TREE_TYPE (pfn0)),
+					   complain);
+		  e2 = cp_build_binary_op (location,
+					   TRUTH_ANDIF_EXPR, e2, e1, complain);
+		  e1 = cp_build_binary_op (location,
+					   EQ_EXPR, delta0, delta1, complain);
+		  e1 = cp_build_binary_op (location,
+					   TRUTH_ORIF_EXPR, e1, e2, complain);
+
+		}
+	      else
+		e1 = cp_build_binary_op (location,
+					 EQ_EXPR, delta0, delta1, complain);
 	    }
 	  else
 	    {
@@ -4591,17 +4600,22 @@ cp_build_binary_op (location_t location,
 
 	         The reason for the `!op0.pfn' bit is that a NULL
 	         pointer-to-member is any member with a zero PFN; the
-	         DELTA field is unspecified.  */
+	         DELTA field is unspecified.  Note: avoid generating the
+		 '!op0.pfn ||' if !op0.pfn is known to be false.  */
  
     	      e1 = cp_build_binary_op (location,
 				       EQ_EXPR, delta0, delta1, complain);
-	      e2 = cp_build_binary_op (location,
-				       EQ_EXPR,
-		      		       pfn0,
-			   	       build_zero_cst (TREE_TYPE (pfn0)),
-				       complain);
-	      e1 = cp_build_binary_op (location,
-				       TRUTH_ORIF_EXPR, e1, e2, complain);
+	      if (TREE_CODE (pfn0) != ADDR_EXPR
+		  || !decl_with_nonnull_addr_p (TREE_OPERAND (pfn0, 0)))
+		{
+		  e2 = cp_build_binary_op (location,
+					   EQ_EXPR,
+					   pfn0,
+					   build_zero_cst (TREE_TYPE (pfn0)),
+					   complain);
+		  e1 = cp_build_binary_op (location,
+					   TRUTH_ORIF_EXPR, e1, e2, complain);
+		}
 	    }
 	  e2 = build2 (EQ_EXPR, boolean_type_node, pfn0, pfn1);
 	  e = cp_build_binary_op (location,
Index: testsuite/g++.dg/warn/Waddress-2.C
===================================================================
--- testsuite/g++.dg/warn/Waddress-2.C	(revision 0)
+++ testsuite/g++.dg/warn/Waddress-2.C	(working copy)
@@ -0,0 +1,24 @@
+// PR c++/64877
+// { dg-options "-Waddress" }
+
+template<class Derived>
+struct S
+{
+  void m() {
+  }
+
+  S()
+  {
+    if (&S<Derived>::Unwrap != &Derived::Unwrap)
+      m();
+  }
+
+  void Unwrap() {
+  }
+};
+
+struct T : public S<T>
+{
+};
+
+T t;

Re: [C++ Patch/RFC] PR 64877

[Jason Merrill]

On 02/03/2015 05:45 AM, Paolo Carlini wrote:
> +	      if (TREE_CODE (pfn0) != ADDR_EXPR
> +		  || !decl_with_nonnull_addr_p (TREE_OPERAND (pfn0, 0)))

I don't like duplicating the logic for when we might know the pfn is 
non-null; that seems fragile.  I'd rather go with the tf_none approach, 
or otherwise suppress the warning.

Jason

Re: [C++ Patch/RFC] PR 64877

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 1261 bytes --]

Hi,

On 02/03/2015 03:19 PM, Jason Merrill wrote:
> On 02/03/2015 05:45 AM, Paolo Carlini wrote:
>> +          if (TREE_CODE (pfn0) != ADDR_EXPR
>> +          || !decl_with_nonnull_addr_p (TREE_OPERAND (pfn0, 0)))
>
> I don't like duplicating the logic for when we might know the pfn is 
> non-null; that seems fragile.
I agree, seems fragile, on the other hand would be rather precise, IMHO. 
We could at least have a function wrapping the above and a comment in 
both places. Anyway...
>   I'd rather go with the tf_none approach, or otherwise suppress the 
> warning.
There are so many ways to fix this, and I'm a bit confused by the range 
of warning suppression mechanisms we have got at this point (eg, 
TREE_NO_WARNING, c_inhibit_evaluation_warnings, the new warning_sentinel 
in pt.c, tf_none, etc.. ;) But this case should be at least relatively 
safe because the expression is internally generated, right? Fiddling 
with tf_none, as mentioned by Manuel, should indeed work, but then, how 
far we would go with that? Only the 'e2 = cp_build_binary_op (location, 
EQ_EXPR, pfn0, ...)' lines or more of the calls for synthesized 
expressions? Would it be Ok for 5.0 to just use TREE_NO_WARNING like in 
the below?

Thanks,
Paolo.

/////////////////////


[-- Attachment #2: patch_64877_2 --]
[-- Type: text/plain, Size: 1761 bytes --]

Index: cp/typeck.c
===================================================================
--- cp/typeck.c	(revision 220371)
+++ cp/typeck.c	(working copy)
@@ -4415,7 +4415,8 @@ cp_build_binary_op (location_t location,
 	      && decl_with_nonnull_addr_p (TREE_OPERAND (op0, 0)))
 	    {
 	      if ((complain & tf_warning)
-		  && c_inhibit_evaluation_warnings == 0)
+		  && c_inhibit_evaluation_warnings == 0
+		  && !TREE_NO_WARNING (op0))
 		warning (OPT_Waddress, "the address of %qD will never be NULL",
 			 TREE_OPERAND (op0, 0));
 	    }
@@ -4436,7 +4437,8 @@ cp_build_binary_op (location_t location,
 	      && decl_with_nonnull_addr_p (TREE_OPERAND (op1, 0)))
 	    {
 	      if ((complain & tf_warning)
-		  && c_inhibit_evaluation_warnings == 0)
+		  && c_inhibit_evaluation_warnings == 0
+		  && !TREE_NO_WARNING (op1))
 		warning (OPT_Waddress, "the address of %qD will never be NULL",
 			 TREE_OPERAND (op1, 0));
 	    }
@@ -4537,6 +4539,8 @@ cp_build_binary_op (location_t location,
 	    op1 = save_expr (op1);
 
 	  pfn0 = pfn_from_ptrmemfunc (op0);
+	  /* Avoid -Waddress warnings (c++/64877).  */
+	  TREE_NO_WARNING (pfn0) = 1;
 	  pfn1 = pfn_from_ptrmemfunc (op1);
 	  delta0 = delta_from_ptrmemfunc (op0);
 	  delta1 = delta_from_ptrmemfunc (op1);
Index: testsuite/g++.dg/warn/Waddress-2.C
===================================================================
--- testsuite/g++.dg/warn/Waddress-2.C	(revision 0)
+++ testsuite/g++.dg/warn/Waddress-2.C	(working copy)
@@ -0,0 +1,24 @@
+// PR c++/64877
+// { dg-options "-Waddress" }
+
+template<class Derived>
+struct S
+{
+  void m() {
+  }
+
+  S()
+  {
+    if (&S<Derived>::Unwrap != &Derived::Unwrap)
+      m();
+  }
+
+  void Unwrap() {
+  }
+};
+
+struct T : public S<T>
+{
+};
+
+T t;

Re: [C++ Patch/RFC] PR 64877

[Jason Merrill]

On 02/03/2015 11:14 AM, Paolo Carlini wrote:
> +	  /* Avoid -Waddress warnings (c++/64877).  */
> +	  TREE_NO_WARNING (pfn0) = 1;

I'd check for ADDR_EXPR before doing this; OK with that change.

Jason