diff mbox

Fix PR62077

Message ID alpine.LSU.2.11.1408131619140.20733@zhemvz.fhfr.qr
State New
Headers show

Commit Message

Richard Biener Aug. 13, 2014, 2:28 p.m. UTC 
On Wed, 13 Aug 2014, Richard Biener wrote:

> 
> The following fixes the LTO bootstrap miscompares on the branches.
> They happen because type_hash_canon can end up returning different
> types (a main variant or a variant type) dependent on whether
> we garbage collected before or not.
> 
> The patch makes us never return a variant type from type_hash_canon
> if we fed it a main variant (and vice versa).

Ok, that was the wrong idea - type_hash_canon says:

tree
type_hash_canon (unsigned int hashcode, tree type)
{
  tree t1;

  /* The hash table only contains main variants, so ensure that's what we're
     being passed.  */
  gcc_assert (TYPE_MAIN_VARIANT (type) == type);

ok - but if I place an assert that we only get main variants from it
as well like with

@@ -6759,6 +6759,7 @@ type_hash_canon (unsigned int hashcode,
   t1 = type_hash_lookup (hashcode, type);
   if (t1 != 0)
     {
+      gcc_assert (TYPE_MAIN_VARIANT (t1) == t1);
       if (GATHER_STATISTICS)
        {
          tree_code_counts[(int) TREE_CODE (type)]--;

then we ICE very quickly because the C++ frontend happily modifies
array types that are already recorded in the type-hash-canon hashtable.

The following patch tries to avoid that and puts in the above assert.

Sofar the patch survived building stage2 in a LTO bootstrap on the
4.9 branch, full testing is scheduled for trunk.  The patch is
aimed at all active branches (where LTO bootstrap is currently
broken because of it).

Jason, are you happy with that (esp. ripping out the odd
type completion stuff that also messes with types recorded in
said hashtable)?  That code was put in to fix PR57325 (the testcase
doesn't ICE after the patch but is rejected) - I suppose the
fix should have been in the processing_template_decl path only
that doesn't go through the type-hash-canon machinery? 
Otherwise can you please take over?

Thanks,
Richard.

2014-08-13  Richard Biener  <rguenther@suse.de>

	PR middle-end/62077
	* tree.c (type_hash_canon): Assert we only get main
	variants out of the hash.

	cp/
	* tree.c (build_cplus_array_type): Properly build
	qualified types.

Comments

Jason Merrill Aug. 13, 2014, 5:49 p.m. UTC | #1 
On 08/13/2014 10:28 AM, Richard Biener wrote:
> Sofar the patch survived building stage2 in a LTO bootstrap on the
> 4.9 branch, full testing is scheduled for trunk.

The patch breaks a lot of C++ testcases, such as
g++.old-deja/g++.other/cvt1.C; I think you need to share the "set the 
canonical type" code with the template path.

> Jason, are you happy with that (esp. ripping out the odd
> type completion stuff that also messes with types recorded in
> said hashtable)?

I'm nervous about it, since it leads to ARRAY_TYPEs with different 
TYPE_ALIGN than their elements, though I'm not sure this actually breaks 
anything.  Perhaps we could copy TYPE_ALIGN and TYPE_USER_ALIGN at the 
same place we copy TYPE_NEEDS_CONSTRUCTING.

Jason
diff mbox

Patch

Index: gcc/tree.c
===================================================================
--- gcc/tree.c	(revision 213814)
+++ gcc/tree.c	(working copy)
@@ -6759,6 +6759,7 @@  type_hash_canon (unsigned int hashcode,
   t1 = type_hash_lookup (hashcode, type);
   if (t1 != 0)
     {
+      gcc_assert (TYPE_MAIN_VARIANT (t1) == t1);
       if (GATHER_STATISTICS)
 	{
 	  tree_code_counts[(int) TREE_CODE (type)]--;
Index: gcc/cp/tree.c
===================================================================
--- gcc/cp/tree.c	(revision 213814)
+++ gcc/cp/tree.c	(working copy)
@@ -824,7 +824,11 @@  build_cplus_array_type (tree elt_type, t
 	build_cplus_array_type
 	  (TYPE_CANONICAL (elt_type),
 	   index_type ? TYPE_CANONICAL (index_type) : index_type);
-      t = build_array_type (elt_type, index_type);
+      if (elt_type != TYPE_MAIN_VARIANT (elt_type))
+	t = build_cplus_array_type (TYPE_MAIN_VARIANT (elt_type),
+				    index_type);
+      else
+	t = build_array_type (elt_type, index_type);
     }
 
   /* Push these needs up so that initialization takes place
@@ -840,37 +844,17 @@  build_cplus_array_type (tree elt_type, t
      element type as well, so fix it up if needed.  */
   if (elt_type != TYPE_MAIN_VARIANT (elt_type))
     {
-      tree m = build_cplus_array_type (TYPE_MAIN_VARIANT (elt_type),
-				       index_type);
-
-      if (TYPE_MAIN_VARIANT (t) != m)
+      tree t1;
+      for (t1 = TYPE_MAIN_VARIANT (t); t1; t1 = TYPE_NEXT_VARIANT (t1))
+	if (TREE_TYPE (t1) == elt_type)
+	  {
+	    t = t1;
+	    break;
+	  }
+      if (!t1)
 	{
-	  if (COMPLETE_TYPE_P (TREE_TYPE (t)) && !COMPLETE_TYPE_P (m))
-	    {
-	      /* m was built before the element type was complete, so we
-		 also need to copy the layout info from t.  We might
-	         end up doing this multiple times if t is an array of
-	         unknown bound.  */
-	      tree size = TYPE_SIZE (t);
-	      tree size_unit = TYPE_SIZE_UNIT (t);
-	      unsigned int align = TYPE_ALIGN (t);
-	      unsigned int user_align = TYPE_USER_ALIGN (t);
-	      enum machine_mode mode = TYPE_MODE (t);
-	      for (tree var = m; var; var = TYPE_NEXT_VARIANT (var))
-		{
-		  TYPE_SIZE (var) = size;
-		  TYPE_SIZE_UNIT (var) = size_unit;
-		  TYPE_ALIGN (var) = align;
-		  TYPE_USER_ALIGN (var) = user_align;
-		  SET_TYPE_MODE (var, mode);
-		  TYPE_NEEDS_CONSTRUCTING (var) = needs_ctor;
-		  TYPE_HAS_NONTRIVIAL_DESTRUCTOR (var) = needs_dtor;
-		}
-	    }
-
-	  TYPE_MAIN_VARIANT (t) = m;
-	  TYPE_NEXT_VARIANT (t) = TYPE_NEXT_VARIANT (m);
-	  TYPE_NEXT_VARIANT (m) = t;
+	  t = build_variant_type_copy (t);
+	  TREE_TYPE (t) = elt_type;
 	}
     }