Re: [PATCH] Fix ubsan tree sharing (PR sanitizer/66908)

[Jakub Jelinek <jakub at redhat dot com>]

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.

	Jakub