[C] : allow aliasing of compatible types derived from enumeral types [PR115157]

Message ID e367dd44ee5634f2ae3653b322339f8d61aa039a.camel@tugraz.at
State New
Headers
Series [C] : allow aliasing of compatible types derived from enumeral types [PR115157] |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Testing passed

Commit Message

Martin Uecker May 21, 2024, 12:18 p.m. UTC
  For enum and integer we allow aliasing by specifically returning
via a langhook the aliasing set of the underlying type.
But this is not sufficient for derived types, i.e. pointers to
enums and pointers to compatible integers also need to have the
same aliasing set.

We also allow forward declarations of enums which is a GNU 
extension, but I think this has to work consistently too, so
we here have the same issue as in C23 with other tagged types.

The solution in this patch is similar to what we do in C23, i.e. 
we start out with structural equality and then set TYPE_CANONICAL 
to the underlying type. The only way to make the TYPE_CANONICAL 
system work with the C rules for type compatility seems to set 
TYPE_CANONICAL to the same type for all types in a compatibility
equivalence class (as compatibility is not transitive this puts
together similar types that are not compatible). This is the
underlying type in this case.  As all types in such an equivalence
class have the same representation, so this should always work 
in my opinion (but maybe there is some middle end aspects I am
still missing).


When testing, I so far only found two minor issues, i.e. when
computing the 'aka' type in diagnostics and an issue with
godump.cc (not sure I fixed this correctly).


Beyond this patch, we need also some change for function types 
in general and there are problably also some other issues related
to incomplete arrays as well  (I added some checking to 'comptypes'
to check that all types ruled compatible by the C FE also have 
either structural equality, or have the same TYPE_CANONICAL, and
this brings up some more inconsistencies).

Thoughts?


Bootstrapped and regression tested on x86_64 (only C, C++ so far).




    C: allow aliasing of compatible types derived from enumeral types [PR115157]
    
    Aliasing of enumeral types with the underlying integer is now allowed
    by setting the aliasing set to zero.  But this does not allow aliasing
    of derived types which are compatible as required by ISO C.  Instead,
    initially set structural equality.  Then set TYPE_CANONICAL and update
    pointers and main variants when the type is completed (as done for
    structures and unions in C23).
    
    PR 115157
    
    gcc/c/
            * c-decl.cc (shadow_tag-warned,parse_xref_tag,start_enum,
            finish_enum): Set SET_TYPE_STRUCTURAL_EQUALITY / TYPE_CANONICAL.
            * c-obj-common.cc (get_alias_set): Remove special case.
            (get_aka_type): Add special case.
    
    gcc/
            * godump.cc (go_output_typedef): use TYPE_MAIN_VARIANT instead
            of TYPE_CANONICAL.
    
    gcc/testsuite/
            * gcc.dg/enum-alias-1.c: New test.
            * gcc.dg/enum-alias-2.c: New test.
            * gcc.dg/enum-alias-3.c: New test.
  

Comments

Joseph Myers May 23, 2024, 8:59 p.m. UTC | #1
On Tue, 21 May 2024, Martin Uecker wrote:
> 
>     C: allow aliasing of compatible types derived from enumeral types [PR115157]
>     
>     Aliasing of enumeral types with the underlying integer is now allowed
>     by setting the aliasing set to zero.  But this does not allow aliasing
>     of derived types which are compatible as required by ISO C.  Instead,
>     initially set structural equality.  Then set TYPE_CANONICAL and update
>     pointers and main variants when the type is completed (as done for
>     structures and unions in C23).
>     
>     PR 115157
>     
>     gcc/c/
>             * c-decl.cc (shadow_tag-warned,parse_xref_tag,start_enum,
>             finish_enum): Set SET_TYPE_STRUCTURAL_EQUALITY / TYPE_CANONICAL.
>             * c-obj-common.cc (get_alias_set): Remove special case.
>             (get_aka_type): Add special case.
>     
>     gcc/
>             * godump.cc (go_output_typedef): use TYPE_MAIN_VARIANT instead
>             of TYPE_CANONICAL.
>     
>     gcc/testsuite/
>             * gcc.dg/enum-alias-1.c: New test.
>             * gcc.dg/enum-alias-2.c: New test.
>             * gcc.dg/enum-alias-3.c: New test.

OK, in the absence of objections on middle-end or Go grounds within the 
next week.
  
