On Sat, 23 Nov 2024, Martin Uecker wrote:
>
> This patch tries fixes the errors we have because of
> flexible array members. I am bit unsure about the exception
> for the mode.
>
> Bootstrapped and regression tested on x86_64.
>
>
>
> Fix type compatibility for types with flexible array member
> [PR113688,PR114014,PR117724]
>
> verify_type checks the compatibility of TYPE_CANONICAL using
> gimple_canonical_types_compatible_p. But it is stricter than what the
> C standard requires and therefor inconsistent with how TYPE_CANONICAL is
> set
> in the C FE. Here, the logic is changed to ignore array size when one of
> the
> types is a flexible array member. To not get errors because of
> inconsistent
> number of members, zero-sized arrays are not ignored anymore when checking
> fields of a struct (which is stricter than what was done before).
> Finally, a exception is added that allows the TYPE_MODE of a type with
> flexible array member to differ from another compatible type.
>
> PR c/113688
> PR c/114014
> PR c/117724
>
> gcc/ChangeLog:
> * tree.cc (gimple_canonical_types_compatible_p): Revise
> logic for types with FAM.
> (verify_type): Add exception for mode for types with FAM.
>
> gcc/testsuite/ChangeLog:
> * gcc.dg/pr113688.c: New test.
> * gcc.dg/pr114014.c: New test.
> * gcc.dg/pr117724.c: New test.
>
> diff --git a/gcc/testsuite/gcc.dg/pr113688.c b/gcc/testsuite/gcc.dg/pr113688.c
> new file mode 100644
> index 00000000000..8dee8c86f1b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr113688.c
> @@ -0,0 +1,8 @@
> +/* { dg-do compile } */
> +/* { dg-options "-g" } */
> +
> +struct S{int x,y[1];}*a;
> +int main(void){
> + struct S{int x,y[];};
> +}
> +
> diff --git a/gcc/testsuite/gcc.dg/pr114014.c b/gcc/testsuite/gcc.dg/pr114014.c
> new file mode 100644
> index 00000000000..ab783f4f85d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr114014.c
> @@ -0,0 +1,14 @@
> +/* PR c/114014
> + * { dg-do compile }
> + * { dg-options "-std=c23 -g" } */
> +
> +struct r {
> + int a;
> + char b[];
> +};
> +struct r {
> + int a;
> + char b[0];
> +}; /* { dg-error "redefinition" } */
> +
> +
> diff --git a/gcc/testsuite/gcc.dg/pr117724.c b/gcc/testsuite/gcc.dg/pr117724.c
> new file mode 100644
> index 00000000000..d631daeb644
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr117724.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-options "-g" } */
> +
> +struct {
> + unsigned long len;
> + unsigned long size;
> + char data[];
> +}; /* { dg-warning "unnamed struct" } */
> +struct {
> + struct {
> + unsigned long len;
> + unsigned long size;
> + char data[6];
> + };
> +}; /* { dg-warning "unnamed struct" } */
> +
> diff --git a/gcc/tree.cc b/gcc/tree.cc
> index 1da06c7d4e9..dbf6b180496 100644
> --- a/gcc/tree.cc
> +++ b/gcc/tree.cc
> @@ -13900,8 +13900,11 @@ gimple_canonical_types_compatible_p (const_tree t1,
> const_tree t2,
> || TREE_CODE (t1) == NULLPTR_TYPE)
> return true;
>
> - /* Can't be the same type if they have different mode. */
> - if (TYPE_MODE (t1) != TYPE_MODE (t2))
> + /* Can't be compatible types if they have different mode. We allow
> + mismatching modes for types with flexible array member. */
> + if (!flexible_array_type_p (t1)
> + && !flexible_array_type_p (t2)
These are quite expensive (recursive on UNION, walking TYPE_FIELDs for
RECORD) and not handling ARRAY_TYPE itself.
I'll also note that same TYPE_CANONICAL implies same TYPE_SIZE given
for aggregate assignment we assume we can take the copy size from
either side.
So I wonder if instead
COMPLETE_TYPE_P (t1)
&& COMPLETE_TYPE_P (t2)
should be used here. I assume struct { int len; char data[1]; } and
struct { int len; char data[2]; } are not compatible but both can
have TYPE_CANONICAL of struct {int len; char data[]; }?
> + && (TYPE_MODE (t1) != TYPE_MODE (t2)))
> return false;
>
> /* Non-aggregate types can be handled cheaply. */
> @@ -13952,7 +13955,7 @@ gimple_canonical_types_compatible_p (const_tree t1,
> const_tree t2,
> {
> case ARRAY_TYPE:
> /* Array types are the same if the element types are the same and
> - the number of elements are the same. */
> + minimum and maximum index are the same. */
> if (!gimple_canonical_types_compatible_p (TREE_TYPE (t1), TREE_TYPE
> (t2),
> trust_type_canonical)
> || TYPE_STRING_FLAG (t1) != TYPE_STRING_FLAG (t2)
> @@ -14046,23 +14049,35 @@ gimple_canonical_types_compatible_p (const_tree t1,
> const_tree t2,
> f1 || f2;
> f1 = TREE_CHAIN (f1), f2 = TREE_CHAIN (f2))
> {
> - /* Skip non-fields and zero-sized fields. */
> - while (f1 && (TREE_CODE (f1) != FIELD_DECL
> - || (DECL_SIZE (f1)
> - && integer_zerop (DECL_SIZE (f1)))))
> + /* Skip non-fields. */
> + while (f1 && (TREE_CODE (f1) != FIELD_DECL))
> f1 = TREE_CHAIN (f1);
> - while (f2 && (TREE_CODE (f2) != FIELD_DECL
> - || (DECL_SIZE (f2)
> - && integer_zerop (DECL_SIZE (f2)))))
> + while (f2 && (TREE_CODE (f2) != FIELD_DECL))
> f2 = TREE_CHAIN (f2);
The above assumes zero-sized fields are last (I guess an OK assumption),
can you put a comment after this indicating this fact?
> if (!f1 || !f2)
> break;
> +
> + tree t1 = TREE_TYPE (f1);
> + tree t2 = TREE_TYPE (f2);
> +
> + /* Special case for flexible array members. */
> + if (TREE_CHAIN (f1) == NULL_TREE
> + && TREE_CHAIN (f2) == NULL_TREE
> + && TREE_CODE (t1) == ARRAY_TYPE
> + && TREE_CODE (t2) == ARRAY_TYPE
> + && (!DECL_NOT_FLEXARRAY (f1)
> + || !DECL_NOT_FLEXARRAY (f2))
> + && TYPE_REVERSE_STORAGE_ORDER (t1) ==
> TYPE_REVERSE_STORAGE_ORDER (t2)
> + && TYPE_NONALIASED_COMPONENT (t1) == TYPE_NONALIASED_COMPONENT
> (t2)
> + && gimple_canonical_types_compatible_p
> + (TREE_TYPE (t1), TREE_TYPE (t2),
> + trust_type_canonical))
> + ;
> /* The fields must have the same name, offset and type. */
> - if (DECL_NONADDRESSABLE_P (f1) != DECL_NONADDRESSABLE_P (f2)
> + else if (DECL_NONADDRESSABLE_P (f1) != DECL_NONADDRESSABLE_P (f2)
> || !gimple_compare_field_offset (f1, f2)
> || !gimple_canonical_types_compatible_p
> - (TREE_TYPE (f1), TREE_TYPE (f2),
> - trust_type_canonical))
> + (t1, t2, trust_type_canonical))
So what was actually mismatching with the old code? Why can you spare
comparing the start of the array member (gimple_compare_field_offset)?
Should the special-casing be only to not compare the array type itself
but its element type? What about int[][] (not sure if there's such
thing in C, an array type of array type with flexible size)?
> return false;
> }
>
> @@ -14206,6 +14221,9 @@ verify_type (const_tree t)
> }
>
> if (COMPLETE_TYPE_P (t) && TYPE_CANONICAL (t)
> + /* We allow a mismatch for flexible array members. */
> + && !flexible_array_type_p (t)
> + && !flexible_array_type_p (TYPE_CANONICAL (t))
Same as above, COMPLETE_TYPE_P (TYPE_CANONICAL (t))?
> && TYPE_MODE (t) != TYPE_MODE (TYPE_CANONICAL (t)))
> {
> error ("%<TYPE_MODE%> of %<TYPE_CANONICAL%> is not compatible");
>
>
>
--
Richard Biener <[email protected]>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
