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:

--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -6938,6 +6938,14 @@ static bool
 verify_constant (tree t, bool allow_non_constant, bool *non_constant_p,
                 bool *overflow_p)
 {
+  /* 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;
+    }
   if (!*non_constant_p && !reduced_constant_expression_p (t))
     {
       if (!allow_non_constant)

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

Reply via email to