On Sat, 7 Dec 2024, Siddhesh Poyarekar wrote:
> On 2024-12-05 05:37, Richard Biener wrote:
> > VN again is the culprit for exploiting address equivalences before
> > __builtin_object_size got the chance to do its job. This time
> > it isn't about union members but adjacent structure fields where
> > an address to one after the last element of an array field can
> > spill over to the next field.
> >
> > The following protects all out-of-bound accesses on the upper bound
> > side (singling out TYPE_MAX_VALUE + 1 is more expensive). It
> > ignores other out-of-bound addresses that would invoke UB.
> >
> > Zero-sized arrays are a bit awkward because the C++ represents them
> > with a -1U upper bound.
> >
> > There's a similar issue for zero-sized components whose address can
> > be the same as the adjacent field in C.
> >
> > Bootstrap and regtest running on x86_64-unknown-linux-gnu.
> >
> > Does this look OK?
>
> I can't approve of course, but the fix looks correct to me module a comment
> typo nit below.
Sure - fixed the comments. I'm still waiting on Jakubs OK though.
Thanks,
Richard.
> Thanks,
> Sid
>
> > We possbly want to have a zero_size_domain_p () function, or clean up
> > the zero-size mess - I refrained from dealing with this in this patch.
> >
> > Richard.
> >
> > PR tree-optimization/117912
> > * tree-ssa-sccvn.cc (copy_reference_ops_from_ref): For addresses
> > of zero-sized components do not set ->off if the object size pass
> > didn't run.
> > For OOB ARRAY_REF accesses in address expressions avoid setting
> > ->off if the object size pass didn't run.
> > (valueize_refs_1): Likewise.
> >
> > * c-c++-common/torture/pr117912-1.c: New testcase.
> > * c-c++-common/torture/pr117912-2.c: Likewise.
> > * gcc.dg/torture/pr117912-3.c: Likewise.
> > ---
> > .../c-c++-common/torture/pr117912-1.c | 28 +++++++++++
> > .../c-c++-common/torture/pr117912-2.c | 28 +++++++++++
> > gcc/testsuite/gcc.dg/torture/pr117912-3.c | 18 +++++++
> > gcc/tree-ssa-sccvn.cc | 47 +++++++++++++++++--
> > 4 files changed, 118 insertions(+), 3 deletions(-)
> > create mode 100644 gcc/testsuite/c-c++-common/torture/pr117912-1.c
> > create mode 100644 gcc/testsuite/c-c++-common/torture/pr117912-2.c
> > create mode 100644 gcc/testsuite/gcc.dg/torture/pr117912-3.c
> >
> > diff --git a/gcc/testsuite/c-c++-common/torture/pr117912-1.c
> > b/gcc/testsuite/c-c++-common/torture/pr117912-1.c
> > new file mode 100644
> > index 00000000000..2750585c7f7
> > --- /dev/null
> > +++ b/gcc/testsuite/c-c++-common/torture/pr117912-1.c
> > @@ -0,0 +1,28 @@
> > +/* { dg-do run } */
> > +
> > +struct S { int a; int b[24]; int c[24]; int d; };
> > +volatile int *p;
> > +
> > +void __attribute__((noipa))
> > +bar (int *q)
> > +{
> > + p = q;
> > +}
> > +
> > +__SIZE_TYPE__ __attribute__((noipa))
> > +foo (struct S *p)
> > +{
> > + bar (&p->b[24]);
> > + bar (&p->c[0]);
> > + return __builtin_object_size (&p->c[0], 1);
> > +}
> > +
> > +int
> > +main()
> > +{
> > + struct S s;
> > + __SIZE_TYPE__ x = foo (&s);
> > + if (x < sizeof (int) * 24)
> > + __builtin_abort ();
> > + return 0;
> > +}
> > diff --git a/gcc/testsuite/c-c++-common/torture/pr117912-2.c
> > b/gcc/testsuite/c-c++-common/torture/pr117912-2.c
> > new file mode 100644
> > index 00000000000..a3a62157563
> > --- /dev/null
> > +++ b/gcc/testsuite/c-c++-common/torture/pr117912-2.c
> > @@ -0,0 +1,28 @@
> > +/* { dg-do run } */
> > +
> > +struct S { int a; int b[0]; int c[24]; int d; };
> > +volatile int *p;
> > +
> > +void __attribute__((noipa))
> > +bar (int *q)
> > +{
> > + p = q;
> > +}
> > +
> > +__SIZE_TYPE__ __attribute__((noipa))
> > +foo (struct S *p)
> > +{
> > + bar (&p->b[0]);
> > + bar (&p->c[0]);
> > + return __builtin_object_size (&p->c[0], 1);
> > +}
> > +
> > +int
> > +main()
> > +{
> > + struct S s;
> > + __SIZE_TYPE__ x = foo (&s);
> > + if (x < sizeof (int) * 24)
> > + __builtin_abort ();
> > + return 0;
> > +}
> > diff --git a/gcc/testsuite/gcc.dg/torture/pr117912-3.c
> > b/gcc/testsuite/gcc.dg/torture/pr117912-3.c
> > new file mode 100644
> > index 00000000000..b4a036439c3
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/torture/pr117912-3.c
> > @@ -0,0 +1,18 @@
> > +/* { dg-do link } */
> > +/* { dg-skip-if "" { *-*-* } { "-O0" } { "" } } */
> > +
> > +long s;
> > +struct A * __attribute__((noipa)) foo (void) {}
> > +struct B {};
> > +struct B __attribute__((noipa)) bar (struct B *) {}
> > +void baz (void) __attribute__((__error__ ("baz")));
> > +struct A { int a; struct B b; char c[]; };
> > +int
> > +main ()
> > +{
> > + struct A *p = foo ();
> > + bar (&p->b);
> > + unsigned long q = __builtin_object_size (p->c, 1);
> > + if (q < s)
> > + baz ();
> > +}
> > diff --git a/gcc/tree-ssa-sccvn.cc b/gcc/tree-ssa-sccvn.cc
> > index 8d74731a891..9b9fbddb1ab 100644
> > --- a/gcc/tree-ssa-sccvn.cc
> > +++ b/gcc/tree-ssa-sccvn.cc
> > @@ -987,9 +987,12 @@ copy_reference_ops_from_ref (tree ref,
> > vec<vn_reference_op_s> *result)
> > + (wi::to_offset (bit_offset) >> LOG2_BITS_PER_UNIT));
> > /* Probibit value-numbering zero offset components
>
> Maybe also fix this typo while we're here?
>
> > of addresses the same before the pass folding
> > - __builtin_object_size had a chance to run. */
> > + __builtin_object_size had a chance to run. Likewise
> > + for components of zero size at arbitrary offset. */
> > if (TREE_CODE (orig) != ADDR_EXPR
> > - || maybe_ne (off, 0)
> > + || (TYPE_SIZE (temp.type)
> > + && !integer_zerop (TYPE_SIZE (temp.type))
> > + && maybe_ne (off, 0))
> > || (cfun->curr_properties & PROP_objsz))
> > off.to_shwi (&temp.off);
> > }
> > @@ -1010,9 +1013,30 @@ copy_reference_ops_from_ref (tree ref,
> > vec<vn_reference_op_s> *result)
> > if (! temp.op2)
> > temp.op2 = size_binop (EXACT_DIV_EXPR, TYPE_SIZE_UNIT (eltype),
> > size_int (TYPE_ALIGN_UNIT (eltype)));
> > + /* Probibit value-numbering addresses of out-of-bound ARRAY_REFs
>
> ... and then avoid propagating that typo here :)
>
> > + the same as addresses of other components before the pass
> > + folding __builtin_object_size had a chance to run. */
> > + bool avoid_oob = true;
> > + if (TREE_CODE (orig) != ADDR_EXPR
> > + || cfun->curr_properties & PROP_objsz)
> > + avoid_oob = false;
> > + else if (poly_int_tree_p (temp.op0))
> > + {
> > + tree ub = array_ref_up_bound (ref);
> > + if (ub
> > + && poly_int_tree_p (ub)
> > + /* ??? The C frontend for T[0] uses [0:] and the
> > + C++ frontend [0:-1U]. See layout_type for how
> > + awkward this is. */
> > + && !integer_minus_onep (ub)
> > + && known_le (wi::to_poly_offset (temp.op0),
> > + wi::to_poly_offset (ub)))
> > + avoid_oob = false;
> > + }
> > if (poly_int_tree_p (temp.op0)
> > && poly_int_tree_p (temp.op1)
> > - && TREE_CODE (temp.op2) == INTEGER_CST)
> > + && TREE_CODE (temp.op2) == INTEGER_CST
> > + && !avoid_oob)
> > {
> > poly_offset_int off = ((wi::to_poly_offset (temp.op0)
> > - wi::to_poly_offset (temp.op1))
> > @@ -1754,6 +1778,23 @@ re_valueize:
> > && poly_int_tree_p (vro->op1)
> > && TREE_CODE (vro->op2) == INTEGER_CST)
> > {
> > + /* Probibit value-numbering addresses of out-of-bound ARRAY_REFs
>
> Likewise.
>
> > + the same as addresses of other components before the pass
> > + folding __builtin_object_size had a chance to run. */
> > + if (!(cfun->curr_properties & PROP_objsz)
> > + && (*orig)[0].opcode == ADDR_EXPR)
> > + {
> > + tree dom = TYPE_DOMAIN ((*orig)[i + 1].type);
> > + if (!dom
> > + || !TYPE_MAX_VALUE (dom)
> > + || !poly_int_tree_p (TYPE_MAX_VALUE (dom))
> > + || integer_minus_onep (TYPE_MAX_VALUE (dom)))
> > + continue;
> > + if (!known_le (wi::to_poly_offset (vro->op0),
> > + wi::to_poly_offset (TYPE_MAX_VALUE (dom))))
> > + continue;
> > + }
> > +
> > poly_offset_int off = ((wi::to_poly_offset (vro->op0)
> > - wi::to_poly_offset (vro->op1))
> > * wi::to_offset (vro->op2)
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
