This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[C++ PATCH] Fix PRs 33112 and 33185


This patch fixes PRs 33112 (a P3) and 33185 (a P1), both of which are
canonical-type failures due to the mis-handling of canonical types for
arrays. My previous patch, which I thought had fixed these problems,
was bogus:

  http://gcc.gnu.org/ml/gcc-patches/2007-09/msg00910.html

The attached patch fixes the problem for real. The fundamental issue
is that cplus_array_compare, which is used to match elements in a hash
table of ARRAY_TYPE nodes, was comparing via type equality, not by
pointer equality. So, we were combining ARRAY_TYPE nodes in a way that
was bound to cause problems later on. Moreover, we weren't putting
things into said hash table immediately, and in some cases the hash
table could get reallocated after we did an insertion but before we
committed the inserted value. Between these two errors, we ended up
with the hard-to-reproduce problems reported in these two PRs, which
depend on various pointer values and when the hash tables were
resized.

Tested i686-pc-linux-gnu; no regressions. Okay for mainline?

  - Doug

2007-09-19  Douglas Gregor  <doug.gregor@gmail.com>

	PR c++/33112
	PR c++/33185	
	* tree.c (cplus_array_compare): Compare pointers, not types.
	(build_cplus_array_type_1): Store new array type into the hash
	table before building the canonical type; build the canonical type
	correctly.
	(cp_build_qualified_type_real): Put all of the array types with
	cv-qualified element types into the C++ array hash table, built as
	variants of the unqualified versions.
Index: tree.c
===================================================================
--- tree.c	(revision 128587)
+++ tree.c	(working copy)
@@ -512,18 +512,12 @@ cplus_array_compare (const void * k1, co
   const_tree const t1 = (const_tree) k1;
   const cplus_array_info *const t2 = (const cplus_array_info*) k2;
 
-  if (!comptypes (TREE_TYPE (t1), t2->type, COMPARE_STRUCTURAL))
-    return 0;
-
-  if (!TYPE_DOMAIN (t1))
-    return !t2->domain;
-
-  if (!t2->domain)
-    return 0;
-
-  return comptypes (TYPE_DOMAIN (t1), t2->domain, COMPARE_STRUCTURAL);
+  return (TREE_TYPE (t1) == t2->type && TYPE_DOMAIN (t1) == t2->domain);
 }
 
+/* Hash table containing all of the C++ array types, including
+   dependent array types and array types whose element type is
+   cv-qualified.  */
 static GTY ((param_is (union tree_node))) htab_t cplus_array_htab;
 
 
@@ -542,7 +536,7 @@ build_cplus_array_type_1 (tree elt_type,
       void **e;
       cplus_array_info cai;
       hashval_t hash;
-      
+
       if (cplus_array_htab == NULL)
 	cplus_array_htab = htab_create_ggc (61, &cplus_array_hash,
 					    &cplus_array_compare, NULL);
@@ -554,31 +548,32 @@ build_cplus_array_type_1 (tree elt_type,
 
       e = htab_find_slot_with_hash (cplus_array_htab, &cai, hash, INSERT); 
       if (*e)
-	/* We have found the type: we're done. */
+	/* We have found the type: we're done.  */
 	return (tree) *e;
       else
 	{
-	  /* Build a new array type. */
+	  /* Build a new array type.  */
 	  t = make_node (ARRAY_TYPE);
 	  TREE_TYPE (t) = elt_type;
 	  TYPE_DOMAIN (t) = index_type;
 
-	  /* Complete building the array type. */
+	  /* Store it in the hash table. */
+	  *e = t;
+
+	  /* Set the canonical type for this new node.  */
 	  if (TYPE_STRUCTURAL_EQUALITY_P (elt_type)
 	      || (index_type && TYPE_STRUCTURAL_EQUALITY_P (index_type)))
 	    SET_TYPE_STRUCTURAL_EQUALITY (t);
 	  else if (TYPE_CANONICAL (elt_type) != elt_type
 		   || (index_type 
-		       && TYPE_CANONICAL (index_type) != index_type))
-	    TYPE_CANONICAL (t) 
-	      = TYPE_CANONICAL 
-	          (build_cplus_array_type_1 (TYPE_CANONICAL (elt_type),
-					     index_type? 
-					       TYPE_CANONICAL (index_type)
-					       : index_type));
-
-	  /* Store it in the hash table. */
-	  *e = t;
+		       && TYPE_CANONICAL (index_type) != index_type)) {
+	    TYPE_CANONICAL (t)
+		= build_cplus_array_type 
+		   (TYPE_CANONICAL (elt_type),
+		    index_type? TYPE_CANONICAL (index_type) : index_type);
+	  }
+	  else
+	    TYPE_CANONICAL (t) = t;
 	}
     }
   else
