diff mbox

Fix TYPE_TYPELESS_STORAGE handling (PR middle-end/80423)

Message ID 20170419062223.GC1777@tucnak
State New
Headers show

Commit Message

Jakub Jelinek April 19, 2017, 6:22 a.m. UTC 
On Wed, Apr 19, 2017 at 07:56:30AM +0200, Jakub Jelinek wrote:
> On Wed, Apr 19, 2017 at 07:45:36AM +0200, Richard Biener wrote:
> > >As mentioned in the PR, we now use TYPE_TYPELESS_STORAGE flag on
> > >ARRAY_TYPEs to denote types that need the special C++ alias handling.
> > >The problem is how is that created, we just use build_array_type and
> > >set TYPE_TYPELESS_STORAGE on the result, but build_array_type uses type
> > >caching, so we might modify that way some other array type.
> > >If all the array type creation goes through build_cplus_array_type,
> > >that
> > >wouldn't be a problem, as that flag is dependent just on the element
> > >type, but that is not the case, c-family as well as the middle-end has
> > >lots of spots that also create array types.  So in the end whether
> > >one gets TYPE_TYPELESS_STORAGE flag or not is quite random, depends on
> > >GC etc.
> > >
> > >The following patch attempts to resolve this, by making the type
> > >hashing
> > >take that flag into account.  Bootstrapped/regtested on x86_64-linux
> > >and
> > >i686-linux, ok for trunk?
> > 
> > When changing the C++ function I thought that calling build_array_type was
> > wrong and it should instead do the same it does in the other places, use
> > its raw creation routine and then the canonical type register stuff.  But
> > I was hesitant to change this at this point.
> 
> The problem is that as the patch shows, we don't need it in a single place
> (the C++ FE), but at least in two places (C++ FE and c-family), and it
> wouldn't surprise me if we don't need it later on in further places
> (e.g. in middle-end, if we have a TYPE_TYPELESS_STORAGE array and say DSE
> wants to create a smaller one with the same property).
> 
> Using a default argument to build_array_type is likely cleaner indeed,
> I'd just then also swap the arguments to build_array_type_1 (the shared
> vs. typeless_storage).

Here in (so far untested) patch form:

2017-04-19  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/80423
	* tree.h (build_array_type): Add typeless_storage default argument.
	* tree.c (type_cache_hasher::equal): Also compare
	TYPE_TYPELESS_STORAGE flag for ARRAY_TYPEs.
	(build_array_type): Add typeless_storage argument, set
	TYPE_TYPELESS_STORAGE to it, if shared also hash it, and pass to
	recursive call.
	(build_nonshared_array_type): Adjust build_array_type_1 caller.
	(build_array_type): Likewise.  Add typeless_storage argument.
c-family/
	* c-common.c (complete_array_type): Preserve TYPE_TYPELESS_STORAGE.
cp/
	* tree.c (build_cplus_array_type): Call build_array_type
	with the intended TYPE_TYPELESS_STORAGE flag value, instead
	of calling build_array_type and modifying later TYPE_TYPELESS_STORAGE
	on the shared type.
testsuite/
	* g++.dg/other/pr80423.C: New test.



	Jakub

Comments

Jakub Jelinek April 20, 2017, 6:21 a.m. UTC | #1 
On Wed, Apr 19, 2017 at 08:22:23AM +0200, Jakub Jelinek wrote:
> Here in (so far untested) patch form:
> 
> 2017-04-19  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/80423
> 	* tree.h (build_array_type): Add typeless_storage default argument.
> 	* tree.c (type_cache_hasher::equal): Also compare
> 	TYPE_TYPELESS_STORAGE flag for ARRAY_TYPEs.
> 	(build_array_type): Add typeless_storage argument, set
> 	TYPE_TYPELESS_STORAGE to it, if shared also hash it, and pass to
> 	recursive call.
> 	(build_nonshared_array_type): Adjust build_array_type_1 caller.
> 	(build_array_type): Likewise.  Add typeless_storage argument.
> c-family/
> 	* c-common.c (complete_array_type): Preserve TYPE_TYPELESS_STORAGE.
> cp/
> 	* tree.c (build_cplus_array_type): Call build_array_type
> 	with the intended TYPE_TYPELESS_STORAGE flag value, instead
> 	of calling build_array_type and modifying later TYPE_TYPELESS_STORAGE
> 	on the shared type.
> testsuite/
> 	* g++.dg/other/pr80423.C: New test.

Just for completeness, it bootstrapped/regtested successfully on x86_64-linux
and i686-linux.

	Jakub
Richard Biener April 20, 2017, 8:28 a.m. UTC | #2 
On Thu, 20 Apr 2017, Jakub Jelinek wrote:

