On Wed, Jul 31, 2013 at 02:52:39PM -0400, Jason Merrill wrote:
> On 07/31/2013 01:33 PM, Marek Polacek wrote:
> >There are still at least two issues though, which is why
> >bootstrap with -fsanitize=undefined fails:
> >
> >http://gcc.gnu.org/ml/gcc-patches/2013-07/msg01480.html
>
> This looks like a serious bug, properly caught by -Wuninitialized.
>
> >When sanitizing,
> >in .uninit1 we have
> > int x.3;
> > int x.2;
> >
> > <bb 2>:
> > x.3_3 = x.2_1(D) >> 1;
> > x = x.3_3;
>
> Note that x.2 is not initialized.
Uh, dunno what I was thinking. Oh, right, the fact that x.2_1 is
SSA_NAME_IS_DEFAULT_DEF confused me -- I though it's actually
initialized to 0, as x is a global var...
It really is a genuine bug. The problem can be seen when the
if-condition in the COMPOUND_EXPR doesn't contain x, e.g. in C89
mode we instrument shift with the simple check:
int y = if ((unsigned int) SAVE_EXPR <o> > 31)
{
__builtin___ubsan_handle_shift_out_of_bounds (&*.Lubsan_data0, (unsigned
long) SAVE_EXPR <x>, (unsigned long) SAVE_EXPR <o>);
}
else
{
0
}, SAVE_EXPR <x> << SAVE_EXPR <o>;;
Now, we gimplify this. SAVE_EXPRs are evaluated only once;
in that COMPOUND_EXPR, when we first encounter SAVE_EXPR <x> and
call get_initialized_tmp_var, we get temporary value for it, x.2.
Then, in the second part of the COMPOUND_EXPR, we meet SAVE_EXPR <x>
again, but it already has been resolved, so we don't create any
initialized var for it. But then we end up with
if (o.1 > 31) goto <D.1462>; else goto <D.1463>;
<D.1462>:
D.1464 = (unsigned long) o.0;
x.2 = x;
D.1466 = (unsigned long) x.2;
__builtin___ubsan_handle_shift_out_of_bounds (&*.Lubsan_data0, D.1466,
D.1464);
goto <D.1467>;
<D.1463>:
<D.1467>:
y = x.2 << o.0;
which means that when o.1 > 31 is false, we jump to D.1463, but
the x.2 is not initialized!
What we could perhaps do is to move the x.2 = x; initialization
before the condition, so that it's always initialized. It's not readily
obvious to me how to implement that nicely, but I could try something, if
this is the way to go. Does anyone have a better idea?
> >and when no sanitizing
> > int x.1;
> > int x.0;
> >
> > <bb 2>:
> > x.0_2 = x;
> > x.1_3 = x.0_2 >> 1;
> > x = x.1_3;
>
> But here x.0 is initialized.
>
> >http://gcc.gnu.org/ml/gcc-patches/2013-07/msg01536.html
>
> Here, the C++ compiler is wrong to fold away the division by zero,
> but given that bug the folding ought to also eliminate the call to
> the sanitize function. Seems like you should attach the call to the
> questionable expression itself.
Thanks, I'll look at this later.
Marek
