Re: [PATCH] tree-object-size: Support strndup and strdup

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

On 2022-09-22 09:02, Jakub Jelinek wrote:
> On Mon, Aug 15, 2022 at 03:23:11PM -0400, Siddhesh Poyarekar wrote:
>> --- a/gcc/tree-object-size.cc
>> +++ b/gcc/tree-object-size.cc
>> @@ -495,6 +495,18 @@ decl_init_size (tree decl, bool min)
>>     return size;
>>   }
>>   
>> +/* Get the outermost object that PTR may point into.  */
>> +
>> +static tree
>> +get_whole_object (const_tree ptr)
>> +{
>> +  tree pt_var = TREE_OPERAND (ptr, 0);
>> +  while (handled_component_p (pt_var))
>> +    pt_var = TREE_OPERAND (pt_var, 0);
>> +
>> +  return pt_var;
>> +}
> 
> Not sure why you want a new function for this.
> This is essentially get_base_address (TREE_OPERAND (ptr, 0)).

Oh, so can addr_object_size be simplified to use get_base_address too?

>>   /* Compute __builtin_object_size for PTR, which is a ADDR_EXPR.
>>      OBJECT_SIZE_TYPE is the second argument from __builtin_object_size.
>>      If unknown, return size_unknown (object_size_type).  */
>> +  if (!size_valid_p (sz, object_size_type)
>> +       || size_unknown_p (sz, object_size_type))
>> +    {
>> +      tree wholesrc = NULL_TREE;
>> +      if (TREE_CODE (src) == ADDR_EXPR)
>> +	wholesrc = get_whole_object (src);
>> +
>> +      if (!(object_size_type & OST_MINIMUM)
>> +	  || (wholesrc && TREE_CODE (wholesrc) == STRING_CST))
> 
> Is this safe?  I mean get_whole_object will also skip ARRAY_REFs with
> variable indexes etc. and the STRING_CST could have embedded '\0's
> in it.
> Even if c_strlen (src, 1) is constant, I don't see what you can assume
> for object size of strndup ("abcd\0efgh", n); for minimum, except 1.

Can't we assume MIN(5, n) for STRING_CST?

For ARRAY_REFs, it may end up being MIN(array_size, n) and not account 
for the NUL termination but I was thinking of that as being a better 
option than bailing out.  Should we try harder here and return, e.g. 
strlen or some equivalent?

> But on the other side, 1 is a safe minimum for OST_MINIMUM of both
> strdup and strndup if you don't find anything more specific (exact strlen
> for strndup) because the terminating '\0' will be always there.

OK, I can return size_one_node as the final return value for OST_MINIMUM 
if we don't find a suitable expression.

> Other than that you'd need to consider INTEGER_CST second strndup argument
> or ranges of the second argument etc.
> E.g. maximum for OST_DYNAMIC could be for strndup (src, n)
> MIN (__bdos (src, ?), n + 1).

Yeah, that's what I return in the end:

   return fold_build2 (MIN_EXPR, sizetype,
                      fold_build2 (PLUS_EXPR, sizetype, size_one_node,n),
                      sz);

where sz is __bdos(src)

> 
>> @@ -2113,7 +2177,7 @@ const pass_data pass_data_object_sizes =
>>     PROP_objsz, /* properties_provided */
>>     0, /* properties_destroyed */
>>     0, /* todo_flags_start */
>> -  0, /* todo_flags_finish */
>> +  TODO_update_ssa_no_phi, /* todo_flags_finish */
>>   };
>>   
>>   class pass_object_sizes : public gimple_opt_pass
>> @@ -2153,7 +2217,7 @@ const pass_data pass_data_early_object_sizes =
>>     0, /* properties_provided */
>>     0, /* properties_destroyed */
>>     0, /* todo_flags_start */
>> -  0, /* todo_flags_finish */
>> +  TODO_update_ssa_no_phi, /* todo_flags_finish */
>>   };
> 
> This is quite expensive.  Do you really need full ssa update, or just
> TODO_update_ssa_only_virtuals would be enough (is it for the missing
> vuse on the strlen call if you emit it)?
> In any case, would be better not to do that always, but only if you
> really need it (emitted the strlen call somewhere; e.g. if __bdos is
> never used, only __bos, it is certainly not needed), todo flags
> can be both in todo_flags_finish and in return value from execute method.

Thanks, I'll find a cheaper way to do this.

Thanks,
Sid