On Wed, Jul 22, 2015 at 07:43:23PM +0200, Jakub Jelinek wrote:
> On Wed, Jul 22, 2015 at 07:26:22PM +0200, Marek Polacek wrote:
> > In this testcase we were generating an uninitialized variable when doing
> > -fsanitize=shift,bounds sanitization. The shift instrumentation is done
> > first; after that, the IR looks like
> >
> > res[i] = (m > 31) ? __ubsan (... tab[i] ...) ? 0, ... tab[i] ...;
> >
> > where tab[i] are identical. That means that when we instrument the first
> > tab[i] (we shouldn't do this I suppose), the second tab[i] is changed as
> > well as they're shared. But that doesn't play well with SAVE_EXPRs, because
> > SAVE_EXPR <i> would only be initialized on one path. Fixed by unsharing
> > the operands when constructing the ubsan check. The .gimple diff is in
> > essence just
> >
> > + i.2 = i;
> > + UBSAN_BOUNDS (0B, i.2, 21);
> > - UBSAN_BOUNDS (0B, i.1, 21);
> >
> > (Merely not instrumenting __ubsan_* wouldn't help exactly because of the
> > sharing.)
> >
> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> That is strange. I'd have expected you'd want to unshare if you want to use
> the same operand multiple times in the same function, instead of unsharing
> it just in case it is shared with something different.
>
> 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.
Marek
