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

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

On 12/15/21 21:54, Jakub Jelinek wrote:
> 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.
> 

OK, I'll fix up all patches for this.

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

Got it, I'll do that.

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

OK.

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

OK.

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

Got it, I'll fix that.

> Also, calling a parameter or var ssa is quite unusual, normally
> one calls a SSA_NAME either name, or ssa_name etc.

OK.

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

OK.

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

I'm emitting both size and wholesize all the time; wholesize only really 
gets used in size_for_offset and otherwise should get DCE'd.  Right now 
for __bos (and constant sizes) wholesize is unused if it is the same as 
size.

FOR GIMPLE_CALL, GIMPLE_NOP, etc. I return the same tree for size and 
wholesize; maybe a trivial pointer comparison (sz != wholesize) ought to 
get rid of most of the uses in size_for_offset.

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

Hmm, so I only really need wholesize in ADDR_EXPR and POINTER_PLUS 
expressions.  I suppose I could improve this bit too and reduce 
wholesize computations.

>> +      /* 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,

Wouldn't the bb minimally have to contain the definition of the object 
whose size we computed?  e.g. for PHI [a(2), b(3)], wouldn't bb 2 at 
least have a statement with the definition of a?

Or wait, there could be situations where the definition is in a 
different block, e.g. bb 1, which has a single edge going on to bb 2?

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

I suppose __bos-like behaviour could be a good compromise, i.e. insert a 
MAX_EXPR (or MIN_EXPR) if we can't find a suitable location to insert on 
edge.

Siddhesh