diff mbox

[PR,debug/45130] fix MEM_REF -fcompare-debug regression

Message ID ory6bgr6yw.fsf@livre.localdomain
State New
Headers show

Commit Message

Alexandre Oliva Sept. 5, 2010, 7:21 a.m. UTC 
MEM_REFs introduced some -fcompare-debug regressions.  There are two
main issues, both related with caching of MEM attributes.

One is that top-level qualifiers in the types of the two MEM_REF
operands, although irrelevant, are regarded as conversions by the tree
dumpers.  When e.g. copyprop replaces a non-const-qualified variable to
a MEM_REF that was built out of a const-qualified temporary, we end up
with a type mismatch, dumped as an explicit type conversion in the
MEM_REF operand.

This, compounded with the second issue, that types are disregarded by
the hash and compare functions used to build the MEM attribute cache,
means that the presence of debug expressions with or without the
mismatches may cause a different-typed expression to be cached.  Then,
non-debug expressions that share the same cached MEM attrs may be dumped
one way or another, failing -fcompare-debug.

As it turns out, disregarding top-level qualifiers is not enough to fix
the problem, because other unrelated types happen to share the same
cache entry, particularly when expressions that dereference NULL are
involved.

To avoid this inappropriate sharing, I've added the OEP_SAME_TYPE flag
to operand_equal_p, as well as to iterative_hash_expr.  This would
render useless the first hunk, to disregard top-level qualifiers in
MEM_REF dumping, but since such mismatches are just noise anyway, I
decided to leave it in.

This patch enabled bootstrap with -fcompare-debug to succeed for all of
stage3, including target libraries, with otherwise standard flags, on
x86_64-linux-gnu and i686-pc-linux-gnu, and for stage3 similar bootstrap
to succeed with -O1 and -O3 all the way to bootstrap-compare, on both
platforms as well.  Regression testing also passed on both platforms.

Ok to install?

Comments

Richard Biener Sept. 5, 2010, 8:49 a.m. UTC | #1 
On Sun, Sep 5, 2010 at 9:21 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> MEM_REFs introduced some -fcompare-debug regressions.  There are two
> main issues, both related with caching of MEM attributes.
>
> One is that top-level qualifiers in the types of the two MEM_REF
> operands, although irrelevant, are regarded as conversions by the tree
> dumpers.  When e.g. copyprop replaces a non-const-qualified variable to
> a MEM_REF that was built out of a const-qualified temporary, we end up
> with a type mismatch, dumped as an explicit type conversion in the
> MEM_REF operand.
>
> This, compounded with the second issue, that types are disregarded by
> the hash and compare functions used to build the MEM attribute cache,
> means that the presence of debug expressions with or without the
> mismatches may cause a different-typed expression to be cached.  Then,
> non-debug expressions that share the same cached MEM attrs may be dumped
> one way or another, failing -fcompare-debug.
>
> As it turns out, disregarding top-level qualifiers is not enough to fix
> the problem, because other unrelated types happen to share the same
> cache entry, particularly when expressions that dereference NULL are
> involved.
>
> To avoid this inappropriate sharing, I've added the OEP_SAME_TYPE flag
> to operand_equal_p, as well as to iterative_hash_expr.  This would
> render useless the first hunk, to disregard top-level qualifiers in
> MEM_REF dumping, but since such mismatches are just noise anyway, I
> decided to leave it in.
>
> This patch enabled bootstrap with -fcompare-debug to succeed for all of
> stage3, including target libraries, with otherwise standard flags, on
> x86_64-linux-gnu and i686-pc-linux-gnu, and for stage3 similar bootstrap
> to succeed with -O1 and -O3 all the way to bootstrap-compare, on both
> platforms as well.  Regression testing also passed on both platforms.
>
> Ok to install?

+   If OEP_SAME_TYPE is set, then mismatched types */

there seems to be missing sth.

You want to change how types of constants are treated, right?  But you
now apply the type check everywhere.

The issue seems also to be a correctness one to me, as we'd happily
share MEM[(char *)ptr] and MEM[(int *)ptr] which would break
TBAA.

