> On Nov 30, 2024, at 07:10, Martin Uecker <[email protected]> wrote:
>
> Am Dienstag, dem 26.11.2024 um 15:15 +0000 schrieb Qing Zhao:
>>
>>> On Nov 25, 2024, at 16:46, Martin Uecker <[email protected]> wrote:
>>>
>>>
>>> Hi Qing,
>>>
>>> Am Montag, dem 25.11.2024 um 17:40 +0000 schrieb Qing Zhao:
>>>> Hi, Martin,
>>>>
>>>> I didn’t go through all the details of your patch.
>>>>
>>>> But I have one question:
>>>>
>>>> Did you consider the effect of the option -fstrict-flex-array
>>>> (https://gcc.gnu.org/onlinedocs/gcc-14.2.0/gcc/C-Dialect-Options.html#index-fstrict-flex-arrays)
>>>> on how gcc treats the zero size trailing array, 1-element trailing array
>>>> as flexible array member in the patch?
>>>
>>> I used the function which was already there which
>>> does not take this into account. For the new version
>>> of the patch this should not matter anymore.
>>
>> Why it’s not matter anymore?
>>
>> For the following testing case:
>>
>> struct S{int x,y[1];}*a;
>> int main(void){
>> struct S{int x,y[];};
>> }
>>
>> With your latest patch, the two structures are considered as compatible
>> with -g;
>> However, if we add -fstrict-flex-array=2 or -fstrict-flex-array=3, the
>> trailing array y[1] is NOT treated
>> as FAM anymore, as a result, these two structure are NOT compatible too.
>>
>> Do I miss anything obvious?
>
> It is not about compatibility from a language semantic point of you
> but for TBAA-compatibility which needs to be consistent with it but
> can (and must be) more general.
>
> For TBAA, I think we want
>
> struct foo { int x; int y[]; };
>
> to be TBAA-compatible to
>
> struct foo { int x; int y[3]; };
Okay, I see now. Thank you for the explanation.
(Now I also see this from the comments of the routine
gimple_canonical_types_compatible_p -:)
Though, what confused me is the testing case in your patch:
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" } */
+
+
Is the above testing case claiming that b[] and b[0] are compatible from a
language semantic point of view?
thanks.
Qing
> even when we do not treat the later as FAM (i.e. still forbid
> out-of-bounds accesses).
>
> E.g. see Richard's comment:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114713#c2
>
>
> Martin
>
>> Thanks.
>>
>> Qing
>>>
>>> Martin
>>>
>>>
>>>>
>>>> thanks.
>>>>
>>>> Qing
>>>>> On Nov 23, 2024, at 14:45, Martin Uecker <[email protected]> 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)
>>>>> + && (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);
>>>>> 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))
>>>>> 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))
>>>>> && TYPE_MODE (t) != TYPE_MODE (TYPE_CANONICAL (t)))
>>>>> {
>>>>> error ("%<TYPE_MODE%> of %<TYPE_CANONICAL%> is not compatible");
