> On Jan 26, 2023, at 12:16 PM, Siddhesh Poyarekar <siddh...@gotplt.org> wrote:
>
> On 2023-01-26 11:20, Qing Zhao wrote:
>> Hi, Siddhesh,
>> Thanks a lot for this patch, after -fstrict-flex-array functionality has
>> been added into GCC,
>> I think that making the tree-object-size to have consistent behavior with
>> flex arrays is a
>> valuable and natural work that need to be added.
>> I also like the comments you added into tree-object-size.cc, making the code
>> much easier to be understood.
>> Minor comments below:
>>> On Dec 21, 2022, at 5:25 PM, Siddhesh Poyarekar <siddh...@gotplt.org> wrote:
>>>
>>> The tree object size pass tries to fail when it detects a flex array in
>>> the struct, but it ends up doing the right thing only when the flex
>>> array is in the outermost struct. For nested cases (such as arrays
>>> nested in a union or an inner struct), it ends up taking whatever value
>>> the flex array is declared with, using zero for the standard flex array,
>>> i.e. [].
>>>
>>> Rework subobject size computation to make it more consistent across the
>>> board, honoring -fstrict-flex-arrays. With this change, any array at
>>> the end of the struct will end up causing __bos to use the allocated
>>> value of the outer object, bailing out in the maximum case when it can't
>>> find it. In the minimum case, it will return the subscript value or the
>>> allocated value of the outer object, whichever is larger.
>> I see from the changes in the testing case, there are the following major
>> changes for the existing behavior (can be show with the testing case)
>> ****For non-nested structures:
>> struct A
>> {
>> char a[10];
>> int b;
>> char c[10];
>> };
>> 1. The Minimum size of the reference to the subobject that is a trailing
>> array of a structure is changed from “0” to “sizeof the subobject"
>>> - if (__builtin_object_size (&p->c, 3) != 0)
>> + if (__builtin_object_size (&p->c, 3) != 10)
>> ****For nested structures:
>> struct D
>> {
>> int i;
>> struct D1
>> {
>> char b;
>> char a[10];
>> } j;
>> };
>> 2. The Maximum size of the reference to the subobject that is a trailing
>> array of the inner structure is changed from “sizeof the subobject” to “-1"
>>> - if (__builtin_object_size (&d->j.a[3], 1) != sizeof (d->j.a) - 3)
>>> + if (__builtin_object_size (&d->j.a[3], 1) != (size_t) -1)
>> .
>> 3. The Minimum size of the reference to the subobject that is a trailing
>> array of the inner structure is changed from “0” to “sizeof the subobject"
>> - if (__builtin_object_size ((char *) &e->j[0], 3) != 0)
>>> + if (__builtin_object_size ((char *) &e->j[0], 3) != sizeof (e->j))
>
> All of the above is correct, thanks for the high level review!
>
>> I think that all the above changes are good. My only concern is, for the
>> change of the Minimum size of the reference to the subobject that is a
>> trailing array (the above case 1 and 3), will there be any negtive impact on
>> the existing application that use it?
>
> I doubt it, because the 0 return value for minimum object size is essentially
> a failure to determine minimum object size, i.e. a special value. This
> change can be interpreted as succeeding to get the minimum object size in
> more cases.
>
> Likewise for the maximum object size change, the change can be interpreted as
> failing to get the maximum object size in more cases. Does that sound
> reasonable?
Yes, make sense to me.
Thanks.
Qing
>
>>> + /* If the subobject size cannot be easily inferred or is smaller than
>>> + the whole size, just use the whole size. */
>> Should the above comment be:
>> + /* If the subobject size cannot be easily inferred or is larger than
>> + the whole size, just use the whole size. */
>>> if (! TYPE_SIZE_UNIT (TREE_TYPE (var))
>>> || ! tree_fits_uhwi_p (TYPE_SIZE_UNIT (TREE_TYPE (var)))
>>> || (pt_var_size && TREE_CODE (pt_var_size) == INTEGER_CST
>>> && tree_int_cst_lt (pt_var_size,
>>> TYPE_SIZE_UNIT (TREE_TYPE (var)))))
>>> var = pt_var;
>
> Oops, yes indeed, fixed in my copy.
>
> Thanks,
> Sid