Thus, simply add a type equality check for operand 1 in operand_equal_p here:

        case MEM_REF:
          /* Require equal access sizes.  We can have incomplete types
             for array references of variable-sized arrays from the
             Fortran frontent though.  */
          return ((TYPE_SIZE (TREE_TYPE (arg0)) == TYPE_SIZE (TREE_TYPE (arg1))
                   || (TYPE_SIZE (TREE_TYPE (arg0))
                       && TYPE_SIZE (TREE_TYPE (arg1))
                       && operand_equal_p (TYPE_SIZE (TREE_TYPE (arg0)),
                                           TYPE_SIZE (TREE_TYPE
(arg1)), flags)))
                  && OP_SAME (0) && OP_SAME (1));

Richard.
diff mbox

Patch

for gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR debug/45419
	PR debug/45408
	* tree-pretty-print.c (dump_generic_node): Disregard top-level
	qualifiers in otherwise equal types.
	* emit-rtl.c (mem_attrs_htab_hash): Hash with OEP_SAME_TYPE.
	(mem_attrs_htab_eq): Compare with OEP_SAME_TYPE.
	* fold-const.c (operand_equal_p): Handle OEP_SAME_TYPE.
	* tree.c (iterative_hash_expr_1): Support OEP_SAME_TYPE.  Renamed
	from...
	(iterative_hash_expr): This.  New wrapper.
	(iterative_hash_expr_flags): New.
	* tree.h (OEP_SAME_TYPE): New enumerator in operand_equal_flag.
	(iterative_hash_expr_flags): Declare.

Index: gcc/tree-pretty-print.c
===================================================================
--- gcc/tree-pretty-print.c.orig	2010-09-04 05:55:51.000000000 -0300
+++ gcc/tree-pretty-print.c	2010-09-04 05:56:06.000000000 -0300
@@ -807,8 +807,6 @@  dump_generic_node (pretty_printer *buffe
 		== TYPE_MODE (TREE_TYPE (TREE_OPERAND (node, 1))))
 	    && (TYPE_REF_CAN_ALIAS_ALL (TREE_TYPE (TREE_OPERAND (node, 0)))
 		== TYPE_REF_CAN_ALIAS_ALL (TREE_TYPE (TREE_OPERAND (node, 1))))
-	    && (TYPE_QUALS (TREE_TYPE (TREE_OPERAND (node, 0)))
-		== TYPE_QUALS (TREE_TYPE (TREE_OPERAND (node, 1))))
 	    /* Same value types ignoring qualifiers.  */
 	    && (TYPE_MAIN_VARIANT (TREE_TYPE (node))
 		== TYPE_MAIN_VARIANT
Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c.orig	2010-09-04 05:54:18.881551941 -0300
+++ gcc/emit-rtl.c	2010-09-04 05:56:07.000000000 -0300
@@ -260,7 +260,7 @@  mem_attrs_htab_hash (const void *x)
 	  ^ (p->addrspace * 4000)
 	  ^ ((p->offset ? INTVAL (p->offset) : 0) * 50000)
 	  ^ ((p->size ? INTVAL (p->size) : 0) * 2500000)
-	  ^ (size_t) iterative_hash_expr (p->expr, 0));
+	  ^ (size_t) iterative_hash_expr_flags (p->expr, 0, OEP_SAME_TYPE));
 }
 
 /* Returns nonzero if the value represented by X (which is really a
@@ -278,7 +278,7 @@  mem_attrs_htab_eq (const void *x, const 
 	  && p->addrspace == q->addrspace
 	  && (p->expr == q->expr
 	      || (p->expr != NULL_TREE && q->expr != NULL_TREE
-		  && operand_equal_p (p->expr, q->expr, 0))));
+		  && operand_equal_p (p->expr, q->expr, OEP_SAME_TYPE))));
 }
 
 /* Allocate a new mem_attrs structure and insert it into the hash table if
Index: gcc/fold-const.c
===================================================================
--- gcc/fold-const.c.orig	2010-09-04 05:54:18.000000000 -0300
+++ gcc/fold-const.c	2010-09-04 05:56:07.000000000 -0300
@@ -2388,7 +2388,9 @@  combine_comparisons (location_t loc,
 
    If OEP_PURE_SAME is set, then pure functions with identical arguments
    are considered the same.  It is used when the caller has other ways
-   to ensure that global memory is unchanged in between.  */
+   to ensure that global memory is unchanged in between.
+
+   If OEP_SAME_TYPE is set, then mismatched types */
 
 int
 operand_equal_p (const_tree arg0, const_tree arg1, unsigned int flags)
