> On Dec 2, 2024, at 16:00, Martin Uecker <[email protected]> wrote:
>
> Am Montag, dem 02.12.2024 um 16:31 +0000 schrieb Qing Zhao:
>>
>>> 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?
>
> It would test that we do not crash with checking.
>
> Semantically, in c23 if you redeclare a type in the same scope then
> it must not only be compatible but is also not allowed to differ.
> So a redeclaration in the same scope has stricter requirements than
> compatibility (this also true for typedefs for example).
So, here does the “compatibility” mean “compatibility from a language semantic
point of view” or TBAA-compability?
>
> Whether we allow
>
> struct r {
> int a;
> char b[];
> };
>
> struct r {
> int a;
> char b[0];
> };
>
> depends on us because the [0] is an extension.
[0] is an extension for representing FAM ONLY when -fstrict-flex-array<3, when
-fstrict-flex-array=3 specified, [0] is NOT considered as an extension for FAM
anymore.
For [1], only when -fstrict-flex-array<2 spedified, it’s considered as an
extension for FAM.
So, I still think that we should consider -fstrict-flex-array and its impact on
the GCC extensions [0] and [1].
Qing
> I would make it
> compatible but not allow redefinition as the types are different.
>
>
> Martin
>
>
>>
>> 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");
>>
>>
>