Ian Lance Taylor May 23, 2024, 9:30 p.m. UTC | #2
On Thu, May 23, 2024 at 2:00 PM Joseph Myers <josmyers@redhat.com> wrote:
>
> On Tue, 21 May 2024, Martin Uecker wrote:
> >
> >     C: allow aliasing of compatible types derived from enumeral types [PR115157]
> >
> >     Aliasing of enumeral types with the underlying integer is now allowed
> >     by setting the aliasing set to zero.  But this does not allow aliasing
> >     of derived types which are compatible as required by ISO C.  Instead,
> >     initially set structural equality.  Then set TYPE_CANONICAL and update
> >     pointers and main variants when the type is completed (as done for
> >     structures and unions in C23).
> >
> >     PR 115157
> >
> >     gcc/c/
> >             * c-decl.cc (shadow_tag-warned,parse_xref_tag,start_enum,
> >             finish_enum): Set SET_TYPE_STRUCTURAL_EQUALITY / TYPE_CANONICAL.
> >             * c-obj-common.cc (get_alias_set): Remove special case.
> >             (get_aka_type): Add special case.
> >
> >     gcc/
> >             * godump.cc (go_output_typedef): use TYPE_MAIN_VARIANT instead
> >             of TYPE_CANONICAL.
> >
> >     gcc/testsuite/
> >             * gcc.dg/enum-alias-1.c: New test.
> >             * gcc.dg/enum-alias-2.c: New test.
> >             * gcc.dg/enum-alias-3.c: New test.
>
> OK, in the absence of objections on middle-end or Go grounds within the
> next week.

The godump.cc patch is

       && (TYPE_CANONICAL (TREE_TYPE (decl)) == NULL_TREE
          || !container->decls_seen.contains
-                                   (TYPE_CANONICAL (TREE_TYPE (decl)))))
+                                   (TYPE_MAIN_VARIANT (TREE_TYPE (decl)))))
     {

What is the problem you are seeing?

This patch isn't right:

1) The code is saying if "X == NULL_TREE || !already_seen(X)".  This
patch is changing the latter X but not the former.  They should be
consistent.

2) At the bottom of that conditional block is code that adds a value
to container->decls_seen.  Today that code is adding TYPE_CANONICAL.
If we change the condition to test TYPE_MAIN_VARIANT, then we need to
add TYPE_MAIN_VARIANT to decls_seen.

Hope that makes sense.

I don't know why the patch is required, but it's fine with those
changes as long as the libgo tests continue to pass.

Ian
  
Martin Uecker May 23, 2024, 9:47 p.m. UTC | #3
Am Donnerstag, dem 23.05.2024 um 14:30 -0700 schrieb Ian Lance Taylor:
> On Thu, May 23, 2024 at 2:00 PM Joseph Myers <josmyers@redhat.com> wrote:
> > 
> > On Tue, 21 May 2024, Martin Uecker wrote:
> > > 
> > >     C: allow aliasing of compatible types derived from enumeral types [PR115157]
> > > 
> > >     Aliasing of enumeral types with the underlying integer is now allowed
> > >     by setting the aliasing set to zero.  But this does not allow aliasing
> > >     of derived types which are compatible as required by ISO C.  Instead,
> > >     initially set structural equality.  Then set TYPE_CANONICAL and update
> > >     pointers and main variants when the type is completed (as done for
> > >     structures and unions in C23).
> > > 
> > >     PR 115157
> > > 
> > >     gcc/c/
> > >             * c-decl.cc (shadow_tag-warned,parse_xref_tag,start_enum,
> > >             finish_enum): Set SET_TYPE_STRUCTURAL_EQUALITY / TYPE_CANONICAL.
> > >             * c-obj-common.cc (get_alias_set): Remove special case.
> > >             (get_aka_type): Add special case.
> > > 
> > >     gcc/
> > >             * godump.cc (go_output_typedef): use TYPE_MAIN_VARIANT instead
> > >             of TYPE_CANONICAL.
> > > 
> > >     gcc/testsuite/
> > >             * gcc.dg/enum-alias-1.c: New test.
> > >             * gcc.dg/enum-alias-2.c: New test.
> > >             * gcc.dg/enum-alias-3.c: New test.
> > 
> > OK, in the absence of objections on middle-end or Go grounds within the
> > next week.
> 
> The godump.cc patch is
> 
>        && (TYPE_CANONICAL (TREE_TYPE (decl)) == NULL_TREE
>           || !container->decls_seen.contains
> -                                   (TYPE_CANONICAL (TREE_TYPE (decl)))))
> +                                   (TYPE_MAIN_VARIANT (TREE_TYPE (decl)))))
>      {
> 
> What is the problem you are seeing?

Test failures in godump-1.c

> 
> This patch isn't right:
> 
> 1) The code is saying if "X == NULL_TREE || !already_seen(X)".  This
> patch is changing the latter X but not the former.  They should be
> consistent.

