[v5,2/4] tree-object-size: Handle function parameters

Handle hints provided by __attribute__ ((access (...))) to compute
dynamic sizes for objects.

gcc/ChangeLog:

	* tree-object-size.c: Include tree-dfa.h.
	(parm_object_size): New function.
	(collect_object_sizes_for): Call it.

gcc/testsuite/ChangeLog:

	* gcc.dg/builtin-dynamic-object-size-0.c (test_parmsz_simple):
	New function.
	(main): Call it.

Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org>
---
 .../gcc.dg/builtin-dynamic-object-size-0.c    | 11 ++++
 gcc/tree-object-size.c                        | 50 ++++++++++++++++++-
 2 files changed, 60 insertions(+), 1 deletion(-)

On Sat, Dec 18, 2021 at 06:05:09PM +0530, Siddhesh Poyarekar wrote:
> @@ -1440,6 +1441,53 @@ cond_expr_object_size (struct object_size_info *osi, tree var, gimple *stmt)
>    return reexamine;
>  }
>  
> +/* Find size of an object passed as a parameter to the function.  */
> +
> +static void
> +parm_object_size (struct object_size_info *osi, tree var)
> +{
> +  int object_size_type = osi->object_size_type;
> +  tree parm = SSA_NAME_VAR (var);
> +
> +  if (!(object_size_type & OST_DYNAMIC) || !POINTER_TYPE_P (TREE_TYPE (parm)))
> +    expr_object_size (osi, var, parm);

This looks very suspicious.  Didn't you mean { expr_object_size (...); return; } here?
Because the code below e.g. certainly assumes OST_DYNAMIC and that TREE_TYPE (parm)
is a pointer type (otherwise TREE_TYPE (TREE_TYPE (...) wouldn't work.

> +
> +  /* Look for access attribute.  */
> +  rdwr_map rdwr_idx;
> +
> +  tree fndecl = cfun->decl;
> +  const attr_access *access = get_parm_access (rdwr_idx, parm, fndecl);
> +  tree typesize = TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (parm)));
> +  tree sz = NULL_TREE;
> +
> +  if (access && access->sizarg != UINT_MAX)

Perhaps && typesize here?  It makes no sense to e.g. create ssa default def
when you aren't going to use it in any way.

> +    {
> +      tree fnargs = DECL_ARGUMENTS (fndecl);
> +      tree arg = NULL_TREE;
> +      unsigned argpos = 0;
> +
> +      /* Walk through the parameters to pick the size parameter and safely
> +	 scale it by the type size.  */
> +      for (arg = fnargs; argpos != access->sizarg && arg;
> +	   arg = TREE_CHAIN (arg), ++argpos);

Instead of a loop with empty body wouldn't it be better to
do the work in that for loop?
I.e. take argpos != access->sizarg && from the condition,
replace arg != NULL_TREE with that argpos == access->sizarg
and add a break;?

> +
> +      if (arg != NULL_TREE && INTEGRAL_TYPE_P (TREE_TYPE (arg)))
> +	{
> +	  sz = get_or_create_ssa_default_def (cfun, arg);

Also, I must say I'm little bit worried about this
get_or_create_ssa_default_def call.  If the SSA_NAME doesn't exist,
so you create it and then attempt to use it but in the end don't
because e.g. some PHI's another argument was unknown etc., will
that SSA_NAME be released through release_ssa_name?
I think GIMPLE is fairly unhappy if there are SSA_NAMEs created and not
released that don't appear in the IL anywhere.

> +	  if (sz != NULL_TREE)
> +	    {
> +	      sz = fold_convert (sizetype, sz);
> +	      if (typesize)
> +		sz = size_binop (MULT_EXPR, sz, typesize);
> +	    }
> +	}
> +    }

	Jakub

On 10/01/2022 16:20, Jakub Jelinek wrote:
> On Sat, Dec 18, 2021 at 06:05:09PM +0530, Siddhesh Poyarekar wrote:
>> @@ -1440,6 +1441,53 @@ cond_expr_object_size (struct object_size_info *osi, tree var, gimple *stmt)
>>     return reexamine;
>>   }
>>   
>> +/* Find size of an object passed as a parameter to the function.  */
>> +
>> +static void
>> +parm_object_size (struct object_size_info *osi, tree var)
>> +{
>> +  int object_size_type = osi->object_size_type;
>> +  tree parm = SSA_NAME_VAR (var);
>> +
>> +  if (!(object_size_type & OST_DYNAMIC) || !POINTER_TYPE_P (TREE_TYPE (parm)))
>> +    expr_object_size (osi, var, parm);
> 
> This looks very suspicious.  Didn't you mean { expr_object_size (...); return; } here?
> Because the code below e.g. certainly assumes OST_DYNAMIC and that TREE_TYPE (parm)
> is a pointer type (otherwise TREE_TYPE (TREE_TYPE (...) wouldn't work.

Indeed, fixed.

> 
>> +
>> +  /* Look for access attribute.  */
>> +  rdwr_map rdwr_idx;
>> +
>> +  tree fndecl = cfun->decl;
>> +  const attr_access *access = get_parm_access (rdwr_idx, parm, fndecl);
>> +  tree typesize = TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (parm)));
>> +  tree sz = NULL_TREE;
>> +
>> +  if (access && access->sizarg != UINT_MAX)
> 
> Perhaps && typesize here?  It makes no sense to e.g. create ssa default def
> when you aren't going to use it in any way.

The typesize is only for scaling; the result of 
get_or_create_ssa_default_def should get returned unscaled if it is 
non-NULL and typesize is NULL; the latter happens when the type is void *:

	  sz = get_or_create_ssa_default_def (cfun, arg);
	  if (sz != NULL_TREE)
	    {
	      sz = fold_convert (sizetype, sz);
	      if (typesize)
		sz = size_binop (MULT_EXPR, sz, typesize);
	    }
	}