> On Wed, Apr 19, 2017 at 08:22:23AM +0200, Jakub Jelinek wrote:
> > Here in (so far untested) patch form:
> > 
> > 2017-04-19  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR middle-end/80423
> > 	* tree.h (build_array_type): Add typeless_storage default argument.
> > 	* tree.c (type_cache_hasher::equal): Also compare
> > 	TYPE_TYPELESS_STORAGE flag for ARRAY_TYPEs.
> > 	(build_array_type): Add typeless_storage argument, set
> > 	TYPE_TYPELESS_STORAGE to it, if shared also hash it, and pass to
> > 	recursive call.
> > 	(build_nonshared_array_type): Adjust build_array_type_1 caller.
> > 	(build_array_type): Likewise.  Add typeless_storage argument.
> > c-family/
> > 	* c-common.c (complete_array_type): Preserve TYPE_TYPELESS_STORAGE.
> > cp/
> > 	* tree.c (build_cplus_array_type): Call build_array_type
> > 	with the intended TYPE_TYPELESS_STORAGE flag value, instead
> > 	of calling build_array_type and modifying later TYPE_TYPELESS_STORAGE
> > 	on the shared type.
> > testsuite/
> > 	* g++.dg/other/pr80423.C: New test.
> 
> Just for completeness, it bootstrapped/regtested successfully on x86_64-linux
> and i686-linux.

Ok.

Richard.
diff mbox

Patch