Maybe the X == NULL_TREE can be removed if we
add TYPE_MAIN_VARIANTs instead?

> 
> 2) At the bottom of that conditional block is code that adds a value
> to container->decls_seen.  Today that code is adding TYPE_CANONICAL.
> If we change the condition to test TYPE_MAIN_VARIANT, then we need to
> add TYPE_MAIN_VARIANT to decls_seen.

Yes, obviously this is wrong. Thanks!

Martin
> 
> Hope that makes sense.
> 
> I don't know why the patch is required, but it's fine with those
> changes as long as the libgo tests continue to pass.


> 
> Ian
  
Ian Lance Taylor May 23, 2024, 9:53 p.m. UTC | #4
On Thu, May 23, 2024 at 2:48 PM Martin Uecker <uecker@tugraz.at> wrote:
>
> Am Donnerstag, dem 23.05.2024 um 14:30 -0700 schrieb Ian Lance Taylor:
> > On Thu, May 23, 2024 at 2:00 PM Joseph Myers <josmyers@redhat.com> wrote:
> > >
> > > On Tue, 21 May 2024, Martin Uecker wrote:
> > > >
> > > >     C: allow aliasing of compatible types derived from enumeral types [PR115157]
> > > >
> > > >     Aliasing of enumeral types with the underlying integer is now allowed
> > > >     by setting the aliasing set to zero.  But this does not allow aliasing
> > > >     of derived types which are compatible as required by ISO C.  Instead,
> > > >     initially set structural equality.  Then set TYPE_CANONICAL and update
> > > >     pointers and main variants when the type is completed (as done for
> > > >     structures and unions in C23).
> > > >
> > > >     PR 115157
> > > >
> > > >     gcc/c/
> > > >             * c-decl.cc (shadow_tag-warned,parse_xref_tag,start_enum,
> > > >             finish_enum): Set SET_TYPE_STRUCTURAL_EQUALITY / TYPE_CANONICAL.
> > > >             * c-obj-common.cc (get_alias_set): Remove special case.
> > > >             (get_aka_type): Add special case.
> > > >
> > > >     gcc/
> > > >             * godump.cc (go_output_typedef): use TYPE_MAIN_VARIANT instead
> > > >             of TYPE_CANONICAL.
> > > >
> > > >     gcc/testsuite/
> > > >             * gcc.dg/enum-alias-1.c: New test.
> > > >             * gcc.dg/enum-alias-2.c: New test.
> > > >             * gcc.dg/enum-alias-3.c: New test.
> > >
> > > OK, in the absence of objections on middle-end or Go grounds within the
> > > next week.
> >
> > The godump.cc patch is
> >
> >        && (TYPE_CANONICAL (TREE_TYPE (decl)) == NULL_TREE
> >           || !container->decls_seen.contains
> > -                                   (TYPE_CANONICAL (TREE_TYPE (decl)))))
> > +                                   (TYPE_MAIN_VARIANT (TREE_TYPE (decl)))))
> >      {
> >
> > What is the problem you are seeing?
>
> Test failures in godump-1.c
>
> >
> > This patch isn't right:
> >
> > 1) The code is saying if "X == NULL_TREE || !already_seen(X)".  This
> > patch is changing the latter X but not the former.  They should be
> > consistent.
>
> Maybe the X == NULL_TREE can be removed if we
> add TYPE_MAIN_VARIANTs instead?

If TYPE_MAIN_VARIANT is never NULL_TREE, then I agree that the
NULL_TREE test can be removed.

Ian
  
Richard Biener May 24, 2024, 5:56 a.m. UTC | #5
On Thu, 23 May 2024, Ian Lance Taylor wrote:

