Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Richard Biener <richard.guenther@gmail.com>]

On Wed, Jul 6, 2022 at 4:20 PM Qing Zhao <qing.zhao@oracle.com> wrote:
>
> (Sorry for the late reply, just came back from a short vacation.)
>
> > On Jul 4, 2022, at 2:49 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> >
> > On Fri, Jul 1, 2022 at 5:32 PM Martin Sebor <msebor@gmail.com> wrote:
> >>
> >> On 7/1/22 08:01, Qing Zhao wrote:
> >>>
> >>>
> >>>> On Jul 1, 2022, at 8:59 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> >>>>
> >>>> On Fri, Jul 01, 2022 at 12:55:08PM +0000, Qing Zhao wrote:
> >>>>> If so, comparing to the current implemenation to have all the checking in middle-end, what’s the
> >>>>> major benefit of moving part of the checking into FE, and leaving the other part in middle-end?
> >>>>
> >>>> The point is recording early what FIELD_DECLs could be vs. can't possibly be
> >>>> treated like flexible array members and just use that flag in the decisions
> >>>> in the current routines in addition to what it is doing.
> >>>
> >>> Okay.
> >>>
> >>> Based on the discussion so far, I will do the following:
> >>>
> >>> 1. Add a new flag “DECL_NOT_FLEXARRAY” to FIELD_DECL;
> >>> 2. In C/C++ FE, set the new flag “DECL_NOT_FLEXARRAY” for a FIELD_DECL based on [0], [1],
> >>>     [] and the option -fstrict-flex-array, and whether it’s the last field of the DECL_CONTEXT.
> >>> 3. In Middle end,  Add a new utility routine is_flexible_array_member_p, which bases on
> >>>     DECL_NOT_FLEXARRAY + array_at_struct_end_p to decide whether the array
> >>>     reference is a real flexible array member reference.
> >
> > I would just update all existing users, not introduce another wrapper
> > that takes DECL_NOT_FLEXARRAY
> > into account additionally.
>
> Okay.
> >
> >>>
> >>>
> >>> Middle end currently is quite mess, array_at_struct_end_p, component_ref_size, and all the phases that
> >>> use these routines need to be updated, + new testing cases for each of the phases.
> >>>
> >>>
> >>> So, I still plan to separate the patch set into 2 parts:
> >>>
> >>>   Part A:    the above 1 + 2 + 3,  and use these new utilities in tree-object-size.cc to resolve PR101836 first.
> >>>                  Then kernel can use __FORTIFY_SOURCE correctly;
> >>>
> >>>   Part B:    update all other phases with the new utilities + new testing cases + resolving regressions.
> >>>
> >>> Let me know if you have any comment and suggestion.
> >>
> >> It might be worth considering whether it should be possible to control
> >> the "flexible array" property separately for each trailing array member
> >> via either a #pragma or an attribute in headers that can't change
> >> the struct layout but that need to be usable in programs compiled with
> >> stricter -fstrict-flex-array=N settings.
> >
> > Or an decl attribute.
>
> Yes, it might be necessary to add a corresponding decl attribute
>
> strict_flex_array (N)
>
> Which is attached to a trailing structure array member to provide the user a finer control when -fstrict-flex-array=N is specified.
>
> So, I will do the following:
>
>
> *****User interface:
>
> 1. command line option:
>      -fstrict-flex-array=N       (N=0, 1, 2, 3)
> 2.  decl attribute:
>      strict_flex_array (N)      (N=0, 1, 2, 3)
>
>
> *****Implementation:
>
> 1. Add a new flag “DECL_NOT_FLEXARRAY” to FIELD_DECL;
> 2. In C/C++ FE, set the new flag “DECL_NOT_FLEXARRAY” for a FIELD_DECL based on [0], [1],
>      [], the option -fstrict-flex-array, the attribute strict_flex_array,  and whether it’s the last field
>      of the DECL_CONTEXT.
> 3. In Middle end,   update all users of “array_at_struct_end_p" or “component_ref_size”, or any place that treats
>     Trailing array as flexible array member with the new flag  DECL_NOT_FLEXARRAY.
>     (Still think we need a new consistent utility routine here).
>
>
> I still plan to separate the patch set into 2 parts:
>
> Part A:    the above 1 + 2 + 3,  and use these new utilities in tree-object-size.cc to resolve PR101836 first.
>                Then kernel can use __FORTIFY_SOURCE correctly.
> Part B:    update all other phases with the new utilities + new testing cases + resolving regressions.
>
>
> Let me know any more comment or suggestion.

Sounds good.  Part 3. is "optimization" and reasonable to do
separately, I'm not sure you need
'B' (since we're not supposed to have new utilities), but instead I'd
do '3.' as part of 'B', just
changing the pieces th resolve PR101836 for part 'A'.

Richard.

> Thanks a lot.
>
> Qing
>
>