Re: [PATCH 05/10] __builtin_dynamic_object_size: Recognize builtin

[Jakub Jelinek <jakub@redhat.com>]

On Wed, Nov 10, 2021 at 12:31:31AM +0530, Siddhesh Poyarekar wrote:
> Recognize the __builtin_dynamic_object_size builtin and add paths in the
> object size path to deal with it, but treat it like
> __builtin_object_size for now.  Also add tests to provide the same
> testing coverage for the new builtin name.
> 
> gcc/ChangeLog:
> 
> 	* builtins.def (BUILT_IN_DYNAMIC_OBJECT_SIZE): New builtin.
> 	* tree-object-size.h (compute_builtin_object_size): Add new
> 	argument dynamic.
> 	* builtins.c (expand_builtin, fold_builtin_2): Handle it.
> 	(fold_builtin_object_size): Handle new builtin and adjust for
> 	change to compute_builtin_object_size.
> 	* tree-object-size.c: Include builtins.h.
> 	(OST_DYNAMIC): New enum value.
> 	(compute_builtin_object_size): Add new argument dynamic.
> 	(addr_object_size): Adjust.
> 	(early_object_sizes_execute_one,
> 	dynamic_object_sizes_execute_one): New functions.
> 	(object_sizes_execute): Rename insert_min_max_p argument to
> 	early. Handle BUILT_IN_DYNAMIC_OBJECT_SIZE and call the new

Two spaces after . instead of just one.

> --- a/gcc/builtins.def
> +++ b/gcc/builtins.def
> @@ -972,6 +972,7 @@ DEF_BUILTIN_STUB (BUILT_IN_STRNCMP_EQ, "__builtin_strncmp_eq")
>  
>  /* Object size checking builtins.  */
>  DEF_GCC_BUILTIN	       (BUILT_IN_OBJECT_SIZE, "object_size", BT_FN_SIZE_CONST_PTR_INT, ATTR_CONST_NOTHROW_LEAF_LIST)
> +DEF_GCC_BUILTIN	       (BUILT_IN_DYNAMIC_OBJECT_SIZE, "dynamic_object_size", BT_FN_SIZE_CONST_PTR_INT, ATTR_NOTHROW_LEAF_LIST)

Are you sure about the omission of CONST_ in there?
If I do:
  size_t a = __builtin_dynamic_object_size (x, 0);
  size_t b = __builtin_dynamic_object_size (x, 0);
I'd expect the compiler to perform it just once.  While it might actually do
it eventually after objsz2 pass lowers it, with the above it won't really do
it.  Perhaps const attribute isn't really safe, the function might need to
read some memory in order to compute the return value, but certainly it will
not store to any memory, so perhaps
ATTR_PURE_NOTHROW_LEAF_LIST ?

> +#define DYNAMIC_OBJECT_SIZE

Why this extra macro?

> +#define __builtin_object_size __builtin_dynamic_object_size

>  extern char ax[];
> +#ifndef DYNAMIC_OBJECT_SIZE

You can #ifndef __builtin_object_size
instead...

> @@ -371,7 +373,8 @@ addr_object_size (struct object_size_info *osi, const_tree ptr,
>  	  || TREE_CODE (TREE_OPERAND (pt_var, 0)) != SSA_NAME)
>  	{
>  	  compute_builtin_object_size (TREE_OPERAND (pt_var, 0),
> -				       object_size_type & ~OST_SUBOBJECT, &sz);
> +				       object_size_type & OST_MINIMUM, &sz,
> +				       object_size_type & OST_DYNAMIC);
>  	}
>        else
>  	{
> @@ -835,9 +838,10 @@ resolve_dependency_loops (struct object_size_info *osi)
>  
>  bool
>  compute_builtin_object_size (tree ptr, int object_size_type,
> -			     tree *psize)
> +			     tree *psize, bool dynamic)
>  {
>    gcc_assert (object_size_type >= 0 && object_size_type < OST_END);
> +  object_size_type |= dynamic ? OST_DYNAMIC : 0;

What's the advantage of another argument and then merging it with
object_size_type over just passing object_size_type which will have
all the bits in?

> +static void
> +early_object_sizes_execute_one (gimple_stmt_iterator *i, gimple *call)
> +{
> +  tree ost = gimple_call_arg (call, 1);
> +  tree lhs = gimple_call_lhs (call);
> +  gcc_assert (lhs != NULL_TREE);
> +
> +  if (tree_fits_uhwi_p (ost))
> +    {
> +      unsigned HOST_WIDE_INT object_size_type = tree_to_uhwi (ost);
> +      tree ptr = gimple_call_arg (call, 0);
> +      if ((object_size_type == 1 || object_size_type == 3)
> +	  && (TREE_CODE (ptr) == ADDR_EXPR || TREE_CODE (ptr) == SSA_NAME))

I think it would be better to have early exits there to avoid
indenting most of the function too much, because the function doesn't
do anything otherwise.  So:
  if (!tree_fits_uhwi_p (ost))
    return;

  unsigned HOST_WIDE_INT object_size_type = tree_to_uhwi (ost);
  tree ptr = gimple_call_arg (call, 0);
  if (object_size_type != 1 && object_size_type != 3)
    return;
  if (TREE_CODE (ptr) != ADDR_EXPR && TREE_CODE (ptr) != SSA_NAME)
    return;

  tree type = ...

> +	{
> +	  tree type = TREE_TYPE (lhs);
> +	  tree bytes;
> +	  if (compute_builtin_object_size (ptr, object_size_type, &bytes))
> +	    {
> +	      tree tem = make_ssa_name (type);
> +	      gimple_call_set_lhs (call, tem);
> +	      enum tree_code code
> +		= object_size_type & OST_MINIMUM ? MAX_EXPR : MIN_EXPR;
> +	      tree cst = fold_convert (type, bytes);
> +	      gimple *g = gimple_build_assign (lhs, code, tem, cst);
> +	      gsi_insert_after (i, g, GSI_NEW_STMT);
> +	      update_stmt (call);
> +	    }
> +	}
> +    }

> +/* Attempt to fold one __builtin_dynamic_object_size call in CALL into an
> +   expression and insert it at I.  Return true if it succeeds.  */
> +
> +static bool
> +dynamic_object_sizes_execute_one (gimple_stmt_iterator *i, gimple *call)
> +{
> +  unsigned numargs = gimple_call_num_args (call);
> +  tree *args = XALLOCAVEC (tree, numargs);
> +  args[0] = gimple_call_arg (call, 0);
> +  args[1] = gimple_call_arg (call, 1);

Why it would have numargs different from 2?  It is a builtin, and we reject
((__SIZE_TYPE__ (*) (const void *, int, int)) &__builtin_object_size) (x, 0, 3)
etc. with error that the builtin must be directly called.
And after all, you rely on it having at least 2 arguments anyway.
So, just tree args[2]; instead of XALLOCAVEC and if you want,
gcc_assert (numargs == 2);
?

	Jakub