| Message ID | 20230124215400.1345220-1-siddhesh@gotplt.org |
|---|---|
| State | New |
| Headers | show |
| Series | tree-optimization/108522 Use COMPONENT_REF offset when available | expand |
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
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 >
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
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
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
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 --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;
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(-)