diff mbox series

[v2,1/4] tree-object-size: use size_for_offset in more cases

Message ID 20240920164029.63843-2-siddhesh@gotplt.org
State New
Headers
Series tree-object-size: Improved offset handling |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Test passed

Commit Message

Siddhesh Poyarekar Sept. 20, 2024, 4:40 p.m. UTC 
  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(-)

Comments

Jakub Jelinek Sept. 27, 2024, 10:31 a.m. UTC | #1 
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
diff mbox series

Patch

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