Re: [PATCH v5 1/4] tree-object-size: Support dynamic sizes in conditions

[Siddhesh Poyarekar]

On 10/01/2022 16:07, Jakub Jelinek wrote:

You test the above with both possibilities.


+  if (test_builtin_calloc_condphi (128, 1, 0) == 128)
+FAIL ();


But not this one, why?  Also, it would be better to have
a != ... test rather than ==, if it is the VLA, then 128 * sizeof (struct { int 
a; char b; })
?


I think I'll move the test_builtin_calloc_condphi test into the 
GIMPLE_CALL patch since that's where it becomes fully functional.


Thanks,
Siddhesh

Re: [PATCH v5 1/4] tree-object-size: Support dynamic sizes in conditions

[Jakub Jelinek via Gcc-patches]

On Sat, Dec 18, 2021 at 06:05:08PM +0530, Siddhesh Poyarekar wrote:
Sorry for the delay.

> +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);
> +  size_t ret = __builtin_dynamic_object_size (cond ? ch : (void *) , 0);
> +
> +  __builtin_free (ch);
> +  return ret;
> +}

> +int
> +main (int argc, char **argv)

You don't use argc nor argv, just leave those out IMO.

> +{
> +  if (test_builtin_malloc_condphi (1) != 32)
> +FAIL ();
> +  if (test_builtin_malloc_condphi (0) != 64)
> +FAIL ();

You test the above with both possibilities.

> +  if (test_builtin_calloc_condphi (128, 1, 0) == 128)
> +FAIL ();

But not this one, why?  Also, it would be better to have
a != ... test rather than ==, if it is the VLA, then 128 * sizeof (struct { int 
a; char b; })
?

> +/* Return true if VAL is represents an initial size for OBJECT_SIZE_TYPE.  */

s/is //

> +
> +static inline bool
> +size_initval_p (tree val, int object_size_type)

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

Formatting, just one space before as_a.

> +  /* Expand all size expressions to put their definitions close to the 
> objects
> + for whom size is being computed.  */

English is not my primary language, but shouldn't whom be used just
when talking about persons?  So for which size instead?

>  
> +static void
> +dynamic_object_size (struct object_size_info *osi, tree var,
> +  tree *size, tree *wholesize)

Missing function comment.

> +  if (i < num_args)
> +sizes = wholesizes = size_unknown (object_size_type);

Perhaps
  ggc_free (sizes);
  gcc_free (wholesizes);
here before the assignment?

> +
> +  /* Point to the same TREE_VEC so that we can avoid emitting two PHI
> + nodes.  */
> +  if (!wholesize_needed)

and make this else if

> +wholesizes = sizes;

and ggc_free (wholesizes); before the assignment?
When it is very easy and provably correct that it will just be memory
to be GCed later...

Otherwise LGTM.

Jakub

[PATCH v5 1/4] tree-object-size: Support dynamic sizes in conditions

[Siddhesh Poyarekar]

Handle GIMPLE_PHI and conditionals specially for dynamic objects,
returning PHI/conditional expressions instead of just a MIN/MAX
estimate.

This makes the returned object size variable for loops and conditionals,
so tests need to be adjusted to look for precise size in some cases.
builtin-dynamic-object-size-5.c had to be modified to only look for
success in maximum object size case and skip over the minimum object
size tests because the result is no longer a compile time constant.

I also added some simple tests to exercise conditionals with dynamic
object sizes.

gcc/ChangeLog:

* builtins.c (fold_builtin_object_size): Adjust for dynamic size
expressions.
* tree-object-size.c: Include gimplify-me.h.
(struct object_size_info): New member UNKNOWNS.
(size_initval_p, size_usable_p, object_sizes_get_raw): New
functions.
(object_sizes_get): Return suitable gimple variable for
object size.
(bundle_sizes): New function.
(object_sizes_set): Use it and handle dynamic object size
expressions.
(object_sizes_set_temp): New function.
(size_for_offset): Adjust for dynamic size expressions.
(emit_phi_nodes, propagate_unknowns, gimplify_size_expressions):
New functions.
(compute_builtin_object_size): Call gimplify_size_expressions
for OST_DYNAMIC.
(dynamic_object_size): New function.
(cond_expr_object_size): Use it.
(phi_dynamic_object_size): New function.
(collect_object_sizes_for): Call it for OST_DYNAMIC.  Adjust to
accommodate dynamic object sizes.

gcc/testsuite/ChangeLog:

* gcc.dg/builtin-dynamic-object-size-0.c: New tests.
* gcc.dg/builtin-dynamic-object-size-10.c: Add comment.
* gcc.dg/builtin-dynamic-object-size-5-main.c: New file.
* gcc.dg/builtin-dynamic-object-size-5.c: Use it and change test
to dg-do run.
* gcc.dg/builtin-object-size-5.c [!N]: Define N.
(test1, test2, test3, test4) [__builtin_object_size]: Expect
exact result for __builtin_dynamic_object_size.
* gcc.dg/builtin-object-size-1.c [__builtin_object_size]: Expect
exact size expressions for __builtin_dynamic_object_size.
* gcc.dg/builtin-object-size-2.c [__builtin_object_size]:
Likewise.
* gcc.dg/builtin-object-size-3.c [__builtin_object_size]:
Likewise.
* gcc.dg/builtin-object-size-4.c [__builtin_object_size]:
Likewise.
* gcc.dg/builtin-object-size-5.c [__builtin_object_size]:
Likewise.

Signed-off-by: Siddhesh Poyarekar 
---
Changes since v4:

- Propagate sameness of size and wholesize through PHI nodes whenever
  possible.  Check and avoid emitting wholesize if it is the same as
  size.
- Bail out and punt to __builtin_object_size if PHI node has any complex
  edges.  Use insert_seq_on_edge to emit size PHI node edge values.
- Free allocations in tests.

 gcc/builtins.c|   6 +-
 .../gcc.dg/builtin-dynamic-object-size-0.c|  77 +++
 .../gcc.dg/builtin-dynamic-object-size-10.c   |   2 +
 .../builtin-dynamic-object-size-5-main.c  |  32 ++
 .../gcc.dg/builtin-dynamic-object-size-5.c|   7 +-
 gcc/testsuite/gcc.dg/builtin-object-size-1.c  | 119 +++-
 gcc/testsuite/gcc.dg/builtin-object-size-2.c  |  92 
 gcc/testsuite/gcc.dg/builtin-object-size-3.c  | 121 +
 gcc/testsuite/gcc.dg/builtin-object-size-4.c  |  78 +++
 gcc/testsuite/gcc.dg/builtin-object-size-5.c  |  22 +-
 gcc/tree-object-size.c| 509 +-
 11 files changed, 1030 insertions(+), 35 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c
 create mode 100644 gcc/testsuite/gcc.dg/builtin-dynamic-object-size-5-main.c

diff --git a/gcc/builtins.c b/gcc/builtins.c
index 00f6c5552bf..01c24f42540 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -10285,7 +10285,8 @@ fold_builtin_object_size (tree ptr, tree ost, enum 
built_in_function fcode)
   if (TREE_CODE (ptr) == ADDR_EXPR)
 {
   compute_builtin_object_size (ptr, object_size_type, );
-  if (int_fits_type_p (bytes, size_type_node))
+  if ((object_size_type & OST_DYNAMIC)
+ || int_fits_type_p (bytes, size_type_node))
return fold_convert (size_type_node, bytes);
 }
   else if (TREE_CODE (ptr) == SSA_NAME)
@@ -10294,7 +10295,8 @@ fold_builtin_object_size (tree ptr, tree ost, enum 
built_in_function fcode)
later.  Maybe subsequent passes will help determining
it.  */
   if (compute_builtin_object_size (ptr, object_size_type, )
- && int_fits_type_p (bytes, size_type_node))
+ && ((object_size_type & OST_DYNAMIC)
+ || int_fits_type_p (bytes, size_type_node)))
return fold_convert (size_type_node, bytes);
 }
 
diff --git a/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c