@@ -2404,6 +2406,9 @@  operand_equal_p (const_tree arg0, const_
   if (!TREE_TYPE (arg0) || !TREE_TYPE (arg1))
     return 0;
 
+  if ((flags & OEP_SAME_TYPE) && TREE_TYPE (arg0) != TREE_TYPE (arg1))
+    return 0;
+
   /* Check equality of integer constants before bailing out due to
      precision differences.  */
   if (TREE_CODE (arg0) == INTEGER_CST && TREE_CODE (arg1) == INTEGER_CST)
@@ -2527,7 +2532,7 @@  operand_equal_p (const_tree arg0, const_
 
       case ADDR_EXPR:
 	return operand_equal_p (TREE_OPERAND (arg0, 0), TREE_OPERAND (arg1, 0),
-				0);
+				(flags & OEP_SAME_TYPE));
       default:
 	break;
       }
Index: gcc/tree.c
===================================================================
--- gcc/tree.c.orig	2010-09-04 05:55:51.881551941 -0300
+++ gcc/tree.c	2010-09-04 12:20:57.000000000 -0300
@@ -6669,11 +6669,12 @@  commutative_ternary_tree_code (enum tree
 /* Generate a hash value for an expression.  This can be used iteratively
    by passing a previous result as the VAL argument.
 
-   This function is intended to produce the same hash for expressions which
-   would compare equal using operand_equal_p.  */
+   This function is intended to produce the same hash for expressions
+   which would compare equal using operand_equal_p, called with the
+   same FLAGS.  */
 
-hashval_t
-iterative_hash_expr (const_tree t, hashval_t val)
+static hashval_t
+iterative_hash_expr_1 (const_tree t, hashval_t val, unsigned int flags)
 {
   int i;
   enum tree_code code;
@@ -6682,6 +6683,9 @@  iterative_hash_expr (const_tree t, hashv
   if (t == NULL_TREE)
     return iterative_hash_hashval_t (0, val);
 
+  if (TREE_TYPE (t) && (flags & OEP_SAME_TYPE))
+    val = iterative_hash_hashval_t (TYPE_UID (TREE_TYPE (t)), val);
+
   code = TREE_CODE (t);
 
   switch (code)
@@ -6707,10 +6711,10 @@  iterative_hash_expr (const_tree t, hashv
       return iterative_hash (TREE_STRING_POINTER (t),
 			     TREE_STRING_LENGTH (t), val);
     case COMPLEX_CST:
-      val = iterative_hash_expr (TREE_REALPART (t), val);
-      return iterative_hash_expr (TREE_IMAGPART (t), val);
+      val = iterative_hash_expr_1 (TREE_REALPART (t), val, flags);
+      return iterative_hash_expr_1 (TREE_IMAGPART (t), val, flags);
     case VECTOR_CST:
-      return iterative_hash_expr (TREE_VECTOR_CST_ELTS (t), val);
+      return iterative_hash_expr_1 (TREE_VECTOR_CST_ELTS (t), val, flags);
     case SSA_NAME:
       /* We can just compare by pointer.  */
       return iterative_hash_host_wide_int (SSA_NAME_VERSION (t), val);
@@ -6721,7 +6725,7 @@  iterative_hash_expr (const_tree t, hashv
       /* A list of expressions, for a CALL_EXPR or as the elements of a
 	 VECTOR_CST.  */
       for (; t; t = TREE_CHAIN (t))
-	val = iterative_hash_expr (TREE_VALUE (t), val);
+	val = iterative_hash_expr_1 (TREE_VALUE (t), val, flags);
       return val;
     case CONSTRUCTOR:
       {
@@ -6729,8 +6733,8 @@  iterative_hash_expr (const_tree t, hashv
 	tree field, value;
 	FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (t), idx, field, value)
 	  {
-	    val = iterative_hash_expr (field, val);
-	    val = iterative_hash_expr (value, val);
+	    val = iterative_hash_expr_1 (field, val, flags);
+	    val = iterative_hash_expr_1 (value, val, flags);
 	  }
 	return val;
       }
@@ -6769,7 +6773,7 @@  iterative_hash_expr (const_tree t, hashv
 	    {
 	      /* Make sure to include signness in the hash computation.  */
 	      val += TYPE_UNSIGNED (TREE_TYPE (t));
-	      val = iterative_hash_expr (TREE_OPERAND (t, 0), val);
+	      val = iterative_hash_expr_1 (TREE_OPERAND (t, 0), val, flags);
 	    }
 
 	  else if (commutative_tree_code (code))
@@ -6778,8 +6782,10 @@  iterative_hash_expr (const_tree t, hashv
 		 however it appears.  We do this by first hashing both operands
 		 and then rehashing based on the order of their independent
 		 hashes.  */
-	      hashval_t one = iterative_hash_expr (TREE_OPERAND (t, 0), 0);
-	      hashval_t two = iterative_hash_expr (TREE_OPERAND (t, 1), 0);
+	      hashval_t one = iterative_hash_expr_1 (TREE_OPERAND (t, 0),
+						     0, flags);
+	      hashval_t two = iterative_hash_expr_1 (TREE_OPERAND (t, 1),
+						     0, flags);
 	      hashval_t t;
 
 	      if (one > two)
@@ -6790,13 +6796,30 @@  iterative_hash_expr (const_tree t, hashv
 	    }
 	  else
 	    for (i = TREE_OPERAND_LENGTH (t) - 1; i >= 0; --i)
-	      val = iterative_hash_expr (TREE_OPERAND (t, i), val);
+	      val = iterative_hash_expr_1 (TREE_OPERAND (t, i), val, flags);
 	}
       return val;
       break;
     }
 }
 
+/* Wrapper for iterative_hash_expr_1, expected to be
+   inlined/specialized with flags == 0.  */
+
+hashval_t
+iterative_hash_expr (const_tree t, hashval_t val)
+{
+  return iterative_hash_expr_1 (t, val, 0);
+}
+
+/* Publicly-visible wrapper for iterative_hash_expr_1.  */
+
+hashval_t
+iterative_hash_expr_flags (const_tree t, hashval_t val, unsigned int flags)
+{
+  return iterative_hash_expr_1 (t, val, flags);
+}
+
 /* Generate a hash value for a pair of expressions.  This can be used
    iteratively by passing a previous result as the VAL argument.
 
Index: gcc/tree.h
===================================================================
--- gcc/tree.h.orig	2010-09-04 05:55:51.868554928 -0300
+++ gcc/tree.h	2010-09-04 05:56:07.000000000 -0300
@@ -4952,7 +4952,8 @@  extern bool fold_deferring_overflow_warn
 enum operand_equal_flag
 {
   OEP_ONLY_CONST = 1,
-  OEP_PURE_SAME = 2
+  OEP_PURE_SAME = 2,
+  OEP_SAME_TYPE = 4
 };
 
 extern int operand_equal_p (const_tree, const_tree, unsigned int);
@@ -5085,6 +5086,8 @@  extern int tree_log2 (const_tree);
 extern int tree_floor_log2 (const_tree);
 extern int simple_cst_equal (const_tree, const_tree);
 extern hashval_t iterative_hash_expr (const_tree, hashval_t);
+extern hashval_t iterative_hash_expr_flags (const_tree, hashval_t,
+					    unsigned int);
 extern hashval_t iterative_hash_exprs_commutative (const_tree,
                                                    const_tree, hashval_t);
 extern hashval_t iterative_hash_host_wide_int (HOST_WIDE_INT, hashval_t);