Request to merge Undefined Behavior Sanitizer in (take 2)

On Wed, Jul 31, 2013 at 02:52:39PM -0400, Jason Merrill wrote:
> >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.

Hm, actually, we can't easily fold the call to the sanitize function
away, I'm afraid, if we want to do it for the 'case <something>'
case.  When we hit the DIV_EXPR in 'case 0 * (1 / 0)',
the ubsan_instrument_division gets 1 as a first argument and 0 as
a second argument, but due to fold_builds in the
ubsan_instrument_division, we replace the case value with just the call
to the __builtin___ubsan_handle_divrem_overflow.  I think, what we
could do, is to tweak verify_constant like this:


The logic behind this is that when we can't easily insert the
ubsan builtin call itself (so that it would get called unconditionally
at the runtime), at least error out; at that point the behavior
is certainly undefined thus we're free to do whatever.  I'd say it's better
than to e.g. insert 0 instead of the expression and carry on.
Also, this could be useful in other situations (at places, where
we can't insert the ubsan builtin call, but we *know* that the 
behavior is undefined).  Yes, the behavior
can be undefined only at runtime, but, this would happen only when
sanitizing and in that case is desirable to report even that.
The is_ubsan_builtin fn is not yet implemented, but you get the idea.

Thoughts?  Thanks,

	Marek

On 08/06/2013 10:42 AM, Marek Polacek wrote:
> Hm, actually, we can't easily fold the call to the sanitize function
> away, I'm afraid, if we want to do it for the 'case <something>'
> case.  When we hit the DIV_EXPR in 'case 0 * (1 / 0)',
> the ubsan_instrument_division gets 1 as a first argument and 0 as
> a second argument, but due to fold_builds in the
> ubsan_instrument_division, we replace the case value with just the call
> to the __builtin___ubsan_handle_divrem_overflow.

Ah, and the call isn't folded away because it has side-effects.

> I think, what we could do, is to tweak verify_constant like this:
>
> +  /* This is to handle e.g. the goofy 'case 0 * (1 / 0)' case.  */
> +  if (flag_sanitize & SANITIZE_UNDEFINED
> +      && TREE_CODE (t) == CALL_EXPR
> +      && is_ubsan_builtin (t))
> +    {
> +      error ("undefined behavior occured");
> +      return *non_constant_p;
> +    }

I think I'd rather handle ubsan builtins specially in dump_expr.

Jason

On Tue, Aug 06, 2013 at 07:07:27PM -0400, Jason Merrill wrote:
> >I think, what we could do, is to tweak verify_constant like this:
> >
> >+  /* This is to handle e.g. the goofy 'case 0 * (1 / 0)' case.  */
> >+  if (flag_sanitize & SANITIZE_UNDEFINED
> >+      && TREE_CODE (t) == CALL_EXPR
> >+      && is_ubsan_builtin (t))
> >+    {
> >+      error ("undefined behavior occured");
> >+      return *non_constant_p;
> >+    }
> 
> I think I'd rather handle ubsan builtins specially in dump_expr.

I might've misunderstood what you mean.  If we drop the hunk above,
then we'll call 
    error ("%q+E is not a constant expression", t);
so, we'll print "is not a constant expression" no matter what, we
surely can recognize the ubsan built-ins in dump_expr, but what would
we do then?

	Marek

On 08/07/2013 06:06 AM, Marek Polacek wrote:
> I might've misunderstood what you mean.  If we drop the hunk above,
> then we'll call
>      error ("%q+E is not a constant expression", t);
> so, we'll print "is not a constant expression" no matter what

Yes, that's fine; 1/0 is not a constant expression, because it has 
undefined behavior.

> we surely can recognize the ubsan built-ins in dump_expr, but what would
> we do then?

Print something more meaningful to the user.

Jason