Re: [PATCH 2/2] Documentation Update.

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

> On Feb 2, 2023, at 3:33 AM, Richard Biener <rguenther@suse.de> wrote:
> 
> On Wed, 1 Feb 2023, Siddhesh Poyarekar wrote:
> 
>> On 2023-02-01 13:24, Qing Zhao wrote:
>>> 
>>> 
>>>> On Feb 1, 2023, at 11:55 AM, Siddhesh Poyarekar <siddhesh@gotplt.org>
>>>> wrote:
>>>> 
>>>> On 2023-01-31 09:11, Qing Zhao wrote:
>>>>> Update documentation to clarify a GCC extension on structure with
>>>>> flexible array member being nested in another structure.
>>>>> gcc/ChangeLog:
>>>>> * doc/extend.texi: Document GCC extension on a structure containing
>>>>> a flexible array member to be a member of another structure.
>>>> 
>>>> Should this resolve pr#77650 since the proposed action there appears to be
>>>> to document these semantics?
>>> 
>>> My understanding of pr77650 is specifically for documentation on the
>>> following case:
>>> 
>>> The structure with a flexible array member is the middle field of another
>>> structure.
>>> 
>>> Which I added in the documentation as the 2nd situation.
>>> However, I am still not very comfortable on my current clarification on this
>>> situation: how should we document on
>>> the expected gcc behavior to handle such situation?
>> 
>> I reckon wording that dissuades programmers from using this might be
>> appropriate, i.e. don't rely on this and if you already have such nested flex
>> arrays, change code to remove them.
>> 
>>>>> +In the above, @code{flex_data.data[]} is allowed to be extended flexibly
>>>>> to
>>>>> +the padding. E.g, up to 4 elements.
>> 
>> """
>> ... Relying on space in struct padding is bad programming practice and any
>> code relying on this behaviour should be modified to ensure that flexible
>> array members only end up at the ends of arrays.  The `-pedantic` flag should
>> help identify such uses.
>> """
>> 
>> Although -pedantic will also flag on flex arrays nested in structs even if
>> they're at the end of the parent struct, so my suggestion on the warning is
>> not really perfect.
> 
> Wow, so I checked and we indeed accept
> 
> struct X { int n; int data[]; };
> struct Y { struct X x; int end; };
> 
> and -pedantic says
> 
> t.c:2:21: warning: invalid use of structure with flexible array member 
> [-Wpedantic]
>    2 | struct Y { struct X x; int end; };
>      |    

Currently, -pedantic report the same message for flex arrays nested in structs at the end of the parent struct AND in the middle of the parent struct. 
Shall we distinguish them and report different warning messages in order to discourage the latter case? 

And at the same time, in the documentation, clarify these two situations, and discourage the latter case at the same time as well?
>       
> 
> and clang reports
> 
> t.c:2:21: warning: field 'x' with variable sized type 'struct X' not at 
> the end of a struct or class is a GNU extension 
> [-Wgnu-variable-sized-type-not-at-end]
> struct Y { struct X x; int end; };
>  
>                  ^

Clang’s warning message is clearer. 
> 
> looking at PR77650 what seems missing there is the semantics of this
> extension as expected/required by the glibc use.  comment#5 seems
> to suggest that for my example above its expected that
> Y.x.data[0] aliases Y.end?!

Should we mentioned this alias relationship in the doc?

>  There must be a better way to write
> the glibc code and IMHO it would be best to deprecate this extension.

Agreed. This is really a bad practice, should be deprecated. 
We can give warning first in this release, and then deprecate this extension in a latter release. 

> Definitely the middle-end wouldn't consider this aliasing for
> my example - maybe it "works" when wrapped inside a union but
> then for sure only when the union is visible in all accesses ...
> 
> typedef union
> {
>  struct __gconv_info __cd;
>  struct
>  {
>    struct __gconv_info __cd;
>    struct __gconv_step_data __data;
>  } __combined;
> } _G_iconv_t;
> 
> could be written as
> 
> typedef union
> {
>  struct __gconv_info __cd;
>  char __dummy[sizeof(struct __gconv_info) + sizeof(struct 
> __gconv_step_data)];
> } _G_iconv_t;
> 
> in case the intent is to provide a complete type with space for
> a single __gconv_step_data.

Since the current middle end doesn’t handle such case consistently, what should we document this case? 
Or just mentioned this case is not handled consistently in the compiler and will be deprecated in the future, 
 user should not depend on it and should rewrite their code?

I don’t think it worth the effort to update GCC to consistently handle this case in general.

What’s your opinion?

Qing


> 
> Richard.