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

[Richard Biener <rguenther@suse.de>]

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.

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.

Richard.