diff mbox series

tree-optimization/108522 Use COMPONENT_REF offset when available

Message ID 20230124215400.1345220-1-siddhesh@gotplt.org
State Accepted
Headers
Series tree-optimization/108522 Use COMPONENT_REF offset when available |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Siddhesh Poyarekar Jan. 24, 2023, 9:54 p.m. UTC 
  Use the offset in TREE_OPERAND(component_ref, 2) when available instead
of DECL_FIELD_OFFSET when trying to compute offset for a COMPONENT_REF.

OK for gcc 13 and gcc 12?

Co-authored-by: Jakub Jelinek <jakub@redhat.com>

gcc/ChangeLog:

	PR tree-optimization/108522
	* tree-object-size.cc (compute_object_offset): Use
	TREE_OPERAND(ref, 2) for COMPONENT_REF when available.

gcc/testsuite/ChangeLog:

	PR tree-optimization/108522
	* builtin-dynamic-object-size-0.c (test_dynarray_struct_member):
	new test.
	(main): Call it.

Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org>
---
Testing:

- Bootstrapped on x86_64, I'm checking to confirm if a couple of
  seemingly unrelated failures are in fact unrelated.
- ubsan config bootstrap and i686 tests in progress

 .../gcc.dg/builtin-dynamic-object-size-0.c       | 16 ++++++++++++++++
 gcc/tree-object-size.cc                          |  4 +++-
 2 files changed, 19 insertions(+), 1 deletion(-)

Comments

Jakub Jelinek Jan. 24, 2023, 10:09 p.m. UTC | #1 
On Tue, Jan 24, 2023 at 04:54:00PM -0500, Siddhesh Poyarekar wrote:
> Use the offset in TREE_OPERAND(component_ref, 2) when available instead
> of DECL_FIELD_OFFSET when trying to compute offset for a COMPONENT_REF.
> 
> OK for gcc 13 and gcc 12?

Ok for trunk, I'd wait a week or two with the backport.

Thanks.

> Co-authored-by: Jakub Jelinek <jakub@redhat.com>
> 
> gcc/ChangeLog:
> 
> 	PR tree-optimization/108522
> 	* tree-object-size.cc (compute_object_offset): Use
> 	TREE_OPERAND(ref, 2) for COMPONENT_REF when available.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR tree-optimization/108522
> 	* builtin-dynamic-object-size-0.c (test_dynarray_struct_member):
> 	new test.
> 	(main): Call it.
> 
> Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org>

	Jakub
