On Thu, Jul 23, 2015 at 02:39:05PM +0200, Jakub Jelinek wrote:
> On Thu, Jul 23, 2015 at 02:20:51PM +0200, Marek Polacek wrote:
> > > So isn't the bug instead that the UBSAN_BOUNDS generating code doesn't
> > > unshare? Of course, these two functions use op0 and/or op1 sometimes
> > > multiple times too and thus they might want to unshare too, but I'd have
> > > expected in a different spot.
> >
> > I don't think so: I think I need to unshare op0 and op1 because the shift
> > instrumentation always duplicates them for the __ubsan_* call.
> > Doing the unsharing in bounds instrumentation is too late (and none of my
> > attempts worked).
> >
> > I can move the unsharing a little bit below, just before creating the
> > BUILT_IN_UBSAN_HANDLE_SHIFT_OUT_OF_BOUNDS call, but otherwise I have no
> > better ideas.
> >
> > So for now I don't have a better patch, sorry.
>
> Actually looking at the callers, your patch is indeed right. They pass
> this op0/op1 to these functions and then use the values as well elsewhere
> (in the actual operation).
>
> Whether it is also needed more times in the body of the functions
> (if you use op0 and/or op1 in the functions more than once in the generated
> expressions) remains to be seen. Of course during gimplification we unshare
> everything shared in the function body, but that might be too late.
> E.g. in ubsan_instrument_division at
> tt = fold_build2 (EQ_EXPR, boolean_type_node, op1,
> build_int_cst (type, -1));
> x = fold_build2 (EQ_EXPR, boolean_type_node, op0,
> TYPE_MIN_VALUE (type));
> you already know op1 has been used once in t, while op0 has not been used
> yet, so unshare_expr (op1) might be desirable. And you know that unless
> if (integer_zerop (t)) op0 will be used afterwards, so you might as well
> unshare_expr (op0) too. Then you use
> t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), op0, t);
> unconditionally. And finally there is:
> tt = build_call_expr_loc (loc, tt, 3, data, ubsan_encode_value (op0),
> ubsan_encode_value (op1));
> which you might want to unshare_expr first (both).
>
> So, please go ahead with your patch and as follow-up, see if further
> unshare_exprs don't need to be added.
Thanks. Ok, I will; and I hope that the follow up will fix the remaining
issue Maxim is seeing on ARM.
Marek
