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

[Richard Biener <rguenther@suse.de>]

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

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.

Richard.

2017-04-11  Richard Biener  <rguenther@suse.de>

	PR middle-end/79671
	* tree.c (gimple_canonical_types_compatible_p): Do not treat
	arrays with differing TYPE_TYPELESS_STORAGE as compatible.

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

Index: gcc/tree.c
===================================================================
*** gcc/tree.c	(revision 246835)
--- gcc/tree.c	(working copy)
*************** gimple_canonical_types_compatible_p (con
*** 13709,13715 ****
  						trust_type_canonical)
  	  || TYPE_STRING_FLAG (t1) != TYPE_STRING_FLAG (t2)
  	  || TYPE_REVERSE_STORAGE_ORDER (t1) != TYPE_REVERSE_STORAGE_ORDER (t2)
! 	  || TYPE_NONALIASED_COMPONENT (t1) != TYPE_NONALIASED_COMPONENT (t2))
  	return false;
        else
  	{
--- 13709,13716 ----
  						trust_type_canonical)
  	  || TYPE_STRING_FLAG (t1) != TYPE_STRING_FLAG (t2)
  	  || TYPE_REVERSE_STORAGE_ORDER (t1) != TYPE_REVERSE_STORAGE_ORDER (t2)
! 	  || TYPE_NONALIASED_COMPONENT (t1) != TYPE_NONALIASED_COMPONENT (t2)
! 	  || TYPE_TYPELESS_STORAGE (t1) != TYPE_TYPELESS_STORAGE (t2))
  	return false;
        else
  	{
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 ****
--- 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 ****
--- 1,5 ----
+ struct X
+ {
+   unsigned char buf[sizeof (int)];
+ };
+ void bar () { struct X x; *(volatile char *)x.buf = 1; }