On Fri, Sep 20, 2024 at 12:40:26PM -0400, Siddhesh Poyarekar wrote:
> When wholesize != size, there is a reasonable opportunity for static
> object sizes also to be computed using size_for_offset, so use that.
>
> gcc/ChangeLog:
>
> * tree-object-size.cc (plus_stmt_object_size): Call
> SIZE_FOR_OFFSET for some negative offset cases.
> * testsuite/gcc.dg/builtin-object-size-3.c (test9): Adjust test.
> * testsuite/gcc.dg/builtin-object-size-4.c (test8): Likewise.
> ---
> gcc/testsuite/gcc.dg/builtin-object-size-3.c | 6 +++---
> gcc/testsuite/gcc.dg/builtin-object-size-4.c | 6 +++---
> gcc/tree-object-size.cc | 2 +-
> 3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/gcc/testsuite/gcc.dg/builtin-object-size-3.c
> b/gcc/testsuite/gcc.dg/builtin-object-size-3.c
> index 3f58da3d500..ec2c62c9640 100644
> --- a/gcc/testsuite/gcc.dg/builtin-object-size-3.c
> +++ b/gcc/testsuite/gcc.dg/builtin-object-size-3.c
> @@ -574,7 +574,7 @@ test9 (unsigned cond)
> if (__builtin_object_size (&p[-4], 2) != (cond ? 6 : 10))
> FAIL ();
> #else
> - if (__builtin_object_size (&p[-4], 2) != 0)
> + if (__builtin_object_size (&p[-4], 2) != 6)
> FAIL ();
> #endif
>
> @@ -585,7 +585,7 @@ test9 (unsigned cond)
> if (__builtin_object_size (p, 2) != ((cond ? 2 : 6) + cond))
> FAIL ();
> #else
> - if (__builtin_object_size (p, 2) != 0)
> + if (__builtin_object_size (p, 2) != 2)
> FAIL ();
> #endif
>
> @@ -598,7 +598,7 @@ test9 (unsigned cond)
> != sizeof (y) - __builtin_offsetof (struct A, c) - 8 + cond)
> FAIL ();
> #else
> - if (__builtin_object_size (p, 2) != 0)
> + if (__builtin_object_size (p, 2) != sizeof (y) - __builtin_offsetof
> (struct A, c) - 8)
> FAIL ();
> #endif
> }
> diff --git a/gcc/testsuite/gcc.dg/builtin-object-size-4.c
> b/gcc/testsuite/gcc.dg/builtin-object-size-4.c
> index b3eb36efb74..7bcd24c4150 100644
> --- a/gcc/testsuite/gcc.dg/builtin-object-size-4.c
> +++ b/gcc/testsuite/gcc.dg/builtin-object-size-4.c
> @@ -482,7 +482,7 @@ test8 (unsigned cond)
> if (__builtin_object_size (&p[-4], 3) != (cond ? 6 : 10))
> FAIL ();
> #else
> - if (__builtin_object_size (&p[-4], 3) != 0)
> + if (__builtin_object_size (&p[-4], 3) != 6)
> FAIL ();
> #endif
>
> @@ -493,7 +493,7 @@ test8 (unsigned cond)
> if (__builtin_object_size (p, 3) != ((cond ? 2 : 6) + cond))
> FAIL ();
> #else
> - if (__builtin_object_size (p, 3) != 0)
> + if (__builtin_object_size (p, 3) != 2)
> FAIL ();
> #endif
>
> @@ -505,7 +505,7 @@ test8 (unsigned cond)
> if (__builtin_object_size (p, 3) != sizeof (y.c) - 8 + cond)
> FAIL ();
> #else
> - if (__builtin_object_size (p, 3) != 0)
> + if (__builtin_object_size (p, 3) != sizeof (y.c) - 8)
> FAIL ();
> #endif
> }
The testcase changes look reasonable to me.
> diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
> index 6544730e153..f8fae0cbc82 100644
> --- a/gcc/tree-object-size.cc
> +++ b/gcc/tree-object-size.cc
> @@ -1527,7 +1527,7 @@ plus_stmt_object_size (struct object_size_info *osi,
> tree var, gimple *stmt)
> if (size_unknown_p (bytes, 0))
> ;
> else if ((object_size_type & OST_DYNAMIC)
> - || compare_tree_int (op1, offset_limit) <= 0)
> + || bytes != wholesize || compare_tree_int (op1, offset_limit) <=
> 0)
> bytes = size_for_offset (bytes, op1, wholesize);
> /* In the static case, with a negative offset, the best estimate for
> minimum size is size_unknown but for maximum size, the wholesize is a
The coding conventions say that in cases like this where the whole condition
doesn't fit on a single line, each ||/&& operand should be on a separate
line.
So, the patch should be adding || bytes != wholesize on a separate line.
That said, there is a pre-existing problem, the tree direct comparisons
(bytes != wholesize here, && wholesize != sz in size_for_offset (note,
again, it should be on a separate line), maybe others).
We do INTEGER_CST caching, either using small array for small values or
hash table for larger ones, so INTEGER_CSTs with the same value of the
same type should be pointer equal unless they are TREE_OVERFLOW or similar,
but for anything else, unless you guarantee that in the "same" case
you assign the same tree to size/wholesize rather than say
perform size_binop twice, I'd expect instead comparisons with
operand_equal_p or something similar.
Though, because this patch is solely for the __builtin_object_size case
and the sizes in that case should be solely INTEGER_CSTs, I guess this patch
is ok with the formatting nit fix (and ideally the one in size_for_offset
too).
Jakub
