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
