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
