On Mon, 9 Dec 2024, Jakub Jelinek wrote:
> Hi!
>
> On Thu, Dec 05, 2024 at 11:37:45AM +0100, 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.
>
> So, as I wrote earlier, I'd suggest dropping gcc.dg/torture/pr117912-3.c
> from the patch and replacing it with the attached one (now tweaked
> to use [[no_unique_address]] and be C++ compatible).
>
> Generally, I'm really surprised all these routines only look at one
> reference, don't compare two references (one earlier seen, another one
> compared against that), which is where something like tree-object-size.cc
> (addr_object_size) like checking could be done, but guess that just
> shows how little I know about SCCVN.
>
> The 3 hunks seem correct in what they attempt to do, zero sized components
> are indeed a problem because they result in the same offset for perhaps
> __bos POV incompatible access. And similarly the out of bounds (invalid)
> plus the boundary case (valid to have address taken, invalid to dereference
> it). And to my surprise (haven't studied how it actually works), if I
> modify the testcases to have
> bar (&p->b[24]);
> bar (&p->b[24]);
> bar (&p->b[24]);
> (in -1.c) or
> bar (&p->b);
> bar (&p->b);
> bar (&p->b);
> (in -3.c), then fre1 still optimizes all the &p->b[24] to just one SSA_NAME
> (or all the &p->b).
yes, the ->off member is to allow non-structurally same refs to
value-number the same. With ->off == -1 (unknown) structurally
same refs are still value-numbered the same.
> > --- 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
>
> s/Probibit/Prohibit/
Fixed everywhere.
> > 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 && integer_nonzerop (TYPE_SIZE (temp.type)) instead?
> I mean, if we have a VL structure
> struct T { int b[n]; };
> struct S { int a; struct T b; int c; };
> then b could have zero size (if n == 0) or non-zero one. Perhaps
> it is punted on elsewhere and __builtin_object_size (&s->b, 1) and
> __builtin_object_size (&s->c, 1) could be different.
Good point. Changed.
>
> > + && 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
>
> s/Probibit/Prohibit/ again
>
> Plus am not sure if out-of-bound is the right term to use, in the test
> b[24] is not out of bounds, it is one past the last element in the array
> in C/C++ terms, which is out of bounds only for dereferences.
> Perhaps calling the var oob is fine if the comment makes it clear that
> it is about real out of bounds accesses as well as taking address of the
> one past the last element in an array.
I've changed it to 'addresses of one-after-the-last element ARRAY_REFs'
> > + 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;
>
> I wonder if we shouldn't punt (aka have avoid_oob true) also on temp.op0
> smaller
> than array_ref_low_bound (ref).
Those would be UB, I've tried to keep the check conservative but fast.
> > + }
> > 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
>
> Again Prohibit
> Same concern about naming.
>
> > + 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;
>
> And again wonder about low bound.
>
> > + }
> > +
> > poly_offset_int off = ((wi::to_poly_offset (vro->op0)
> > - wi::to_poly_offset (vro->op1))
> > * wi::to_offset (vro->op2)
>
> Otherwise LGTM.
I'll retest, repost and push.
Thanks,
Richard.
> --- gcc/testsuite/c-c++-common/torture/pr117912-3.c.jj 2024-12-09
> 12:04:07.892204122 +0100
> +++ gcc/testsuite/c-c++-common/torture/pr117912-3.c 2024-12-09
> 12:10:25.309936271 +0100
> @@ -0,0 +1,61 @@
> +/* { dg-do run } */
> +/* { dg-additional-options "-std=gnu++20" { target c++ } } */
> +
> +struct B {};
> +struct A { int a;
> +#ifdef __cplusplus
> + [[no_unique_address]]
> +#endif
> + struct B b;
> + char c[]; };
> +volatile void *p;
> +
> +void __attribute__((noipa))
> +bar (void *q)
> +{
> + p = q;
> +}
> +
> +__SIZE_TYPE__ __attribute__((noipa))
> +foo (struct A *p)
> +{
> + bar (&p->b);
> + bar (&p->c);
> + return __builtin_object_size (&p->c, 1);
> +}
> +
> +__SIZE_TYPE__ __attribute__((noipa))
> +baz (void)
> +{
> + struct A *p = (struct A *) __builtin_malloc (__builtin_offsetof (struct A,
> c) + 64);
> + bar (&p->b);
> + bar (&p->c);
> + return __builtin_object_size (&p->c, 1);
> +}
> +
> +__SIZE_TYPE__ __attribute__((noipa))
> +qux (struct A *p)
> +{
> + bar (&p->b);
> + bar (&p->c);
> + return __builtin_object_size (&p->c, 3);
> +}
> +
> +__SIZE_TYPE__ __attribute__((noipa))
> +boo (void)
> +{
> + struct A *p = (struct A *) __builtin_malloc (__builtin_offsetof (struct A,
> c) + 64);
> + bar (&p->b);
> + bar (&p->c);
> + return __builtin_object_size (&p->c, 3);
> +}
> +
> +int
> +main ()
> +{
> + static struct A a = { .a = 1, .b = {}, .c = { 1, 2, 3, 4, 0 } };
> + if (foo (&a) < 5)
> + __builtin_abort ();
> + if (baz () < 64)
> + __builtin_abort ();
> +}
>
>
> Jakub
>
>
--
Richard Biener <[email protected]>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
