diff mbox series

[C] fix aliasing for structures/unions with incomplete types

Message ID 0379aa34b5050780f99894b4040e416e3e95e851.camel@tugraz.at
State New
Headers show
Series [C] fix aliasing for structures/unions with incomplete types | expand

Commit Message

Martin Uecker April 2, 2024, 7:02 p.m. UTC 
While fixing the other issue, I realized that the way the
equivalence classes are computed for TYPE_CANONICAL did
not take into account that completion of struct types
also affectes compatibility of types that contain pointers
to them.  So the algorithm must be more conservative
creating bigger equivalence classes.



Bootstrapped and regession tested on x86_64



[C23]fix aliasing for structures/unions with incomplete types

When incomplete structure/union types are completed later, compatibility
of struct types that contain pointers to such types changes.  When forming
equivalence classes for TYPE_CANONICAL, we therefor need to be conservative
and treat all structs with the same tag which are pointer targets as
equivalent.

gcc/c/
	* c-typeck.cc (comptypes_internal): Add flag to track
	whether a struct is the target of a pointer.
	(tagged_types_tu_compatible): When forming equivalence
	classes, treat pointed-to structs as equivalent.

gcc/testsuite/
	* gcc.dg/c23-tag-incomplate-alias-1.c: New test.
---
 gcc/c/c-typeck.cc                             | 11 ++++++
 .../gcc.dg/c23-tag-incomplete-alias-1.c       | 34 +++++++++++++++++++
 2 files changed, 45 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/c23-tag-incomplete-alias-1.c

Comments

Joseph Myers April 2, 2024, 8:42 p.m. UTC | #1 
On Tue, 2 Apr 2024, Martin Uecker wrote:

> [C23]fix aliasing for structures/unions with incomplete types
> 
> When incomplete structure/union types are completed later, compatibility
> of struct types that contain pointers to such types changes.  When forming
> equivalence classes for TYPE_CANONICAL, we therefor need to be conservative
> and treat all structs with the same tag which are pointer targets as
> equivalent.

I don't see how what it done is actually about "which are pointer 
targets".