--- gcc/tree.h.jj	2017-04-18 15:13:25.180398014 +0200
+++ gcc/tree.h	2017-04-19 08:16:12.859844328 +0200
@@ -4068,7 +4068,7 @@  extern tree build_truth_vector_type (uns
 extern tree build_same_sized_truth_vector_type (tree vectype);
 extern tree build_opaque_vector_type (tree innertype, int nunits);
 extern tree build_index_type (tree);
-extern tree build_array_type (tree, tree);
+extern tree build_array_type (tree, tree, bool = false);
 extern tree build_nonshared_array_type (tree, tree);
 extern tree build_array_type_nelts (tree, unsigned HOST_WIDE_INT);
 extern tree build_function_type (tree, tree);
--- gcc/tree.c.jj	2017-04-18 15:13:25.158398308 +0200
+++ gcc/tree.c	2017-04-19 08:17:50.938542960 +0200
@@ -7073,7 +7073,9 @@  type_cache_hasher::equal (type_hash *a,
         break;
       return 0;
     case ARRAY_TYPE:
-      return TYPE_DOMAIN (a->type) == TYPE_DOMAIN (b->type);
+      return (TYPE_TYPELESS_STORAGE (a->type)
+	      == TYPE_TYPELESS_STORAGE (b->type)
+	      && TYPE_DOMAIN (a->type) == TYPE_DOMAIN (b->type));
 
     case RECORD_TYPE:
     case UNION_TYPE:
@@ -8350,10 +8352,12 @@  subrange_type_for_debug_p (const_tree ty
 
 /* Construct, lay out and return the type of arrays of elements with ELT_TYPE
    and number of elements specified by the range of values of INDEX_TYPE.
+   If TYPELESS_STORAGE is true, TYPE_TYPELESS_STORAGE flag is set on the type.
    If SHARED is true, reuse such a type that has already been constructed.  */
 
 static tree
-build_array_type_1 (tree elt_type, tree index_type, bool shared)
+build_array_type_1 (tree elt_type, tree index_type, bool typeless_storage,
+		    bool shared)
 {
   tree t;
 
@@ -8367,6 +8371,7 @@  build_array_type_1 (tree elt_type, tree
   TREE_TYPE (t) = elt_type;
   TYPE_DOMAIN (t) = index_type;
   TYPE_ADDR_SPACE (t) = TYPE_ADDR_SPACE (elt_type);
+  TYPE_TYPELESS_STORAGE (t) = typeless_storage;
   layout_type (t);
 
   /* If the element type is incomplete at this point we get marked for
@@ -8381,6 +8386,7 @@  build_array_type_1 (tree elt_type, tree
       hstate.add_object (TYPE_HASH (elt_type));
       if (index_type)
 	hstate.add_object (TYPE_HASH (index_type));
+      hstate.add_flag (typeless_storage);
       t = type_hash_canon (hstate.end (), t);
     }
 
@@ -8396,7 +8402,7 @@  build_array_type_1 (tree elt_type, tree
 	  = build_array_type_1 (TYPE_CANONICAL (elt_type),
 				index_type
 				? TYPE_CANONICAL (index_type) : NULL_TREE,
-				shared);
+				typeless_storage, shared);
     }
 
   return t;
@@ -8405,9 +8411,9 @@  build_array_type_1 (tree elt_type, tree
 /* Wrapper around build_array_type_1 with SHARED set to true.  */
 
 tree
-build_array_type (tree elt_type, tree index_type)
+build_array_type (tree elt_type, tree index_type, bool typeless_storage)
 {
-  return build_array_type_1 (elt_type, index_type, true);
+  return build_array_type_1 (elt_type, index_type, typeless_storage, true);
 }
 
 /* Wrapper around build_array_type_1 with SHARED set to false.  */
@@ -8415,7 +8421,7 @@  build_array_type (tree elt_type, tree in
 tree
 build_nonshared_array_type (tree elt_type, tree index_type)
 {
-  return build_array_type_1 (elt_type, index_type, false);
+  return build_array_type_1 (elt_type, index_type, false, false);
 }
 
 /* Return a representation of ELT_TYPE[NELTS], using indices of type
--- gcc/c-family/c-common.c.jj	2017-04-18 15:14:29.383541136 +0200
+++ gcc/c-family/c-common.c	2017-04-19 08:15:55.346076712 +0200
@@ -6386,7 +6386,6 @@  complete_array_type (tree *ptype, tree i
 {
   tree maxindex, type, main_type, elt, unqual_elt;
   int failure = 0, quals;
-  hashval_t hashcode = 0;
   bool overflow_p = false;
 
   maxindex = size_zero_node;
@@ -6480,13 +6479,15 @@  complete_array_type (tree *ptype, tree i
   TYPE_DOMAIN (main_type)
     = build_range_type (TREE_TYPE (maxindex),
 			build_int_cst (TREE_TYPE (maxindex), 0), maxindex);
+  TYPE_TYPELESS_STORAGE (main_type) = TYPE_TYPELESS_STORAGE (type);
   layout_type (main_type);
 
   /* Make sure we have the canonical MAIN_TYPE. */
-  hashcode = iterative_hash_object (TYPE_HASH (unqual_elt), hashcode);
-  hashcode = iterative_hash_object (TYPE_HASH (TYPE_DOMAIN (main_type)),
-				    hashcode);
-  main_type = type_hash_canon (hashcode, main_type);
+  inchash::hash hstate;
+  hstate.add_object (TYPE_HASH (unqual_elt));
+  hstate.add_object (TYPE_HASH (TYPE_DOMAIN (main_type)));
+  hstate.add_flag (TYPE_TYPELESS_STORAGE (main_type));
+  main_type = type_hash_canon (hstate.end (), main_type);
 
   /* Fix the canonical type.  */
   if (TYPE_STRUCTURAL_EQUALITY_P (TREE_TYPE (main_type))
@@ -6497,7 +6498,8 @@  complete_array_type (tree *ptype, tree i
 	       != TYPE_DOMAIN (main_type)))
     TYPE_CANONICAL (main_type)
       = build_array_type (TYPE_CANONICAL (TREE_TYPE (main_type)),
-			  TYPE_CANONICAL (TYPE_DOMAIN (main_type)));
+			  TYPE_CANONICAL (TYPE_DOMAIN (main_type)),
+			  TYPE_TYPELESS_STORAGE (main_type));
   else
     TYPE_CANONICAL (main_type) = main_type;
 
--- gcc/cp/tree.c.jj	2017-04-18 15:13:27.136371909 +0200
+++ gcc/cp/tree.c	2017-04-19 08:15:39.191291063 +0200
@@ -949,14 +949,14 @@  build_cplus_array_type (tree elt_type, t
     }
   else
     {
-      t = build_array_type (elt_type, index_type);
-      if (elt_type == unsigned_char_type_node
-	  || elt_type == signed_char_type_node
-	  || elt_type == char_type_node
-	  || (TREE_CODE (elt_type) == ENUMERAL_TYPE
-	      && TYPE_CONTEXT (elt_type) == std_node
-	      && !strcmp ("byte", TYPE_NAME_STRING (elt_type))))
-	TYPE_TYPELESS_STORAGE (t) = 1;
+      bool typeless_storage
+	= (elt_type == unsigned_char_type_node
+	   || elt_type == signed_char_type_node
+	   || elt_type == char_type_node
+	   || (TREE_CODE (elt_type) == ENUMERAL_TYPE
+	       && TYPE_CONTEXT (elt_type) == std_node
+	       && !strcmp ("byte", TYPE_NAME_STRING (elt_type))));
+      t = build_array_type (elt_type, index_type, typeless_storage);
     }
 
   /* Now check whether we already have this array variant.  */
--- gcc/testsuite/g++.dg/other/pr80423.C.jj	2017-04-19 08:14:42.539042760 +0200
+++ gcc/testsuite/g++.dg/other/pr80423.C	2017-04-19 08:14:42.539042760 +0200
@@ -0,0 +1,11 @@ 
+// PR middle-end/80423
+// { dg-do compile { target c++11 } }
+
+typedef unsigned char uint8_t;                                                                                                                     
+struct A {                                                                                                                                         
+  template <int N> A(unsigned char (&)[N]);                                                                                                        
+};                                                                                                                                                 
+void fn1(A) {                                                                                                                                      
+  uint8_t a[]{0};                                                                                                                                  
+  fn1(a);                                                                                                                                          
+}