On Wed, Sep 23, 2015 at 01:08:53PM +0200, Bernd Schmidt wrote:
> On 09/22/2015 05:11 PM, Marek Polacek wrote:
>
> >diff --git gcc/c-family/c-ubsan.c gcc/c-family/c-ubsan.c
> >index e0cce84..d2bc264 100644
> >--- gcc/c-family/c-ubsan.c
> >+++ gcc/c-family/c-ubsan.c
> >@@ -104,6 +104,7 @@ ubsan_instrument_division (location_t loc, tree op0,
> >tree op1)
> > }
> > }
> > t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), unshare_expr (op0), t);
> >+ t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), unshare_expr (op1), t);
> > if (flag_sanitize_undefined_trap_on_error)
> > tt = build_call_expr_loc (loc, builtin_decl_explicit (BUILT_IN_TRAP),
> > 0);
> > else
>
> I really don't know this code, but just before the location you're patching,
> there's this:
>
> /* In case we have a SAVE_EXPR in a conditional context, we need to
> make sure it gets evaluated before the condition. If the OP0 is
> an instrumented array reference, mark it as having side effects so
> it's not folded away. */
> if (flag_sanitize & SANITIZE_BOUNDS)
> {
> tree xop0 = op0;
> while (CONVERT_EXPR_P (xop0))
> xop0 = TREE_OPERAND (xop0, 0);
> if (TREE_CODE (xop0) == ARRAY_REF)
> {
> TREE_SIDE_EFFECTS (xop0) = 1;
> TREE_SIDE_EFFECTS (op0) = 1;
> }
> }
>
> Does that need to be done for op1 as well? (I really wonder why this is
> needed or whether it's sufficient to find such an ARRAY_REF if you can have
> more complex operands).
Good point. I've dug into this and that hunk doesn't seem to be needed
(anymore?). I suppose there was a reason I added that, but removing it
doesn't seem to break anything. It can be triggered with a code like:
struct S
{
unsigned long a[1];
int l;
};
static inline unsigned long
fn (const struct S *s, int i)
{
return s->a[i] / i;
}
int
main ()
{
struct S s;
fn (&s, 1);
}
With the hunk, we sanitize the same array twice -- that's "suboptimal". With
the hunk removed, we sanitize the array just once as expected.
> The same pattern occurs in another function, so it may be best to break it
> out into a new function if additional occurrences are necessary.
Given that the code above seems to be useless now, I think let's put this
patch in as-is, backport it to gcc-5, then remove those redundant hunks on
trunk and add the testcase above. Do you agree?
Marek
