Re: [PATCH v4 3/6] tree-object-size: Support dynamic sizes in conditions

[Jakub Jelinek <jakub@redhat.com>]

On Wed, Dec 01, 2021 at 07:57:54PM +0530, Siddhesh Poyarekar wrote:
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c
> @@ -0,0 +1,72 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2" } */
> +
> +typedef __SIZE_TYPE__ size_t;
> +#define abort __builtin_abort
> +
> +size_t
> +__attribute__ ((noinline))
> +test_builtin_malloc_condphi (int cond)
> +{
> +  void *ret;
> + 
> +  if (cond)
> +    ret = __builtin_malloc (32);
> +  else
> +    ret = __builtin_malloc (64);
> +
> +  return __builtin_dynamic_object_size (ret, 0);
> +}
> +
> +size_t
> +__attribute__ ((noinline))
> +test_builtin_calloc_condphi (size_t cnt, size_t sz, int cond)
> +{
> +  struct
> +    {
> +      int a;
> +      char b;
> +    } bin[cnt];
> +
> +  char *ch = __builtin_calloc (cnt, sz);
> +
> +  return __builtin_dynamic_object_size (cond ? ch : (void *) &bin, 0);

I think it would be nice if the testcases didn't leak memory, can
you replace return ... with size_t ret = 
and add
  __builtin_free (ch);
  return ret;
in both cases (in the first perhaps rename ret to ch first.

> --- a/gcc/testsuite/gcc.dg/builtin-object-size-5.c
> +++ b/gcc/testsuite/gcc.dg/builtin-object-size-5.c
> @@ -1,5 +1,7 @@
>  /* { dg-do compile { target i?86-*-linux* i?86-*-gnu* x86_64-*-linux* } } */
>  /* { dg-options "-O2" } */
> +/* For dynamic object sizes we 'succeed' if the returned size is known for
> +   maximum object size.  */
>  
>  typedef __SIZE_TYPE__ size_t;
>  extern void abort (void);
> @@ -13,7 +15,11 @@ test1 (size_t x)
>  
>    for (i = 0; i < x; ++i)
>      p = p + 4;
> +#ifdef __builtin_object_size
> +  if (__builtin_object_size (p, 0) == -1)
> +#else
>    if (__builtin_object_size (p, 0) != sizeof (buf) - 8)
> +#endif
>      abort ();
>  }
>  
> @@ -25,10 +31,15 @@ test2 (size_t x)
>  
>    for (i = 0; i < x; ++i)
>      p = p + 4;
> +#ifdef __builtin_object_size
> +  if (__builtin_object_size (p, 1) == -1)
> +#else
>    if (__builtin_object_size (p, 1) != sizeof (buf) - 8)
> +#endif
>      abort ();
>  }

