Re: [PATCH] tree-object-size: Always set computed bit for bdos [PR113012]

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

On 2023-12-19 17:57, Jakub Jelinek wrote:
> On Mon, Dec 18, 2023 at 11:42:52AM -0500, Siddhesh Poyarekar wrote:
>> It is always safe to set the computed bit for dynamic object sizes at
>> the end of collect_object_sizes_for because even in case of a dependency
>> loop encountered in nested calls, we have an SSA temporary to actually
>> finish the object size expression.  The reexamine pass for dynamic
>> object sizes is only for propagation of unknowns and gimplification of
>> the size expressions, not for loop resolution as in the case of static
>> object sizes.
>>
>> gcc/ChangeLog:
>>
>> 	PR tree-optimization/113012
>> 	* gcc.dg/ubsan/pr113012.c: New test case.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR tree-optimization/113012
>> 	* tree-object-size.cc (compute_builtin_object_size): Expand
>> 	comment for dynamic object sizes.
>> 	(collect_object_sizes_for): Always set COMPUTED bitmap for
>> 	dynamic object sizes.
> 
> You have the gcc/ChangeLog and gcc/testsuite/ChangeLog hunks swapped,
> I think this wouldn't get through pre-commit hook.

Oops, fixed.

>> --- a/gcc/tree-object-size.cc
>> +++ b/gcc/tree-object-size.cc
>> @@ -1185,10 +1185,12 @@ compute_builtin_object_size (tree ptr, int object_size_type,
>>   	  osi.tos = NULL;
>>   	}
>>   
>> -      /* First pass: walk UD chains, compute object sizes that
>> -	 can be computed.  osi.reexamine bitmap at the end will
>> -	 contain what variables were found in dependency cycles
>> -	 and therefore need to be reexamined.  */
>> +      /* First pass: walk UD chains, compute object sizes that can be computed.
>> +	 osi.reexamine bitmap at the end will contain what variables that need
> 
> This wording is weird.
> Perhaps ... will contain versions of SSA_NAMEs that need
> to be reexamined. ?
> Because varno seems to be SSA_NAME_VERSION.

Ack, it's unrelated to my change, but fixed since I touched the comment 
block.

>> @@ -1823,11 +1825,16 @@ collect_object_sizes_for (struct object_size_info *osi, tree var)
>>         gcc_unreachable ();
>>       }
>>   
>> -  if (! reexamine || object_sizes_unknown_p (object_size_type, varno))
>> +  /* Dynamic sizes use placeholder temps to return an answer, so it is always
>> +   * safe to set COMPUTED for them.  */
> 
> We don't use this style of comments, please replace the * at the start of
> second line with a space.

Oops, fixed.

>> +  if ((object_size_type & OST_DYNAMIC)
>> +      || !reexamine || object_sizes_unknown_p (object_size_type, varno))
>>       {
>>         bitmap_set_bit (computed[object_size_type], varno);
>>         if (!(object_size_type & OST_DYNAMIC))
>>   	bitmap_clear_bit (osi->reexamine, varno);
>> +      else if (reexamine)
>> +	bitmap_set_bit (osi->reexamine, varno);
>>       }
>>     else
>>       {
> 
> Otherwise LGTM, but please wait at least a few weeks before backporting to
> 13.

OK, I'll push with fixes to trunk in a bit and then push to 13 next year.

Thanks,
Sid