> On Jan 10, 2023, at 3:06 AM, Richard Biener <rguent...@suse.de> wrote:
>
> 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.
Will do that.
>
>>>
>>>>>
>>>>> 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.
Thanks for the information. Will study this a little bit later.
Qing
>
> 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)