I'd say for __bdos it would be better to rewrite the testcase
as dg-do run, perhaps use somewhat smaller buffer (say 16 times smaller;
and dg-additional-sources for a file that actually defines that buffer
and main.  Perhaps you can have those
#ifdef __builtin_object_size
  if (__builtin_object_size (p, 0) != sizeof (buf) - 8 - 4 * x)
#else
in there, just in the wrapper that #define __builtin_object_size
make it dg-do run and have dg-additional-sources (and
#ifndef N
#define N 0x40000000
#endif
and use that as size of buf.

> +	gcc_checking_assert (is_gimple_variable (ret)

This should be TREE_CODE (ret) == SSA_NAME
The reason why is_gimple_variable accepts VAR_DECLs/PARM_DECLs/RESULT_DECLs
is high vs. low gimple, but size_type_node sizes are gimple types and
both objsz passes are run when in ssa form, so it should always be either
a SSA_NAME or INTEGER_CST.

> +			     || TREE_CODE (ret) == INTEGER_CST);
> +    }
> +
> +  return ret;
>  }
>  
>  /* Set size for VARNO corresponding to OSI to VAL.  */
> @@ -176,27 +218,113 @@ object_sizes_initialize (struct object_size_info *osi, unsigned varno,
>    object_sizes[object_size_type][varno].wholesize = wholeval;
>  }
>  
> +/* Return a MODIFY_EXPR for cases where SSA and EXPR have the same type.  The
> +   TREE_VEC is returned only in case of PHI nodes.  */
> +
> +static tree
> +bundle_sizes (tree ssa, tree expr)
> +{
> +  gcc_checking_assert (TREE_TYPE (ssa) == sizetype);
> +
> +  if (!TREE_TYPE (expr))
> +    {
> +      gcc_checking_assert (TREE_CODE (expr) == TREE_VEC);

I think I'd prefer to do it the other way, condition on TREE_CODE (expr) == TREE_VEC
and if needed assert it has NULL TREE_TYPE.

> +      TREE_VEC_ELT (expr, TREE_VEC_LENGTH (expr) - 1) = ssa;
> +      return expr;
> +    }
> +
> +  gcc_checking_assert (types_compatible_p (TREE_TYPE (expr), sizetype));
> +  return size_binop (MODIFY_EXPR, ssa, expr);

This looks wrong.  MODIFY_EXPR isn't a binary expression
(tcc_binary/tcc_comparison), size_binop shouldn't be called on it.
I think you even don't want to fold it, so
  return build2 (MODIFY_EXPR, sizetype, ssa, expr);
?
Also, calling a parameter or var ssa is quite unusual, normally
one calls a SSA_NAME either name, or ssa_name etc.

> +	  gcc_checking_assert (size_initval_p (oldval, object_size_type));
> +	  gcc_checking_assert (size_initval_p (old_wholeval,
> +					       object_size_type));
> +	  /* For dynamic object sizes, all object sizes that are not gimple
> +	     variables will need to be gimplified.  */
> +	  if (TREE_CODE (wholeval) != INTEGER_CST
> +	      && !is_gimple_variable (wholeval))
> +	    {
> +	      bitmap_set_bit (osi->reexamine, varno);
> +	      wholeval = bundle_sizes (make_ssa_name (sizetype), wholeval);
> +	    }
> +	  if (TREE_CODE (val) != INTEGER_CST && !is_gimple_variable (val))

Again twice above.

> +/* Set temporary SSA names for object size and whole size to resolve dependency
> +   loops in dynamic size computation.  */
> +
> +static inline void
> +object_sizes_set_temp (struct object_size_info *osi, unsigned varno)
> +{
> +  tree val = object_sizes_get (osi, varno);
> +
> +  if (size_initval_p (val, osi->object_size_type))
> +    object_sizes_set (osi, varno,
> +		      make_ssa_name (sizetype),
> +		      make_ssa_name (sizetype));

This makes me a little bit worried.  Do you compute the wholesize SSA_NAME
at runtime always, or only when it is really needed and known not to always
be equal to the size?
I mean, e.g. for the cases where there is just const char *p = malloc (size);
and the pointer is never increased size == wholesize.  For __bos it will
just be 2 different INTEGER_CSTs, but if it would at runtime mean we compute
something twice and hope we eventually find out during later passes
it is the same, it would be bad.

> +  tree phires = TREE_VEC_ELT (wholesize, TREE_VEC_LENGTH (wholesize) - 1);
> +  gphi *wholephi = create_phi_node (phires, gimple_bb (stmt));
> +  phires = TREE_VEC_ELT (size, TREE_VEC_LENGTH (size) - 1);
> +  gphi *phi = create_phi_node (phires, gimple_bb (stmt));
> +  gphi *obj_phi =  as_a <gphi *> (stmt);

Just one space instead of 2 above.
And the above shows that you actually create 2 PHIs unconditionally,
rather than trying to do that only if 1) wholesize is ever actually
different from size 2) something needs wholesize.

> +      /* If we built an expression, we will need to build statements
> +	 and insert them on the edge right away.  */
> +      if (!is_gimple_variable (wsz))

Again, above comments about is_gimple_variable.

> +	wsz = force_gimple_operand (wsz, &seq, true, NULL);
> +      if (!is_gimple_variable (sz))
> +	{
> +	  gimple_seq s;
> +	  sz = force_gimple_operand (sz, &s, true, NULL);
> +	  gimple_seq_add_seq (&seq, s);
> +	}
> +
> +      if (seq)
> +	{
> +	  edge e = gimple_phi_arg_edge (obj_phi, i);
> +
> +	  /* Put the size definition before the last statement of the source
> +	     block of the PHI edge.  This ensures that any branches at the end
> +	     of the source block remain the last statement.  We are OK even if
> +	     the last statement is the definition of the object since it will
> +	     succeed any definitions that contribute to its size and the size
> +	     expression will succeed them too.  */
> +	  gimple_stmt_iterator gsi = gsi_last_bb (e->src);
> +	  gsi_insert_seq_before (&gsi, seq, GSI_CONTINUE_LINKING);

This looks problematic.  The last stmt in the bb might not exist at all,
or can be one that needs to terminate the bb (stmt_ends_bb_p), or can be
some other normal stmt that is just last in the bb, or it can be a debug
stmt.  E.g. for -fcompare-debug sanity, inserting before the last stmt
in the block is wrong, because without -g it could be some random stmt and
with -g it can be a debug stmt, so the resulting stmts will then differ
between -g and -g0.  Or the last stmt could actually be computing something
you use in the expressions?
I think generally you want gsi_insert_seq_on_edge, just be prepared that it
doesn't always work - one can't insert on EH or ABNORMAL edges.
For EH/ABNORMAL edges not really sure what can be done, punt, force just
__bos behavior for it, or perhaps insert before the last stmt in that case,
but beware that it would need to be SSA_NAME_OCCURS_IN_ABNORMAL_PHI
SSA_NAME which I think needs to have underlying SSA_NAME_VAR and needs to
follow rules such that out of SSA can handle it.

	Jakub