Re: Request to merge Undefined Behavior Sanitizer in (take 2)

[Marek Polacek <polacek at redhat dot com>]

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