Re: [PATCH 2/2] tree-object-size: More consistent behaviour with flex arrays

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

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 <siddhesh@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?

>> +	  /* 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