> 
>> +    {
>> +      tree fnargs = DECL_ARGUMENTS (fndecl);
>> +      tree arg = NULL_TREE;
>> +      unsigned argpos = 0;
>> +
>> +      /* Walk through the parameters to pick the size parameter and safely
>> +	 scale it by the type size.  */
>> +      for (arg = fnargs; argpos != access->sizarg && arg;
>> +	   arg = TREE_CHAIN (arg), ++argpos);
> 
> Instead of a loop with empty body wouldn't it be better to
> do the work in that for loop?
> I.e. take argpos != access->sizarg && from the condition,
> replace arg != NULL_TREE with that argpos == access->sizarg
> and add a break;?

Fixed.

> 
>> +
>> +      if (arg != NULL_TREE && INTEGRAL_TYPE_P (TREE_TYPE (arg)))
>> +	{
>> +	  sz = get_or_create_ssa_default_def (cfun, arg);
> 
> Also, I must say I'm little bit worried about this
> get_or_create_ssa_default_def call.  If the SSA_NAME doesn't exist,
> so you create it and then attempt to use it but in the end don't
> because e.g. some PHI's another argument was unknown etc., will
> that SSA_NAME be released through release_ssa_name?
> I think GIMPLE is fairly unhappy if there are SSA_NAMEs created and not
> released that don't appear in the IL anywhere.

AFAICT, set_ss_default_def ends up creating a definition for the new 
SSA_NAME it creates, so it does end up in the IR and in case of object 
size computation failure, it just ends up being a dead store.  I've 
added a test to verify this:

size_t
__attribute__ ((access (__read_write__, 1, 3)))
__attribute__ ((noinline))
test_parmsz_unknown (void *obj, void *unknown, size_t sz, int cond)
{
   return __builtin_dynamic_object_size (cond ? obj : unknown, 0);
}

which works as expected and returns -1.

Thanks,
Siddhesh