Re: [PATCH] Add a new type attribute always_alias (PR79671)

[Richard Biener <rguenther@suse.de>]

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

On Tue, 11 Apr 2017, Richard Biener wrote:

> On Tue, 11 Apr 2017, Richard Biener wrote:
> 
> > On Mon, 10 Apr 2017, Jason Merrill wrote:
> > 
> > > On Mon, Apr 10, 2017 at 11:30 AM, Richard Biener <rguenther@suse.de> wrote:
> > > > On Mon, 10 Apr 2017, Jason Merrill wrote:
> > > >> On Mon, Apr 10, 2017 at 8:50 AM, Richard Biener <rguenther@suse.de> wrote:
> > > >> >         * tree.c (build_cplus_array_type): Set TYPE_TYPELESS_STORAGE
> > > >> >         for arrays of unsigned char or std::byte.
> > > >>
> > > >> I think it would be good to have a flag to select whether these
> > > >> semantics apply to any char variant and std::byte, only unsigned char
> > > >> and std::byte, or only std::byte.
> > > >
> > > > Any suggestion?  Not sure we really need it (I'm hesitant to write
> > > > all the testcases to verify it actually works).
> > > 
> > > Well, there's existing code that uses plain char (e.g. boost) that I
> > > want to support.  If there's a significant optimization penalty for
> > > allowing that, we probably also want to be able to limit the impact.
> > > If there isn't much penalty, let's just support all char variants.
> > 
> > I haven't assessed the penalty involved but it can't be worse than
> > the situation we had in GCC 6.  So I think it's reasonable to support
> > all char variants for now.  One could add some instrumenting to
> > alias_set_subset_of / alias_sets_conflict_p but it would only yield
> > an upper bound on the number of failed queries (TBAA is a quite early
> > out before PTA info is used for example).
> > 
> > The following variant -- added missing
> > 
> > Index: gcc/cp/tree.c
> > ===================================================================
> > --- gcc/cp/tree.c       (revision 246832)
> > +++ gcc/cp/tree.c       (working copy)
> > @@ -972,6 +979,7 @@ build_cplus_array_type (tree elt_type, t
> >                  as it will overwrite alignment etc. of all variants.  */
> >               TYPE_SIZE (t) = TYPE_SIZE (m);
> >               TYPE_SIZE_UNIT (t) = TYPE_SIZE_UNIT (m);
> > +             TYPE_TYPELESS_STORAGE (t) = TYPE_TYPELESS_STORAGE (m);
> >             }
> >  
> >           TYPE_MAIN_VARIANT (t) = m;
> > 
> > that caused LTO bootstrap to fail and removed the tree-ssa-structalias.c
> > change (committed separately) [LTO] bootstrapped and tested ok on 
> > x86_64-unknown-linux-gnu.
> > 
> > I've tested some template examples and they seem to work fine.
> > 
> > Ok for trunk?
> > 
> > Disclaimer: there might still be an issue with cross-language LTO
> > support, but it might at most result in TYPE_TYPELESS_STORAGE
> > getting lost.  Trying to craft a testcase to verify my theory.
> 
> Not too difficult in the end (see below).  A fix (below) is to
> not treat arrays with differing TYPE_TYPELESS_STORAGE as
> compatible for the purpose of computing TYPE_CANONICAL (and
> thus recursively structures containing them).  While they'd
> still alias each other (because currently a TYPE_TYPELESS_STORAGE
> member makes the whole thing effectively alias anything) this
> results in warnings in case such a type is used in the interface
> between C and C++ (it's also considered a ODR type).
> 
> t.C:17:17: warning: type of Β‘barΒ’ does not match original declaration 
> [-Wlto-type-mismatch]
>  extern "C" void bar (X *);
>                  ^
> t2.c:5:6: note: Β‘barΒ’ was previously declared here
>  void bar (struct X *p) {}
>       ^
> 
> if you add a struct X * parameter to bar().
> 
> So it's not the optimal solution here.  Another fix would be to
> somehow make TYPE_TYPELESS_STORAGE "sticky" when merging canonical
> types but there's not precedent in doing this kind of thing and
> I'm not sure we'll get everything merged before computing alias
> sets.
> 
> CCing Honza.

So after discussion and some more thinking we don't really benefit
(and really can't) from having different aggregates with such
members distignuishable via their alias set.  So the following
simplifies the approach and makes the above LTO bits trivial
by more following Bernds approach of assigning the types alias
set zero and instead of in the alias machinery propagate the
flag on the types.

It should also make reference_alias_ptr_type correct and it
does the flag propagation in type layout.

[LTO] bootstrap and regtest running on x86_64-unknown-linux-gnu.

The C++ bits are unchanged.

Richard.

2017-04-11  Richard Biener  <rguenther@suse.de>
	Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR middle-end/79671
	* alias.c (component_uses_parent_alias_set_from): Handle
	TYPE_TYPELESS_STORAGE.
	(get_alias_set): Likewise.
	* tree-core.h (tree_type_common): Add typeless_storage flag.
	* tree.h (TYPE_TYPELESS_STORAGE): New macro.
	* stor-layout.c (place_union_field): Set TYPE_TYPELESS_STORAGE
	for types containing members with TYPE_TYPELESS_STORAGE.
	(place_field): Likewise.
	(layout_type): Likewise for ARRAY_TYPE.
	* lto-streamer-out.c (hash_tree): Hash TYPE_TYPELESS_STORAGE.
	* tree-streamer-in.c (unpack_ts_type_common_value_fields): Stream
	TYPE_TYPELESS_STORAGE.
	* tree-streamer-out.c (pack_ts_type_common_value_fields): Likewise.

	lto/
	* lto.c (compare_tree_sccs_1): Compare TYPE_TYPELESS_STORAGE.

	cp/
	* tree.c (build_cplus_array_type): Set TYPE_TYPELESS_STORAGE
	for arrays of character or std::byte type.

	* g++.dg/torture/pr79671.C: New testcase.
	* g++.dg/lto/pr79671_0.C: Likewise.
	* g++.dg/lto/pr79671_1.c: Likewise.

Index: gcc/alias.c
===================================================================
--- gcc/alias.c	(revision 246835)
+++ gcc/alias.c	(working copy)
@@ -613,6 +619,10 @@ component_uses_parent_alias_set_from (co
 {
   const_tree found = NULL_TREE;
 
+  if (AGGREGATE_TYPE_P (TREE_TYPE (t))
+      && TYPE_TYPELESS_STORAGE (TREE_TYPE (t)))
+    return const_cast <tree> (t);
+
   while (handled_component_p (t))
     {
       switch (TREE_CODE (t))
@@ -883,6 +894,10 @@ get_alias_set (tree t)
      variant.  */
   t = TYPE_MAIN_VARIANT (t);
 
+  if (AGGREGATE_TYPE_P (t)
+      && TYPE_TYPELESS_STORAGE (t))
+    return 0;
+
   /* Always use the canonical type as well.  If this is a type that
      requires structural comparisons to identify compatible types
      use alias set zero.  */
Index: gcc/cp/tree.c
===================================================================
--- gcc/cp/tree.c	(revision 246835)
+++ gcc/cp/tree.c	(working copy)
@@ -949,6 +949,13 @@ 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;
     }
 
   /* Now check whether we already have this array variant.  */
@@ -972,6 +979,7 @@ build_cplus_array_type (tree elt_type, t
 		 as it will overwrite alignment etc. of all variants.  */
 	      TYPE_SIZE (t) = TYPE_SIZE (m);
 	      TYPE_SIZE_UNIT (t) = TYPE_SIZE_UNIT (m);
+	      TYPE_TYPELESS_STORAGE (t) = TYPE_TYPELESS_STORAGE (m);
 	    }
 
 	  TYPE_MAIN_VARIANT (t) = m;
Index: gcc/lto/lto.c
===================================================================
--- gcc/lto/lto.c	(revision 246835)
+++ gcc/lto/lto.c	(working copy)
@@ -1162,6 +1162,8 @@ compare_tree_sccs_1 (tree t1, tree t2, t
 	}
       else if (code == ARRAY_TYPE)
 	compare_values (TYPE_NONALIASED_COMPONENT);
+      if (AGGREGATE_TYPE_P (t1))
+	compare_values (TYPE_TYPELESS_STORAGE);
       compare_values (TYPE_PACKED);
       compare_values (TYPE_RESTRICT);
       compare_values (TYPE_USER_ALIGN);
Index: gcc/lto-streamer-out.c
===================================================================
--- gcc/lto-streamer-out.c	(revision 246835)
+++ gcc/lto-streamer-out.c	(working copy)
@@ -1143,6 +1143,8 @@ hash_tree (struct streamer_tree_cache_d
 	}
       else if (code == ARRAY_TYPE)
 	hstate.add_flag (TYPE_NONALIASED_COMPONENT (t));
+      if (AGGREGATE_TYPE_P (t))
+	hstate.add_flag (TYPE_TYPELESS_STORAGE (t));
       hstate.commit_flag ();
       hstate.add_int (TYPE_PRECISION (t));
       hstate.add_int (TYPE_ALIGN (t));
Index: gcc/stor-layout.c
===================================================================
--- gcc/stor-layout.c	(revision 246835)
+++ gcc/stor-layout.c	(working copy)
@@ -1091,6 +1091,10 @@ place_union_field (record_layout_info rl
   if (TREE_CODE (TREE_TYPE (field)) == ERROR_MARK)
     return;
 
+  if (AGGREGATE_TYPE_P (TREE_TYPE (field))
+      && TYPE_TYPELESS_STORAGE (TREE_TYPE (field)))
+    TYPE_TYPELESS_STORAGE (rli->t) = 1;
+
   /* We assume the union's size will be a multiple of a byte so we don't
      bother with BITPOS.  */
   if (TREE_CODE (rli->t) == UNION_TYPE)
@@ -1168,6 +1172,10 @@ place_field (record_layout_info rli, tre
       return;
     }
 
+  if (AGGREGATE_TYPE_P (type)
+      && TYPE_TYPELESS_STORAGE (type))
+    TYPE_TYPELESS_STORAGE (rli->t) = 1;
+
   /* Work out the known alignment so far.  Note that A & (-A) is the
      value of the least-significant bit in A that is one.  */
   if (! integer_zerop (rli->bitpos))
@@ -2340,6 +2348,8 @@ layout_type (tree type)
 		SET_TYPE_MODE (type, BLKmode);
 	      }
 	  }
+	if (AGGREGATE_TYPE_P (element))
+	  TYPE_TYPELESS_STORAGE (type) = TYPE_TYPELESS_STORAGE (element);
 	/* When the element size is constant, check that it is at least as
 	   large as the element alignment.  */
 	if (TYPE_SIZE_UNIT (element)
Index: gcc/testsuite/g++.dg/lto/pr79671_0.C
===================================================================
--- gcc/testsuite/g++.dg/lto/pr79671_0.C	(nonexistent)
+++ gcc/testsuite/g++.dg/lto/pr79671_0.C	(working copy)
@@ -0,0 +1,26 @@
+// { dg-lto-do run }
+
+void *operator new(__SIZE_TYPE__, void *p2) { return p2; }
+struct B { B(int i_) : i(i_) {} int i; };
+struct X
+{
+  unsigned char buf[sizeof (B)];
+};
+
+int __attribute__((noinline)) foo()
+{
+  X x alignas (B), y alignas (B);
+  new (&x) B (0);
+  y = x;
+  B *q = reinterpret_cast <B *>(&y);
+  asm volatile ("" : "=r" (q) : "r" (q));
+  return q->i;
+}
+extern "C" void bar ();
+int main()
+{
+  if (foo() != 0)
+    __builtin_abort ();
+  bar ();
+  return 0;
+}
Index: gcc/testsuite/g++.dg/lto/pr79671_1.c
===================================================================
--- gcc/testsuite/g++.dg/lto/pr79671_1.c	(nonexistent)
+++ gcc/testsuite/g++.dg/lto/pr79671_1.c	(working copy)
@@ -0,0 +1,5 @@
+struct X
+{
+  unsigned char buf[sizeof (int)];
+};
+void bar () { struct X x; *(volatile char *)x.buf = 1; }
Index: gcc/testsuite/g++.dg/torture/pr79671.C
===================================================================
--- gcc/testsuite/g++.dg/torture/pr79671.C	(nonexistent)
+++ gcc/testsuite/g++.dg/torture/pr79671.C	(working copy)
@@ -0,0 +1,25 @@
+// { dg-do run }
+
+void *operator new(__SIZE_TYPE__, void *p2) { return p2; }
+struct B { B(int i_) : i(i_) {} int i; };
+struct X
+{
+  unsigned char buf[sizeof (B)];
+};
+
+int __attribute__((noinline)) foo()
+{
+  X x alignas(B), y alignas(B);
+  new (&x) B (0);
+  y = x;
+  B *q = reinterpret_cast <B *>(&y);
+  asm volatile ("" : "=r" (q) : "r" (q));
+  return q->i;
+}
+
+int main()
+{
+  if (foo() != 0)
+    __builtin_abort ();
+  return 0;
+}
Index: gcc/tree-core.h
===================================================================
--- gcc/tree-core.h	(revision 246835)
+++ gcc/tree-core.h	(working copy)
@@ -1511,7 +1511,9 @@ struct GTY(()) tree_type_common {
      so we need to store the value 32 (not 31, as we need the zero
      as well), hence six bits.  */
   unsigned align : 6;
-  unsigned spare : 25;
+  unsigned typeless_storage : 1;
+  unsigned spare : 24;
+
   alias_set_type alias_set;
   tree pointer_to;
   tree reference_to;
Index: gcc/tree-streamer-in.c
===================================================================
--- gcc/tree-streamer-in.c	(revision 246835)
+++ gcc/tree-streamer-in.c	(working copy)
@@ -376,6 +376,8 @@ unpack_ts_type_common_value_fields (stru
     }
   else if (TREE_CODE (expr) == ARRAY_TYPE)
     TYPE_NONALIASED_COMPONENT (expr) = (unsigned) bp_unpack_value (bp, 1);
+  if (AGGREGATE_TYPE_P (expr))
+    TYPE_TYPELESS_STORAGE (expr) = (unsigned) bp_unpack_value (bp, 1);
   TYPE_PRECISION (expr) = bp_unpack_var_len_unsigned (bp);
   SET_TYPE_ALIGN (expr, bp_unpack_var_len_unsigned (bp));
 #ifdef ACCEL_COMPILER
Index: gcc/tree-streamer-out.c
===================================================================
--- gcc/tree-streamer-out.c	(revision 246835)
+++ gcc/tree-streamer-out.c	(working copy)
@@ -328,6 +328,8 @@ pack_ts_type_common_value_fields (struct
     }
   else if (TREE_CODE (expr) == ARRAY_TYPE)
     bp_pack_value (bp, TYPE_NONALIASED_COMPONENT (expr), 1);
+  if (AGGREGATE_TYPE_P (expr))
+    bp_pack_value (bp, TYPE_TYPELESS_STORAGE (expr), 1);
   bp_pack_var_len_unsigned (bp, TYPE_PRECISION (expr));
   bp_pack_var_len_unsigned (bp, TYPE_ALIGN (expr));
 }
Index: gcc/tree.h
===================================================================
--- gcc/tree.h	(revision 246835)
+++ gcc/tree.h	(working copy)
@@ -2035,6 +2035,13 @@ extern machine_mode element_mode (const_
 #define TYPE_NONALIASED_COMPONENT(NODE) \
   (ARRAY_TYPE_CHECK (NODE)->type_common.transparent_aggr_flag)
 
+/* For an ARRAY_TYPE, a RECORD_TYPE, a UNION_TYPE or a QUAL_UNION_TYPE
+   whether the array is typeless storage or the type contains a member
+   with this flag set.  Such types are excempt from type-based alias
+   analysis.  */
+#define TYPE_TYPELESS_STORAGE(NODE) \
+  (TREE_CHECK4 (NODE, RECORD_TYPE, UNION_TYPE, QUAL_UNION_TYPE, ARRAY_TYPE)->type_common.typeless_storage)
+
 /* Indicated that objects of this type should be laid out in as
    compact a way as possible.  */
 #define TYPE_PACKED(NODE) (TYPE_CHECK (NODE)->base.u.bits.packed_flag)