> On Thu, May 23, 2024 at 2:48 PM Martin Uecker <uecker@tugraz.at> wrote:
> >
> > Am Donnerstag, dem 23.05.2024 um 14:30 -0700 schrieb Ian Lance Taylor:
> > > On Thu, May 23, 2024 at 2:00 PM Joseph Myers <josmyers@redhat.com> wrote:
> > > >
> > > > On Tue, 21 May 2024, Martin Uecker wrote:
> > > > >
> > > > >     C: allow aliasing of compatible types derived from enumeral types [PR115157]
> > > > >
> > > > >     Aliasing of enumeral types with the underlying integer is now allowed
> > > > >     by setting the aliasing set to zero.  But this does not allow aliasing
> > > > >     of derived types which are compatible as required by ISO C.  Instead,
> > > > >     initially set structural equality.  Then set TYPE_CANONICAL and update
> > > > >     pointers and main variants when the type is completed (as done for
> > > > >     structures and unions in C23).
> > > > >
> > > > >     PR 115157
> > > > >
> > > > >     gcc/c/
> > > > >             * c-decl.cc (shadow_tag-warned,parse_xref_tag,start_enum,
> > > > >             finish_enum): Set SET_TYPE_STRUCTURAL_EQUALITY / TYPE_CANONICAL.
> > > > >             * c-obj-common.cc (get_alias_set): Remove special case.
> > > > >             (get_aka_type): Add special case.
> > > > >
> > > > >     gcc/
> > > > >             * godump.cc (go_output_typedef): use TYPE_MAIN_VARIANT instead
> > > > >             of TYPE_CANONICAL.
> > > > >
> > > > >     gcc/testsuite/
> > > > >             * gcc.dg/enum-alias-1.c: New test.
> > > > >             * gcc.dg/enum-alias-2.c: New test.
> > > > >             * gcc.dg/enum-alias-3.c: New test.
> > > >
> > > > OK, in the absence of objections on middle-end or Go grounds within the
> > > > next week.
> > >
> > > The godump.cc patch is
> > >
> > >        && (TYPE_CANONICAL (TREE_TYPE (decl)) == NULL_TREE
> > >           || !container->decls_seen.contains
> > > -                                   (TYPE_CANONICAL (TREE_TYPE (decl)))))
> > > +                                   (TYPE_MAIN_VARIANT (TREE_TYPE (decl)))))
> > >      {
> > >
> > > What is the problem you are seeing?
> >
> > Test failures in godump-1.c
> >
> > >
> > > This patch isn't right:
> > >
> > > 1) The code is saying if "X == NULL_TREE || !already_seen(X)".  This
> > > patch is changing the latter X but not the former.  They should be
> > > consistent.
> >
> > Maybe the X == NULL_TREE can be removed if we
> > add TYPE_MAIN_VARIANTs instead?
> 
> If TYPE_MAIN_VARIANT is never NULL_TREE, then I agree that the
> NULL_TREE test can be removed.

TYPE_MAIN_VARIANT is indeed never NULL_TREE.

Richard.
  

Patch

diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
index b691b91b3db..6e6606c9570 100644
--- a/gcc/c/c-decl.cc
+++ b/gcc/c/c-decl.cc
@@ -5051,7 +5051,7 @@  shadow_tag_warned (const struct c_declspecs *declspecs, int warned)
 	      if (t == NULL_TREE)
 		{
 		  t = make_node (code);
-		  if (flag_isoc23 && code != ENUMERAL_TYPE)
+		  if (flag_isoc23 || code == ENUMERAL_TYPE)
 		    SET_TYPE_STRUCTURAL_EQUALITY (t);
 		  pushtag (input_location, name, t);
 		}
