Re: [PATCH 3/4] c23: aliasing of compatible tagged types

[Martin Uecker <uecker@tugraz.at>]

Thanks Joseph, I will sent an updated series tomorrow. 

Richard, maybe you could look at what I wrote below
about my use of TYPE_CANONICAL ?  Does this make sense?


Am Donnerstag, dem 23.11.2023 um 23:47 +0000 schrieb Joseph Myers:
> On Thu, 16 Nov 2023, Martin Uecker wrote:
> 
> > Tell the backend which types are equivalent by setting
> > TYPE_CANONICAL to one struct in the set of equivalent
> > structs. Structs are considered equivalent by ignoring
> > all sizes of arrays nested in types below field level.
> 
> Is TYPE_CANONICAL *only* used for alias analysis?  It's not obvious to me 
> that setting TYPE_CANONICAL to a type that's definitely not equivalent for 
> other purposes is necessarily safe.

My understand is that it is used for aliasing analysis and also
checking of conversions.  TYPE_CANONICAL must be consistent with
the idea the middle-end has about type conversions.  But as long
as we do not give the same TYPE_CANONICAL to types the middle-end
thinks must be incompatible using its own type checking machinery,
it should be safe even for types the C standard thinks must be
incompatible for some reason.

> I also think more rationale is needed for ignoring sizes like this.  Is it 
> intended for e.g. making structs with flexible array members 
> alias-compatible with similar structs with a fixed-size array?

The main reason are pointers to arrays:

struct foo { int (*x)[]; }
struct foo { int (*x)[2]; };
struct foo { int (*x)[1]; };

So at least when putting it in terms of equivalence classes,
one has no choice than making those types equivalent. So
all those would get the same TYPE_CANONICAL. The middle-end 
does not care about the different pointer types (in
useless_type_conversion_p or
gimple_canonical_types_compatible_p).


Martin




> 
> > @@ -1250,6 +1266,9 @@ comptypes_internal (const_tree type1, const_tree type2,
> >  
> >  	if ((d1 == NULL_TREE) != (d2 == NULL_TREE))
> >  	  data->different_types_p = true;
> > +	/* ignore size mismatches */
> > +	if (data->equiv)
> > +	  return 1;
> 
> Should start comment with capital letter, end with '.'.
> 
> > diff --git a/gcc/testsuite/gcc.dg/c23-tag-2.c b/gcc/testsuite/gcc.dg/c23-tag-2.c
> > index 5dd4a21e9df..e28c2b5eea2 100644
> > --- a/gcc/testsuite/gcc.dg/c23-tag-2.c
> > +++ b/gcc/testsuite/gcc.dg/c23-tag-2.c
> > @@ -1,5 +1,5 @@
> > -/* { dg-do compile { target { ! "*-*-*" } } }
> > - * { dg-options "-std=c23" }
> > +/* { dg-do compile }
> > + * { dg-options "-std=c2x" }
> >   */
> >  
> >  // compatibility of structs in assignment
> > diff --git a/gcc/testsuite/gcc.dg/c23-tag-5.c b/gcc/testsuite/gcc.dg/c23-tag-5.c
> > index ff40d07aef1..95a04bf9b0e 100644
> > --- a/gcc/testsuite/gcc.dg/c23-tag-5.c
> > +++ b/gcc/testsuite/gcc.dg/c23-tag-5.c
> > @@ -1,5 +1,6 @@
> > -/* { dg-do run { target { ! "*-*-*" } } }
> > - * { dg-options "-std=c23" }
> > +/*
> > + * { dg-do run }
> > + * { dg-options "-std=c2x" }
> 
> These tests should not be changed to use -std=c2x.
> 
> > diff --git a/gcc/testsuite/gcc.dg/c23-tag-alias-2.c b/gcc/testsuite/gcc.dg/c23-tag-alias-2.c
> > new file mode 100644
> > index 00000000000..555c30a8501
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/c23-tag-alias-2.c
> > @@ -0,0 +1,73 @@
> > +/*
> > + * { dg-do run }
> > + * { dg-options "-std=c23 -O2" }
> > + */
> > +
> > +
> > +struct foo { int x; };
> > +
> > +int test_foo1(struct foo* a, void* b)
> > +{
> > +	a->x = 1;
> > +
> > +	struct foo { int x; int y; }* p = b;
> > +	p->x = 2;
> > +
> > +	return a->x;
> > +}
> 
> > +int main()
> > +{
> > +	struct foo y;
> > +
> > +	if (1 != test_foo1(&y, &y))
> > +		__builtin_abort();
> 
> This test appears to be testing various invalid cases - testing that the 
> compiler does not consider aliasing to occur in those cases (even though 
> in fact there is aliasing).
> 
> If that's the intent of this test, it definitely needs commenting.  The 
> test would also need to (be a gnu23-* test and) use appropriate attributes 
> to disable interprocedural analysis, since it would be entirely valid for 
> the compiler in this test to inline test_foo1, see that p->x in fact 
> points to the same location as a->x despite the incompatible types, and 
> have the function return 2.
> 
> The same applies to c23-tag-alias-4.c and c23-tag-alias-5.c.
> 
> > diff --git a/gcc/testsuite/gcc.dg/c23-tag-alias-5.c b/gcc/testsuite/gcc.dg/c23-tag-alias-5.c
> > new file mode 100644
> > index 00000000000..4e956720143
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/c23-tag-alias-5.c
> > @@ -0,0 +1,30 @@
> > +/* { dg-do run }
> > + * { dg-options "-std=c23 -O2" }
> > + */
> > +
> > +// not sure this is wise, but this was already like thi sbefore
> 
> "this before"
>