On Mon, 9 Jan 2023, Qing Zhao wrote:
>
>
> > On Jan 9, 2023, at 2:11 AM, Richard Biener <rguent...@suse.de> wrote:
> >
> > On Thu, 22 Dec 2022, Qing Zhao wrote:
> >
> >>
> >>
> >>> On Dec 22, 2022, at 2:09 AM, Richard Biener <rguent...@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 <rguent...@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.z...@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 <rguent...@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)