> @@ -1355,6 +1356,7 @@ comptypes_internal (const_tree type1, const_tree type2,
>        /* Do not remove mode information.  */
>        if (TYPE_MODE (t1) != TYPE_MODE (t2))
>  	return false;
> +      data->pointedto = true;
>        return comptypes_internal (TREE_TYPE (t1), TREE_TYPE (t2), data);

This appears to be more like "which are the targets of pointers that *have 
just been compared in the present comptypes call*".  Not which are targets 
of some other pointers not involved in that call.

Maybe some such logic based only on pointers compared in the present call 
makes sense for some purposes, but it's not clear to me that either this 
or any similar approach is a good approach for TYPE_CANONICAL - couldn't 
that mean that two types are considered equivalent for TYPE_CANONICAL at 
one point in the translation unit, but no longer equivalent at some later 
point when the comparison takes place in the context of comparing two 
other pointer types?
Martin Uecker April 2, 2024, 9:22 p.m. UTC | #2 
Am Dienstag, dem 02.04.2024 um 20:42 +0000 schrieb Joseph Myers:
> On Tue, 2 Apr 2024, Martin Uecker wrote:
> 
> > [C23]fix aliasing for structures/unions with incomplete types
> > 
> > When incomplete structure/union types are completed later, compatibility
> > of struct types that contain pointers to such types changes.  When forming
> > equivalence classes for TYPE_CANONICAL, we therefor need to be conservative
> > and treat all structs with the same tag which are pointer targets as
> > equivalent.
> 
> I don't see how what it done is actually about "which are pointer 
> targets".

Right, I see now that the description needs to be improved. This refers
only to targets of pointers included somewhere in the type we process
for purposes of determining the equivalence class of this type (but
not for other contexts).

> 
> > @@ -1355,6 +1356,7 @@ comptypes_internal (const_tree type1, const_tree type2,
> >        /* Do not remove mode information.  */
> >        if (TYPE_MODE (t1) != TYPE_MODE (t2))
> >  	return false;
> > +      data->pointedto = true;
> >        return comptypes_internal (TREE_TYPE (t1), TREE_TYPE (t2), data);
> 
> This appears to be more like "which are the targets of pointers that *have 
> just been compared in the present comptypes call*".  Not which are targets 
> of some other pointers not involved in that call.

Correct.
> 
> Maybe some such logic based only on pointers compared in the present call 
> makes sense for some purposes, but it's not clear to me that either this 
> or any similar approach is a good approach for TYPE_CANONICAL - couldn't 
> that mean that two types are considered equivalent for TYPE_CANONICAL at 
> one point in the translation unit, but no longer equivalent at some later 
> point when the comparison takes place in the context of comparing two 
> other pointer types?

They would always be considered equivalent when pointed to from another
struct (or indirectly from a type nested in this struct) for purposes
of determining the equivalence class of this struct.

When not a pointer target, i.e. when considering the struct type itself
for which we compute TYPE_CANONICAL or another struct type directly used
for a member, then the types are compared by recursing into them. Such 
types can never be incomplete at this point, so this is also stable property.

To summarize: for determining equivalence classes we always stop
the recursion after following pointers into other structs. We give
the same TYPE_CANONICAL to the following two structs foo:

struct foo { struct aa { int x; } *p; };
struct foo { struct aa { float x; } *p; };

while we give different TYPE_CANONICAL to

struct bar { struct aa { int x; } p; };
struct bar { struct aa { float x; } p; };

(not pointer).  The reason is that for the struct foo's there
could be a 

struct foo { struct aa *p; };

with incomplete type struct aa that later turns out to be compatible
with either of them. So we have to put them all into the same 
equivalence class.

(a potential alternative is to compute the classes only at the very end
when all types have stablized, but this would require much more changes
and another pass over all the types.)


Note that the TYPE_CANONICAL for the aa's is not affected in any case and
always computed based on *their* content  independent of whether they are
pointer targets or not.  (but this reminds me to double check what
happens with types that are never completed in a TU.).


I hope this explanation makes sense.


Martin




>
Joseph Myers April 3, 2024, 3:33 p.m. UTC | #3 
On Tue, 2 Apr 2024, Martin Uecker wrote:

> Am Dienstag, dem 02.04.2024 um 20:42 +0000 schrieb Joseph Myers:
> > On Tue, 2 Apr 2024, Martin Uecker wrote:
> > 
> > > [C23]fix aliasing for structures/unions with incomplete types
> > > 
> > > When incomplete structure/union types are completed later, compatibility
> > > of struct types that contain pointers to such types changes.  When forming
> > > equivalence classes for TYPE_CANONICAL, we therefor need to be conservative
> > > and treat all structs with the same tag which are pointer targets as
> > > equivalent.
> > 
> > I don't see how what it done is actually about "which are pointer 
> > targets".
> 
> Right, I see now that the description needs to be improved. This refers
> only to targets of pointers included somewhere in the type we process
> for purposes of determining the equivalence class of this type (but
> not for other contexts).

A detailed self-contained description of the TYPE_CANONICAL / aliasing 
logic also needs to go somewhere in a comment in the source code rather 
than only in commit messages / emails.
diff mbox series

Patch

diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
index ddeab1e2a8a..b86450580ad 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -1170,6 +1170,7 @@  struct comptypes_data {
   bool different_types_p;
   bool warning_needed;
   bool anon_field;
+  bool pointedto;
   bool equiv;
 
   const struct tagged_tu_seen_cache* cache;
@@ -1355,6 +1356,7 @@  comptypes_internal (const_tree type1, const_tree type2,
       /* Do not remove mode information.  */
       if (TYPE_MODE (t1) != TYPE_MODE (t2))
 	return false;
+      data->pointedto = true;
       return comptypes_internal (TREE_TYPE (t1), TREE_TYPE (t2), data);
 
     case FUNCTION_TYPE:
@@ -1513,6 +1515,14 @@  tagged_types_tu_compatible_p (const_tree t1, const_tree t2,
   if (TYPE_NAME (t1) != TYPE_NAME (t2))
     return false;
 
+  /* When forming equivalence classes for TYPE_CANONICAL in C23, we
+     have to treat structs with the same tag as equivalent, when they
+     are targets of pointers inside other structs.  This is necessary
+     so that the relationship of types does not change when incomplete
+     types are completed.  */
+  if (data->equiv && data->pointedto)
+    return true;
+
   if (!data->anon_field && NULL_TREE == TYPE_NAME (t1))
     return false;
 
@@ -1608,6 +1618,7 @@  tagged_types_tu_compatible_p (const_tree t1, const_tree t2,
 	      return false;
 
 	    data->anon_field = !DECL_NAME (s1);
+	    data->pointedto = false;
 
 	    data->cache = &entry;
 	    if (!comptypes_internal (TREE_TYPE (s1), TREE_TYPE (s2), data))
diff --git a/gcc/testsuite/gcc.dg/c23-tag-incomplete-alias-1.c b/gcc/testsuite/gcc.dg/c23-tag-incomplete-alias-1.c
new file mode 100644
index 00000000000..7fb6a8513b2
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/c23-tag-incomplete-alias-1.c
@@ -0,0 +1,34 @@ 
+/* { dg-do run } 
+ * { dg-options "-std=c23 -O2" } */
+
+[[gnu::noinline]]
+void *alias(void *ap, void *b, void *x, void *y)
+{
+	struct foo { struct bar *f; } *a = ap;
+	struct bar { long x; };
+
+	a->f = x;
+
+	{
+		struct bar;
+		struct foo { struct bar *f; };
+		struct bar { long x; };
+
+		((struct foo*)b)->f = y;
+	}
+
+
+	return a->f;
+}
+
+int main()
+{
+	struct bar { long x; };
+	struct foo { struct bar *f; } a;
+	struct bar x, y;
+	if (&y != alias(&a, &a, &x, &y))
+		__builtin_abort();
+
+	return 0;
+}
+