On Fri, Mar 10, 2023 at 07:07:36PM +0100, Jakub Jelinek wrote: > 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?
That sounds good, I'll push the original patch with the new test now. Thanks. > > 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 > Marek