Re: gcc-13/changes.html: Mention -fstrict-flex-arrays and its impact

[Richard Biener <rguenther@suse.de>]

On Mon, 9 Jan 2023, Qing Zhao wrote:

> 
> 
> > On Jan 9, 2023, at 2:11 AM, Richard Biener <rguenther@suse.de> wrote:
> > 
> > On Thu, 22 Dec 2022, Qing Zhao wrote:
> > 
> >> 
> >> 
> >>> On Dec 22, 2022, at 2:09 AM, Richard Biener <rguenther@suse.de> wrote:
> >>> 
> >>> On Wed, 21 Dec 2022, Qing Zhao wrote:
> >>> 
> >>>> Hi, Richard,
> >>>> 
> >>>> Thanks a lot for your comments.
> >>>> 
> >>>>> On Dec 21, 2022, at 2:12 AM, Richard Biener <rguenther@suse.de> wrote:
> >>>>> 
> >>>>> On Tue, 20 Dec 2022, Qing Zhao wrote:
> >>>>> 
> >>>>>> Hi,
> >>>>>> 
> >>>>>> This is the patch for mentioning -fstrict-flex-arrays and -Warray-bounds=2 changes in gcc-13/changes.html.
> >>>>>> 
> >>>>>> Let me know if you have any comment or suggestions.
> >>>>> 
> >>>>> Some copy editing below
> >>>>> 
> >>>>>> Thanks.
> >>>>>> 
> >>>>>> Qing.
> >>>>>> 
> >>>>>> =======================================
> >>>>>> From c022076169b4f1990b91f7daf4cc52c6c5535228 Mon Sep 17 00:00:00 2001
> >>>>>> From: Qing Zhao <qing.zhao@oracle.com>
> >>>>>> Date: Tue, 20 Dec 2022 16:13:04 +0000
> >>>>>> Subject: [PATCH] gcc-13/changes: Mention -fstrict-flex-arrays and its impact.
> >>>>>> 
> >>>>>> ---
> >>>>>> htdocs/gcc-13/changes.html | 15 +++++++++++++++
> >>>>>> 1 file changed, 15 insertions(+)
> >>>>>> 
> >>>>>> diff --git a/htdocs/gcc-13/changes.html b/htdocs/gcc-13/changes.html
> >>>>>> index 689178f9..47b3d40f 100644
> >>>>>> --- a/htdocs/gcc-13/changes.html
> >>>>>> +++ b/htdocs/gcc-13/changes.html
> >>>>>> @@ -39,6 +39,10 @@ a work-in-progress.</p>
> >>>>>>   <li>Legacy debug info compression option <code>-gz=zlib-gnu</code> was removed
> >>>>>>     and the option is ignored right now.</li>
> >>>>>>   <li>New debug info compression option value <code>-gz=zstd</code> has been added.</li>
> >>>>>> +    <li><code>-Warray-bounds=2</code> will no longer issue warnings for out of bounds
> >>>>>> +      accesses to trailing struct members of one-element array type anymore. Please
> >>>>>> +      add <code>-fstrict-flex-arrays=level</code> to control how the compiler treat
> >>>>>> +      trailing arrays of structures as flexible array members. </li>
> >>>>> 
> >>>>> "Instead it diagnoses accesses to trailing arrays according to 
> >>>>> <code>-fstrict-flex-arrays</code>."
> >>>> 
> >>>> Okay.
> >>>>> 
> >>>>>> </ul>
> >>>>>> 
> >>>>>> 
> >>>>>> @@ -409,6 +413,17 @@ a work-in-progress.</p>
> >>>>>> <h2>Other significant improvements</h2>
> >>>>>> 
> >>>>>> <!-- <h3 id="uninitialized">Eliminating uninitialized variables</h3> -->
> >>>>>> +<h3 id="flexible array">Treating trailing arrays as flexible array members</h3>
> >>>>>> +
> >>>>>> +<ul>
> >>>>>> + <li>GCC can now control when to treat the trailing array of a structure as a 
> >>>>>> +     flexible array member for the purpose of accessing the elements of such
> >>>>>> +     an array. By default, all trailing arrays of structures are treated as
> >>>>> 
> >>>>> all trailing arrays in aggregates are treated
> >>>> Okay.
> >>>>> 
> >>>>>> +     flexible array members. Use the new command-line option
> >>>>>> +     <code>-fstrict-flex-array=level</code> to control how GCC treats the trailing
> >>>>>> +     array of a structure as a flexible array member at different levels.
> >>>>> 
> >>>>> <code>-fstrict-flex-arrays</code> to control which trailing array
> >>>>> members are streated as flexible arrays.
> >>>> 
> >>>> Okay.
> >>>> 
> >>>>> 
> >>>>> I've also just now noticed that there's now a flag_strict_flex_arrays
> >>>>> check in the middle-end (in array bound diagnostics) but this option
> >>>>> isn't streamed or handled with LTO.  I think you want to replace that
> >>>>> with the appropriate DECL_NOT_FLEXARRAY check.
> >>>> 
> >>>> We need to know the level value of the strict_flex_arrays on the struct 
> >>>> field to issue proper warnings at different levels. DECL_NOT_FLEXARRAY 
> >>>> does not include such info. So, what should I do? Streaming the 
> >>>> flag_strict_flex_arrays with LTO?
> >>> 
> >>> But you do
> >>> 
> >>> if (compref)
> >>>   {
> >>>     /* Try to determine special array member type for this 
> >>> COMPONENT_REF.  */
> >>>     sam = component_ref_sam_type (arg);
> >>>     /* Get the level of strict_flex_array for this array field.  */
> >>>     tree afield_decl = TREE_OPERAND (arg, 1);
> >>>     strict_flex_array_level = strict_flex_array_level_of (afield_decl);
> >>> 
> >>> I see that function doesn't look at DECL_NOT_FLEXARRAY but just
> >>> checks attributes (those are streamed in LTO).
> >> 
> >> Yes, checked both flag_strict_flex_arrays and attributes. 
> >> 
> >> There are two places in middle end calling ?strict_flex_array_level_of? function, 
> >> one inside ?array_bounds_checker::check_array_ref?, another one inside ?component_ref_size?.
> >> Shall we check DECL_NOT_FLEXARRAY field instead of calling ?strict_flex_array_level_of? in both places?
> > 
> > I wonder if that function should check DECL_NOT_FLEXARRAY?
> 
> The function ?strict_flex_array_level_of? is intended to query the LEVEL of strict_flex_array, only check DECL_NOT_FLEXARRAY is not enough. 
> 
> So, I think the major question here is: 
> 
> Do we need  the LEVEL of strict_flex_array information in the Middle end?
> 
> The current major use of LEVEL of strict_flex_array in the middle end is two places:
> 
> 	1. In the routine ?component_ref_size?: to determine the size of the trailing array based on the level of the strict_flex_array.
>         2. In the routine ?array_bounds_checker::check_array_ref?: to issue different information for -Wstrict-flex-array based on different level.
> 
> 
> Just double checked the above 1, and 2. Without LEVEL of strict_flex_array info, 1 should be fine
> 2, as you mentioned previously, the major impact will be that the LEVEL information is lost in the issued message, but that might be not a big
> issue.
> 
> So, I will try to eliminate the reference to ?flag_strict_flex_array? in the middle end, replace it with ?DECL_NOT_FLEXARRAY?, and come up with
> an updated patch for this change.
> 
> How do you think?

Yes, that sounds good.

> > 
> >>> 
> >>> OK, so I suppose the diagnostic itself would become just less precise
> >>> as in "trailing array %qT should not be used as a flexible array member"
> >>> without the "for level N and above" part of the diagnostic?
> >> 
> >> Yes, that might be the major impact.
> >> 
> >> If only check DECL_NOT_FLEXARRAY, we will lose such information. Does that matter?
> > 
> > I think the main information is preserved in diagnosing the flex vs.
> > non-flex array assumption.
> Yes. Agreed.
> 
> > 
> >>> 
> >>>>> We might also want
> >>>>> to see how inlining accesses from TUs with different -fstrict-flex-arrays
> >>>>> setting behaves when accessing the same structure (and whether we might
> >>>>> want to issue an ODR style diagnostic there).
> >>> 
> >>> This mixing also means streaming -fstrict-flex-arrays won't be of much
> >>> help in general.
> >> 
> >> Then under such situation, i.e, different -fstrict-flex-arrays levels for the same structure from different TUs, how should we handle it? 
> > 
> > I think in similar situations we try to DWIM, but in some cases it will
> > result in "garbage" behavior.  I don't think there's anything we can
> > do here besides trying to diagnose such mismatches with -flto at the WPA
> > stage.
> 
> Shall we issue warning for such mismatches? Where is the place I can add such warnings?

I'm not sure - we'd have to restrict it to "used" types and in principle
only when actual objects pass from one TU to another with different
flex-array semantics.  Otherwise we'll risk tons of diagnostics when
people "forget" -fstrict-flex-arrays on some TUs but pull in common
headers.

The C++ ODR diagnostics reside in ipa-devirt.cc, I'm not sure diagnostics
on flex arrays would fit there.

I just wanted to bring this up, I do not have a good idea how or where
to implement it.

Richard.

> thanks.
> 
> Qing
> > 
> > Richard.
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)