@@ -713,59 +708,44 @@ cp_build_qualified_type_real (tree type,
 
       if (!t)
 	{
-	  tree domain = TYPE_DOMAIN (type);
+	  tree index_type = TYPE_DOMAIN (type);
+	  void **e;
+	  cplus_array_info cai;
+	  hashval_t hash;
+
+	  if (cplus_array_htab == NULL)
+	    cplus_array_htab = htab_create_ggc (61, &cplus_array_hash,
+						&cplus_array_compare, 
+						NULL);
+
+	  hash = (htab_hash_pointer (element_type)
+		  ^ htab_hash_pointer (index_type));
+	  cai.type = element_type;
+	  cai.domain = index_type;
+	  
+	  e = htab_find_slot_with_hash (cplus_array_htab, &cai, hash, INSERT);
+	  if (*e)
+	    /* We have found the type: we're done. */
+	    return (tree) *e;
 
-	  /* Make a new array type, just like the old one, but with the
-	     appropriately qualified element type.  */
+	  /* Build a new array type and add it into the table.  */
 	  t = build_variant_type_copy (type);
 	  TREE_TYPE (t) = element_type;
+	  *e = t;
 
-	  /* This is a new type. */
-	  TYPE_CANONICAL (t) = t;
-
-	  if (dependent_type_p (element_type)
-	      || (domain
-		  && value_dependent_expression_p (TYPE_MAX_VALUE (domain))))
-	    {
-	      /* The new dependent array type we just created might be
-		 equivalent to an existing dependent array type, so we
-		 need to keep track of this new array type with a
-		 lookup into CPLUS_ARRAY_HTAB. Note that we cannot
-		 directly call build_cplus_array_type (that would
-		 recurse) or build_cplus_array_type_1 (that would lose
-		 attributes). */
-	      void **e;
-	      cplus_array_info cai;
-	      hashval_t hash;
-
-	      if (cplus_array_htab == NULL)
-		cplus_array_htab = htab_create_ggc (61, &cplus_array_hash,
-						    &cplus_array_compare, 
-						    NULL);
-	  
-	      hash = (htab_hash_pointer (element_type)
-		      ^ htab_hash_pointer (domain));
-	      cai.type = element_type;
-	      cai.domain = domain;
-	  
-	      e = htab_find_slot_with_hash (cplus_array_htab, &cai, hash, 
-					    INSERT); 
-	      if (! *e)
-		/* Save this new type. */
-		*e = t;
-	    }
-
-	  if (TYPE_STRUCTURAL_EQUALITY_P (TREE_TYPE (t))
-		   || (TYPE_DOMAIN (t)
-		       && TYPE_STRUCTURAL_EQUALITY_P (TYPE_DOMAIN (t))))
+	  /* Set the canonical type for this new node.  */
+	  if (TYPE_STRUCTURAL_EQUALITY_P (element_type)
+	      || (index_type && TYPE_STRUCTURAL_EQUALITY_P (index_type)))
 	    SET_TYPE_STRUCTURAL_EQUALITY (t);
+	  else if (TYPE_CANONICAL (element_type) != element_type
+		   || (index_type 
+		       && TYPE_CANONICAL (index_type) != index_type))
+	    TYPE_CANONICAL (t)
+	      = build_cplus_array_type
+	         (TYPE_CANONICAL (element_type),
+		  index_type? TYPE_CANONICAL (index_type) : index_type);
 	  else
-	    TYPE_CANONICAL (t) 
-	      = TYPE_CANONICAL 
-	          (build_array_type (TYPE_CANONICAL (TREE_TYPE (t)),
-				     TYPE_DOMAIN (t)? 
-				       TYPE_CANONICAL (TYPE_DOMAIN(t))
-				       : TYPE_DOMAIN (t)));
+	    TYPE_CANONICAL (t) = t;
 	}
 
       /* Even if we already had this variant, we update

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]