@@ -8828,7 +8828,7 @@  parser_xref_tag (location_t loc, enum tree_code code, tree name,
      the forward-reference will be altered into a real type.  */
 
   ref = make_node (code);
-  if (flag_isoc23 && code != ENUMERAL_TYPE)
+  if (flag_isoc23 || code == ENUMERAL_TYPE)
     SET_TYPE_STRUCTURAL_EQUALITY (ref);
   if (code == ENUMERAL_TYPE)
     {
@@ -9919,6 +9919,7 @@  start_enum (location_t loc, struct c_enum_contents *the_enum, tree name,
     {
       enumtype = make_node (ENUMERAL_TYPE);
       TYPE_SIZE (enumtype) = NULL_TREE;
+      SET_TYPE_STRUCTURAL_EQUALITY (enumtype);
       pushtag (loc, name, enumtype);
       if (fixed_underlying_type != NULL_TREE)
 	{
@@ -9935,6 +9936,8 @@  start_enum (location_t loc, struct c_enum_contents *the_enum, tree name,
 	  TYPE_SIZE (enumtype) = NULL_TREE;
 	  TYPE_PRECISION (enumtype) = TYPE_PRECISION (fixed_underlying_type);
 	  ENUM_UNDERLYING_TYPE (enumtype) = fixed_underlying_type;
+	  TYPE_CANONICAL (enumtype) = TYPE_CANONICAL (fixed_underlying_type);
+	  c_update_type_canonical (enumtype);
 	  layout_type (enumtype);
 	}
     }
@@ -10094,6 +10097,10 @@  finish_enum (tree enumtype, tree values, tree attributes)
       ENUM_UNDERLYING_TYPE (enumtype) =
 	c_common_type_for_size (TYPE_PRECISION (tem), TYPE_UNSIGNED (tem));
 
+      TYPE_CANONICAL (enumtype) =
+	TYPE_CANONICAL (ENUM_UNDERLYING_TYPE (enumtype));
+      c_update_type_canonical (enumtype);
+
       layout_type (enumtype);
     }
 
diff --git a/gcc/c/c-objc-common.cc b/gcc/c/c-objc-common.cc
index b7c72d2609c..551ec6f4b65 100644
--- a/gcc/c/c-objc-common.cc
+++ b/gcc/c/c-objc-common.cc
@@ -130,6 +130,8 @@  get_aka_type (tree type)
 
       result = get_aka_type (orig_type);
     }
+  else if (TREE_CODE (type) == ENUMERAL_TYPE)
+    return type;
   else
     {
       tree canonical = TYPE_CANONICAL (type);
@@ -418,11 +420,6 @@  c_var_mod_p (tree x, tree fn ATTRIBUTE_UNUSED)
 alias_set_type
 c_get_alias_set (tree t)
 {
-  /* Allow aliasing between enumeral types and the underlying
-     integer type.  This is required since those are compatible types.  */
-  if (TREE_CODE (t) == ENUMERAL_TYPE)
-    return get_alias_set (ENUM_UNDERLYING_TYPE (t));
-
   /* Structs with variable size can alias different incompatible
      structs.  Let them alias anything.   */
   if (RECORD_OR_UNION_TYPE_P (t) && C_TYPE_VARIABLE_SIZE (t))
diff --git a/gcc/godump.cc b/gcc/godump.cc
index 66e73ade7df..372871a59ec 100644
--- a/gcc/godump.cc
+++ b/gcc/godump.cc
@@ -1121,7 +1121,7 @@  go_output_typedef (class godump_container *container, tree decl)
       && !container->decls_seen.contains (TREE_TYPE (decl))
       && (TYPE_CANONICAL (TREE_TYPE (decl)) == NULL_TREE
 	  || !container->decls_seen.contains
-				    (TYPE_CANONICAL (TREE_TYPE (decl)))))
+				    (TYPE_MAIN_VARIANT (TREE_TYPE (decl)))))
     {
       tree element;
 
diff --git a/gcc/testsuite/gcc.dg/enum-alias-1.c b/gcc/testsuite/gcc.dg/enum-alias-1.c
new file mode 100644
index 00000000000..8fa30eb7897
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/enum-alias-1.c
@@ -0,0 +1,24 @@ 
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+enum E { E1 = -1, E2 = 0, E3 = 1 };
+
+typedef int A;
+typedef enum E B;
+
+_Static_assert(_Generic((A){ 0 }, B: 1), "");
+
+void* foo(void* a, void *b, A *c, B *d)
+{
+	*(A**)a = c;
+	*(B**)b = d;
+	return *(A**)a;
+}
+
+int main()
+{
+	A *a, b, c;
+	if (&c != (A*)foo(&a, &a, &b, &c))
+		__builtin_abort();
+}
+
diff --git a/gcc/testsuite/gcc.dg/enum-alias-2.c b/gcc/testsuite/gcc.dg/enum-alias-2.c
new file mode 100644
index 00000000000..7ca3f3b2db8
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/enum-alias-2.c
@@ -0,0 +1,25 @@ 
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+typedef int A;
+
+void* foo(void* a, void *b, void *c, void *d)
+{
+	*(A**)a = c;
+
+	{
+		typedef enum E B;
+		enum E { E1 = -1, E2 = 0, E3 = 1 };
+		*(B**)b = d;
+	}
+
+	return *(A**)a;
+}
+
+int main()
+{
+	A *a, b, c;
+	if (&c != (A*)foo(&a, &a, &b, &c))
+		__builtin_abort();
+}
+
diff --git a/gcc/testsuite/gcc.dg/enum-alias-3.c b/gcc/testsuite/gcc.dg/enum-alias-3.c
new file mode 100644
index 00000000000..36a4f02a455
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/enum-alias-3.c
@@ -0,0 +1,26 @@ 
+/* { dg-do run } */
+/* { dg-options "-O2 -flto" } */
+
+typedef int *A;
+
+void* foo(void* a, void *b, void *c, void *d)
+{
+	*(A**)a = c;
+
+	typedef enum E *B;
+	enum E { E1 = -1, E2 = 0, E3 = 1 };
+	{
+		*(B**)b = d;
+	}
+
+	return *(A**)a;
+}
+
+int main()
+{
+	A *a, b, c;
+	if (&c != (A*)foo(&a, &a, &b, &c))
+		__builtin_abort();
+}
+
+