Richard Biener Jan. 25, 2023, 7:44 a.m. UTC | #2 
On Tue, Jan 24, 2023 at 10:54 PM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
>
> Use the offset in TREE_OPERAND(component_ref, 2) when available instead
> of DECL_FIELD_OFFSET when trying to compute offset for a COMPONENT_REF.
>
> OK for gcc 13 and gcc 12?
>
> Co-authored-by: Jakub Jelinek <jakub@redhat.com>
>
> gcc/ChangeLog:
>
>         PR tree-optimization/108522
>         * tree-object-size.cc (compute_object_offset): Use
>         TREE_OPERAND(ref, 2) for COMPONENT_REF when available.
>
> gcc/testsuite/ChangeLog:
>
>         PR tree-optimization/108522
>         * builtin-dynamic-object-size-0.c (test_dynarray_struct_member):
>         new test.
>         (main): Call it.
>
> Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org>
> ---
> Testing:
>
> - Bootstrapped on x86_64, I'm checking to confirm if a couple of
>   seemingly unrelated failures are in fact unrelated.
> - ubsan config bootstrap and i686 tests in progress
>
>  .../gcc.dg/builtin-dynamic-object-size-0.c       | 16 ++++++++++++++++
>  gcc/tree-object-size.cc                          |  4 +++-
>  2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c b/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c
> index f9047a037d9..569c0a87722 100644
> --- a/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c
> +++ b/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c
> @@ -314,6 +314,20 @@ test_dynarray_struct_subobj2 (size_t sz, size_t off, size_t *objsz)
>    return __builtin_dynamic_object_size (&bin.c[off], 1);
>  }
>
> +/* See pr #108522.  */
> +size_t
> +__attribute__ ((noinline))
> +test_dynarray_struct_member (size_t sz)
> +{
> +  struct
> +    {
> +      char a[sz];
> +      char b;
> +    } s;
> +
> +  return __builtin_dynamic_object_size (&s.b, 0);
> +}
> +
>  size_t
>  __attribute__ ((noinline))
>  test_substring (size_t sz, size_t off)
> @@ -619,6 +633,8 @@ main (int argc, char **argv)
>    if (test_dynarray_struct_subobj2 (42, 4, &objsz)
>      != objsz - 4 - sizeof (long) - sizeof (int))
>      FAIL ();
> +  if (test_dynarray_struct_member (42) != sizeof (char))
> +    FAIL ();
>    if (test_substring_ptrplus (128, 4) != (128 - 4) * sizeof (int))
>      FAIL ();
>    if (test_substring_ptrplus (128, 142) != 0)
> diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
> index 356591c22cc..de93ffad9c9 100644
> --- a/gcc/tree-object-size.cc
> +++ b/gcc/tree-object-size.cc
> @@ -412,7 +412,9 @@ compute_object_offset (const_tree expr, const_tree var)
>         return base;
>
>        t = TREE_OPERAND (expr, 1);
> -      off = size_binop (PLUS_EXPR, DECL_FIELD_OFFSET (t),
> +      off = size_binop (PLUS_EXPR,
> +                       (TREE_OPERAND (expr, 2) ? TREE_OPERAND (expr, 2)
> +                        : DECL_FIELD_OFFSET (t)),

That isn't correct - operand 2 is the field offset in units of
DECL_OFFSET_ALIGN (t) / BITS_PER_UNIT.
See component_ref_filed_offset (), maybe you should be using that
function instead?

>                         size_int (tree_to_uhwi (DECL_FIELD_BIT_OFFSET (t))
>                                   / BITS_PER_UNIT));
>        break;
> --
> 2.38.1
>
Jakub Jelinek Jan. 25, 2023, 8:24 a.m. UTC | #3 
On Wed, Jan 25, 2023 at 08:44:31AM +0100, Richard Biener wrote:
> > --- a/gcc/tree-object-size.cc
> > +++ b/gcc/tree-object-size.cc
> > @@ -412,7 +412,9 @@ compute_object_offset (const_tree expr, const_tree var)
> >         return base;
> >
> >        t = TREE_OPERAND (expr, 1);
> > -      off = size_binop (PLUS_EXPR, DECL_FIELD_OFFSET (t),
> > +      off = size_binop (PLUS_EXPR,
> > +                       (TREE_OPERAND (expr, 2) ? TREE_OPERAND (expr, 2)
> > +                        : DECL_FIELD_OFFSET (t)),
> 
> That isn't correct - operand 2 is the field offset in units of
> DECL_OFFSET_ALIGN (t) / BITS_PER_UNIT.
> See component_ref_filed_offset (), maybe you should be using that
> function instead?

Oops.  s/filed/field/ I was looking for such a function, but didn't find it
last night :(.

	Jakub
Siddhesh Poyarekar Jan. 25, 2023, 11:22 a.m. UTC | #4 
On 2023-01-25 02:44, Richard Biener wrote:
>>         t = TREE_OPERAND (expr, 1);
>> -      off = size_binop (PLUS_EXPR, DECL_FIELD_OFFSET (t),
>> +      off = size_binop (PLUS_EXPR,
>> +                       (TREE_OPERAND (expr, 2) ? TREE_OPERAND (expr, 2)
>> +                        : DECL_FIELD_OFFSET (t)),
> 
> That isn't correct - operand 2 is the field offset in units of
> DECL_OFFSET_ALIGN (t) / BITS_PER_UNIT.
> See component_ref_filed_offset (), maybe you should be using that
> function instead?

Ahh, and it passed my testing only because I was testing a char. 
Thanks, I'll test and send an update with additional tests.

Thanks,
Sid
Jakub Jelinek Jan. 25, 2023, 11:27 a.m. UTC | #5 
On Wed, Jan 25, 2023 at 06:22:56AM -0500, Siddhesh Poyarekar wrote:
> On 2023-01-25 02:44, Richard Biener wrote:
> > >         t = TREE_OPERAND (expr, 1);
> > > -      off = size_binop (PLUS_EXPR, DECL_FIELD_OFFSET (t),
> > > +      off = size_binop (PLUS_EXPR,
> > > +                       (TREE_OPERAND (expr, 2) ? TREE_OPERAND (expr, 2)
> > > +                        : DECL_FIELD_OFFSET (t)),
> > 
> > That isn't correct - operand 2 is the field offset in units of
> > DECL_OFFSET_ALIGN (t) / BITS_PER_UNIT.
> > See component_ref_filed_offset (), maybe you should be using that
> > function instead?
> 
> Ahh, and it passed my testing only because I was testing a char. Thanks,
> I'll test and send an update with additional tests.

I think you want something like:
  struct S {
    char a[n];
    unsigned long long b;
    int d;
    char e[2 * n];
  } s;
and test say bdos of &s.d, 0 and &s.e[n - 2], 0

	Jakub
Jakub Jelinek Jan. 25, 2023, 11:33 a.m. UTC | #6 
On Wed, Jan 25, 2023 at 12:27:13PM +0100, Jakub Jelinek via Gcc-patches wrote:
> On Wed, Jan 25, 2023 at 06:22:56AM -0500, Siddhesh Poyarekar wrote:
> > On 2023-01-25 02:44, Richard Biener wrote:
> > > >         t = TREE_OPERAND (expr, 1);
> > > > -      off = size_binop (PLUS_EXPR, DECL_FIELD_OFFSET (t),
> > > > +      off = size_binop (PLUS_EXPR,
> > > > +                       (TREE_OPERAND (expr, 2) ? TREE_OPERAND (expr, 2)
> > > > +                        : DECL_FIELD_OFFSET (t)),
> > > 
> > > That isn't correct - operand 2 is the field offset in units of
> > > DECL_OFFSET_ALIGN (t) / BITS_PER_UNIT.
> > > See component_ref_filed_offset (), maybe you should be using that
> > > function instead?
> > 
> > Ahh, and it passed my testing only because I was testing a char. Thanks,
> > I'll test and send an update with additional tests.
> 
> I think you want something like:
>   struct S {
>     char a[n];
>     unsigned long long b;
>     int d;
>     char e[2 * n];
>   } s;
> and test say bdos of &s.d, 0 and &s.e[n - 2], 0

And in the caller compared that to offsetof/sizeof based expressions
for a similar structure which just uses constants instead of the n
in there.

	Jakub
diff mbox series

Patch

diff --git a/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c b/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c
index f9047a037d9..569c0a87722 100644
--- a/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c
+++ b/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c
@@ -314,6 +314,20 @@  test_dynarray_struct_subobj2 (size_t sz, size_t off, size_t *objsz)
   return __builtin_dynamic_object_size (&bin.c[off], 1);
 }
 
+/* See pr #108522.  */
+size_t
+__attribute__ ((noinline))
+test_dynarray_struct_member (size_t sz)
+{
+  struct
+    {
+      char a[sz];
+      char b;
+    } s;
+
+  return __builtin_dynamic_object_size (&s.b, 0);
+}
+
 size_t
 __attribute__ ((noinline))
 test_substring (size_t sz, size_t off)
@@ -619,6 +633,8 @@  main (int argc, char **argv)
   if (test_dynarray_struct_subobj2 (42, 4, &objsz)
     != objsz - 4 - sizeof (long) - sizeof (int))
     FAIL ();
+  if (test_dynarray_struct_member (42) != sizeof (char))
+    FAIL ();
   if (test_substring_ptrplus (128, 4) != (128 - 4) * sizeof (int))
     FAIL ();
   if (test_substring_ptrplus (128, 142) != 0)
diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
index 356591c22cc..de93ffad9c9 100644
--- a/gcc/tree-object-size.cc
+++ b/gcc/tree-object-size.cc
@@ -412,7 +412,9 @@  compute_object_offset (const_tree expr, const_tree var)
 	return base;
 
       t = TREE_OPERAND (expr, 1);
-      off = size_binop (PLUS_EXPR, DECL_FIELD_OFFSET (t),
+      off = size_binop (PLUS_EXPR,
+			(TREE_OPERAND (expr, 2) ? TREE_OPERAND (expr, 2)
+			 : DECL_FIELD_OFFSET (t)),
 			size_int (tree_to_uhwi (DECL_FIELD_BIT_OFFSET (t))
 				  / BITS_PER_UNIT));
       break;