On Thu, Mar 09, 2023 at 07:44:53PM -0500, Marek Polacek wrote:
> On Thu, Mar 09, 2023 at 09:44:49AM +0100, Jakub Jelinek wrote:
> > On Thu, Mar 09, 2023 at 08:12:47AM +0000, Richard Biener wrote:
> > > I think this is a reasonable way to address the regression, so OK.
> >
> > It is true that both C and C++ (including c++14_down and c++17 and later
> > where the latter have different ordering rules) evaluate the lhs of
> > MODIFY_EXPR after rhs, so conceptually this patch makes sense.
>
> Thank you both for taking a look.
>
> > But I wonder why we do in ubsan_maybe_instrument_array_ref:
> > if (e != NULL_TREE)
> > {
> > tree t = copy_node (*expr_p);
> > TREE_OPERAND (t, 1) = build2 (COMPOUND_EXPR, TREE_TYPE (op1),
> > e, op1);
> > *expr_p = t;
> > }
> > rather than modification of the ARRAY_REF's operand in place. If we
> > did that, we wouldn't really care about the order, shared tree would
> > be instrumented once, with SAVE_EXPR in there making sure we don't
> > compute that multiple times. Is that because the 2 copies could
> > have side-effects and we do want to evaluate those multiple times?
>
> I'd assumed that that was the point of the copy_node. But now that
> I'm actually experimenting with it, I can't trigger any problems
> without the copy_node. So maybe we can use the following patch, which
> also adds a new test, bounds-21.c, to check that side-effects are
> evaluated correctly. I didn't bother writing a description for this
> patch yet because I sort of think we should apply both patches at the
> same time.
Perhaps it would be safer to apply for GCC 13 just your first patch
and maybe the testsuite coverage from this one and defer this change
for GCC 14?
> Regtested on x86_64-pc-linux-gnu.
>
> -- >8 --
> PR sanitizer/108060
> PR sanitizer/109050
>
> gcc/c-family/ChangeLog:
>
> * c-ubsan.cc (ubsan_maybe_instrument_array_ref): Don't copy_node.
>
> gcc/testsuite/ChangeLog:
>
> * c-c++-common/ubsan/bounds-17.c: New test.
> * c-c++-common/ubsan/bounds-18.c: New test.
> * c-c++-common/ubsan/bounds-19.c: New test.
> * c-c++-common/ubsan/bounds-20.c: New test.
> * c-c++-common/ubsan/bounds-21.c: New test.
Jakub
