Re: [PATCH 1/2] Handle component_ref to a structre/union field including flexible array member [PR101832]

[Qing Zhao <qing.zhao@oracle.com>]

> On Feb 6, 2023, at 4:31 AM, Richard Biener <rguenther@suse.de> wrote:
> 
> On Fri, 3 Feb 2023, Qing Zhao wrote:
> 
>> 
>> 
>>> On Feb 3, 2023, at 2:49 AM, Richard Biener <rguenther@suse.de> wrote:
>>> 
>>> On Thu, 2 Feb 2023, Qing Zhao wrote:
>>> 
>>>> 
>>>> 
>>>>> On Feb 2, 2023, at 8:54 AM, Richard Biener <rguenther@suse.de> wrote:
>>>>> 
>>>>> On Thu, 2 Feb 2023, Qing Zhao wrote:
>>>>> 
>>>>>> 
>>>>>> 
>>> 
>>> [...]
>>> 
>>>>>>>>>> +	return flexible_size_type_p (TREE_TYPE (last));
>>>>>>>>> 
>>>>>>>>> For types with many members this can become quite slow (IIRC we had
>>>>>>>>> bugs about similar walks of all fields in types), and this function
>>>>>>>>> looks like it's invoked multiple times on the same type per TU.
>>>>>>>>> 
>>>>>>>>> In principle the property is fixed at the time we lay out a record
>>>>>>>>> type, so we might want to compute it at that time and record the
>>>>>>>>> result.
>>>>>>>> 
>>>>>>>> You mean in FE? 
>>>>>>> 
>>>>>>> Yes, either in the frontend or in the middle-ends layout_type.
>>>>>>> 
>>>>>>>> Yes, that?s better and cleaner.
>>>>>>>> 
>>>>>>>> I will add one more field in the TYPE structure to record this information and check this field during middle end.
>>>>>>>> 
>>>>>>>> I had the same thought in the beginning, but not sure whether adding a 
>>>>>>>> new field in IR is necessary or not, other places in middle end might 
>>>>>>>> not use this new field.
>>>>>>> 
>>>>>>> It might be interesting to search for other code walking all fields of
>>>>>>> a type to determine this or similar info.
>>>>>> 
>>>>>> There is one which is defined in tree.cc but only is referenced in c/c-decl.cc:
>>>>>> 
>>>>>> /* Determine whether TYPE is a structure with a flexible array member,
>>>>>> or a union containing such a structure (possibly recursively).  */
>>>>>> flexible_array_type_p
>>>>>> 
>>>>>> However, this routine is a little different than the one I tried to add:
>>>>>> 
>>>>>> In the current routine ?flexible_array_type_p?,  only one level nesting in the structure is accepted, multiple nesting in structure is not permitted.
>>>>>> 
>>>>>> So, my question is:  shall we accept multiple nesting in structure? i.e.
>>>>> 
>>>>> If we don't reject the testcase with an error, then yes.
>>>> 
>>>> Gcc currently accepts the multiple nesting in structure without error.  
>>>> So, we will continue to accept such extension as long as the flex array 
>>>> is at the end of the structure. At the same time, for the case the flex 
>>>> array is in the middle of the structure, issue additional warnings now 
>>>> to discourage such usage, and deprecate this case in a future release.
>>>> 
>>>> Does this sound reasonable? 
>>> 
>>> Please don't mix several issues - I think the flex array in the
>>> middle of a structure is separate and we shouldn't report that
>>> as flexible_array_type_p or flexible_size_type_p since the size
>>> of the containing structure is not variable.
>> Agreed on this.
>> 
>> My major question here is (for documentation change, sorry for mixing 
>> this thread with the documentation change): do we need to document this 
>> case together with the case in which struct with flex array is embedded 
>> into another structure? (As a GCC extension?)
> 
> I think this should be Josephs call - documenting this might
> encourage people to use such an extension, even if it's a bad
> one we want to get rid of.
That’s true...
> 
> Maybe the easiest thing is to come up with a patch documenting it
> which we can then turn into a deprecation note depending on this
> outcome.

In the other thread for the documentation change, I have listed a plan based on the discussion.
 Could you please take a look at it and provide me some comments in that thread? (I just copied my 
plan below for your convenience)

Thanks.

Qing

==================

In GCC13:

1. Add documentation in extend.texi to include all the following 3 cases as GCC extension:

Case 1: The structure with a flexible array member is the last field of another
structure, for example:

struct flex  { int length; char data[]; }
struct out_flex { int m; struct flex flex_data; }

In the above, flex_data.data[] is considered as a flexible array too.

Case 2: The structure with a flexible array member is the field of another union, for example:

struct flex1  { int length1; char data1[]; }
struct flex2  { int length2; char data2[]; }
union out_flex { struct flex1 flex_data1; struct flex2 flex_data2; }

In the above, flex_data1.data1[] or flex_data2.data2[] is considered as flexible arrays too.

Case 3: The structure with a flexible array member is the middle field of another
structure, for example:

struct flex  { int length; char data[]; }
struct out_flex { int m; struct flex flex_data; int n; }

In the above, flex_data.data[] is allowed to be extended flexibly to
the padding. E.g, up to 4 elements.

However, relying on space in struct padding is a bad programming practice,  compilers do not 
handle such extension consistently, and any code relying on this behavior should be modified
to ensure that flexible array members only end up at the ends of structures.

Please use warning option -Wgnu-variable-sized-type-not-at-end (to be consistent with CLANG) 
to identify all such cases in the source code and modify them. This extension will be deprecated
from gcc in the next release.

2. Add a new warning option -Wgnu-varaible-sized-type-not-at-end to warn such usage.

In GCC14:

1. Include this new warning -Wgnu-varaible-sized-type-not-at-end to -Wall
2. Deprecate this extension from GCC. (Or delay this to next release?).


> 
> Richard.