[PATCH 05/10] __builtin_dynamic_object_size: Recognize builtin

[Siddhesh Poyarekar]

On 11/23/21 18:11, Jakub Jelinek wrote:
> 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 ?

Thanks, I'll fix this.

> 
>> +#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...

I'll fix this too.

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

I kept the size bits as an internal detail, I can define them in 
tree-object-size.h and hae builtins.c (and others) use them.

>> +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;

OK, I'll fix this.

> 
>    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);
> ?

Thanks, I'll fix this too.

Siddhesh