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

[Jakub Jelinek <jakub@redhat.com>]

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)).

>  /* 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.
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.
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).

> @@ -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.

	Jakub