diff mbox

Don't fold away division by zero (PR middle-end/66127)

Message ID 20150513134111.GA27320@redhat.com
State New
Headers show

Commit Message

Marek Polacek May 13, 2015, 1:41 p.m. UTC 
As discussed in the PR, match.pd happily folds 0 * whatever into 0.  That
is undesirable from the C/C++ FE POV, since it can make us accept invalid
initializers.

So fixed in match.pd -- I'd hope there's a better way to do this, but this
seems to work.  There was some fallout, but nothing unexpected or surprising.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2015-05-13  Marek Polacek  <polacek@redhat.com>

	PR middle-end/66127
	* match.pd (0 * X): Preserve divisions by zero.

	* g++.dg/warn/overflow-warn-1.C: Adjust dg-warning and dg-error.
	* g++.dg/warn/overflow-warn-3.C: Likewise.
	* g++.dg/warn/overflow-warn-4.C: Likewise.
	* g++.dg/ubsan/div-by-zero-1.C: Likewise.


	Marek

Comments

Jakub Jelinek May 13, 2015, 1:55 p.m. UTC | #1 
On Wed, May 13, 2015 at 03:41:11PM +0200, Marek Polacek wrote:
> As discussed in the PR, match.pd happily folds 0 * whatever into 0.  That
> is undesirable from the C/C++ FE POV, since it can make us accept invalid
> initializers.
> 
> So fixed in match.pd -- I'd hope there's a better way to do this, but this
> seems to work.  There was some fallout, but nothing unexpected or surprising.

Will it handle cases 0 * (int) (1 / 0) etc., when perhaps the division by
zero isn't immediately the operand of mult, but somewhere deeper?
Also, can't the divisor be optimized into 0 only later on, so your code
would still see !integer_zerop there and fold into 0?
Perhaps the answer is that in both cases we'd have simplified those already
into a division by zero.

	Jakub
Marek Polacek May 13, 2015, 2:11 p.m. UTC | #2 
On Wed, May 13, 2015 at 03:55:10PM +0200, Jakub Jelinek wrote:
> On Wed, May 13, 2015 at 03:41:11PM +0200, Marek Polacek wrote:
> > As discussed in the PR, match.pd happily folds 0 * whatever into 0.  That
> > is undesirable from the C/C++ FE POV, since it can make us accept invalid
> > initializers.
> > 
> > So fixed in match.pd -- I'd hope there's a better way to do this, but this
> > seems to work.  There was some fallout, but nothing unexpected or surprising.
> 
> Will it handle cases 0 * (int) (1 / 0) etc., when perhaps the division by
> zero isn't immediately the operand of mult, but somewhere deeper?

It won't handle e.g. 0 * (unsigned) (1 / 0).

> Also, can't the divisor be optimized into 0 only later on, so your code
> would still see !integer_zerop there and fold into 0?
> Perhaps the answer is that in both cases we'd have simplified those already
> into a division by zero.

Yes, it's a dumb attempt.

I don't know how to reliably fix this :(.  We really want 
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66127#c1>...

	Marek
Marek Polacek May 13, 2015, 2:17 p.m. UTC | #3 
On Wed, May 13, 2015 at 04:11:15PM +0200, Marek Polacek wrote:
> I don't know how to reliably fix this :(.

Except disabling (0 * X) -> 0 completely, that is.

	Marek
Richard Biener May 13, 2015, 2:19 p.m. UTC | #4 
On Wed, 13 May 2015, Marek Polacek wrote:

> On Wed, May 13, 2015 at 03:55:10PM +0200, Jakub Jelinek wrote:
> > On Wed, May 13, 2015 at 03:41:11PM +0200, Marek Polacek wrote:
> > > As discussed in the PR, match.pd happily folds 0 * whatever into 0.  That
> > > is undesirable from the C/C++ FE POV, since it can make us accept invalid
> > > initializers.
> > > 
> > > So fixed in match.pd -- I'd hope there's a better way to do this, but this
> > > seems to work.  There was some fallout, but nothing unexpected or surprising.
> > 
> > Will it handle cases 0 * (int) (1 / 0) etc., when perhaps the division by
> > zero isn't immediately the operand of mult, but somewhere deeper?
> 
> It won't handle e.g. 0 * (unsigned) (1 / 0).
> 
> > Also, can't the divisor be optimized into 0 only later on, so your code
> > would still see !integer_zerop there and fold into 0?
> > Perhaps the answer is that in both cases we'd have simplified those already
> > into a division by zero.
> 
> Yes, it's a dumb attempt.
> 
> I don't know how to reliably fix this :(.  We really want 
> <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66127#c1>...

Yeah, I think we can't reliably "avoid" folding away traps like this
so any attempt is useless (and we should simply not do this).

The only way I see is to make all expressions that might trap set
TREE_SIDE_EFFECTS on the expression, so we'd fold it to
a, 0.

Richard.
Richard Biener May 13, 2015, 2:19 p.m. UTC | #5 
On Wed, 13 May 2015, Marek Polacek wrote:

> On Wed, May 13, 2015 at 04:11:15PM +0200, Marek Polacek wrote:
> > I don't know how to reliably fix this :(.
> 
> Except disabling (0 * X) -> 0 completely, that is.

Well, make sure that X has TREE_SIDE_EFFECTS set.  That way we'd fold
to 0, X.

Richard.
Joseph Myers May 13, 2015, 3:06 p.m. UTC | #6 
On Wed, 13 May 2015, Marek Polacek wrote:

> On Wed, May 13, 2015 at 03:55:10PM +0200, Jakub Jelinek wrote:
> > On Wed, May 13, 2015 at 03:41:11PM +0200, Marek Polacek wrote:
> > > As discussed in the PR, match.pd happily folds 0 * whatever into 0.  That
> > > is undesirable from the C/C++ FE POV, since it can make us accept invalid
> > > initializers.
> > > 
> > > So fixed in match.pd -- I'd hope there's a better way to do this, but this
> > > seems to work.  There was some fallout, but nothing unexpected or surprising.
> > 
> > Will it handle cases 0 * (int) (1 / 0) etc., when perhaps the division by
> > zero isn't immediately the operand of mult, but somewhere deeper?
> 
> It won't handle e.g. 0 * (unsigned) (1 / 0).

I think this illustrates why handling this in the folding-for-optimization 
is a mistake.

For optimization, it's perfectly fine to fold away 0 * (something without 
side effects), and division by 0 should only be considered to have side 
effects if language semantic options were specified to that effect (such 
as using ubsan), which should then have the effect of producing GENERIC 
that's not a simple division but represents whatever checks are required.

Rather, how about having an extra argument to c_fully_fold_internal (e.g. 
for_int_const) indicating that the folding is of an expression with 
integer constant operands (so this argument would be true in the new case 
of folding the contents of a C_MAYBE_CONST_EXPR with 
C_MAYBE_CONST_EXPR_INT_OPERANDS set)?  In that special case, 
c_fully_fold_internal would only fold the expression itself if all 
evaluated operands folded to INTEGER_CSTs (so if something that doesn't 
get folded, such as division by 0, is encountered, then all evaluated 
expressions containing it also don't get folded).
diff mbox

Patch

diff --git gcc/match.pd gcc/match.pd
index 51a950a..510e2db 100644
--- gcc/match.pd
+++ gcc/match.pd
@@ -78,7 +78,19 @@  along with GCC; see the file COPYING3.  If not see
 
 (simplify
  (mult @0 integer_zerop@1)
- @1)
+ /* Make sure to preserve divisions by zero.  */
+ (with { enum tree_code code = TREE_CODE (@0); }
+  (if ((code != TRUNC_DIV_EXPR
+	&& code != EXACT_DIV_EXPR
+	&& code != ROUND_DIV_EXPR
+	&& code != FLOOR_DIV_EXPR
+	&& code != CEIL_DIV_EXPR
+	&& code != TRUNC_MOD_EXPR
+	&& code != CEIL_MOD_EXPR
+	&& code != FLOOR_MOD_EXPR
+	&& code != ROUND_MOD_EXPR)
+       || !integer_zerop (TREE_OPERAND (@0, 1)))
+ @1)))
 
 /* Maybe fold x * 0 to 0.  The expressions aren't the same
    when x is NaN, since x * 0 is also NaN.  Nor are they the
--- gcc/testsuite/g++.dg/warn/overflow-warn-1.C
+++ gcc/testsuite/g++.dg/warn/overflow-warn-1.C
@@ -17,7 +17,7 @@  enum e {
   /* But as in DR#031, the 1/0 in an evaluated subexpression means the
      whole expression violates the constraints.  */
   E4 = 0 * (1 / 0), /* { dg-warning "division by zero" } */
-  /* { dg-error "enumerator value for 'E4' is not an integer constant" "enum error" { xfail *-*-* } 19 } */
+  /* { dg-error "enumerator value for 'E4' is not an integer constant|not a constant.expression" "enum error" { target *-*-* } 19 } */
   E5 = INT_MAX + 1, /* { dg-warning "integer overflow in expression" } */
   /* { dg-warning "overflow in constant expression" "constant" { target *-*-* } 21 } */
   /* Again, overflow in evaluated subexpression.  */
@@ -30,8 +30,9 @@  enum e {
 struct s {
   int a;
   int : 0 * (1 / 0); /* { dg-warning "division by zero" } */
+  /* { dg-error "not an integer constant|not a constant.expression" "bit-field error" { target *-*-* } 32 } */
   int : 0 * (INT_MAX + 1); /* { dg-warning "integer overflow in expression" } */
-  /* { dg-warning "overflow in constant expression" "constant" { target *-*-* } 33 } */
+  /* { dg-warning "overflow in constant expression" "constant" { target *-*-* } 34 } */
 };
 
 void
@@ -45,7 +46,6 @@  f (void)
 /* This expression is neither required to be constant.  */
 static int sc = INT_MAX + 1; /* { dg-warning "integer overflow in expression" } */
 
-
 // Test for overflow in null pointer constant.  
 void *n = 0;
 /* The first two of these involve overflow, so are not null pointer
@@ -54,7 +54,7 @@  void *n = 0;
 void *p = 0 * (INT_MAX + 1); /* { dg-warning "integer overflow in expression" } */
 /* { dg-warning "invalid conversion from 'int' to 'void" "null" { target *-*-* } 54 } */
 void *q = 0 * (1 / 0); /* { dg-warning "division by zero" } */
-/* { dg-error "invalid conversion from 'int' to 'void*'" "null" { xfail *-*-* } 56 } */
+/* { dg-warning "invalid conversion from 'int' to 'void\\*'" "null" { target *-*-* } 56 } */
 void *r = (1 ? 0 : INT_MAX+1); /* { dg-bogus "integer overflow in expression" "" { xfail *-*-* } } */
 
 void
@@ -63,9 +63,10 @@  g (int i)
   switch (i)
     {
     case 0 * (1/0): /* { dg-warning "division by zero" } */
+      /* { dg-error "not a constant.expression" "case error" { target *-*-* } 65 } */
       ;
     case 1 + 0 * (INT_MAX + 1): /* { dg-warning "integer overflow in expression" } */
-      /* { dg-warning "overflow in constant expression" "constant" { target *-*-* } 67 } */
+      /* { dg-warning "overflow in constant expression" "constant" { target *-*-* } 68 } */
       ;
     }
 }
--- gcc/testsuite/g++.dg/warn/overflow-warn-3.C
+++ gcc/testsuite/g++.dg/warn/overflow-warn-3.C
@@ -17,7 +17,7 @@  enum e {
   /* But as in DR#031, the 1/0 in an evaluated subexpression means the
      whole expression violates the constraints.  */
   E4 = 0 * (1 / 0), /* { dg-warning "division by zero" } */
-  /* { dg-error "enumerator value for 'E4' is not an integer constant" "enum error" { xfail *-*-* } 19 } */
+  /* { dg-error "enumerator value for 'E4' is not an integer constant|not a constant.expression" "enum error" { target *-*-* } 19 } */
   E5 = INT_MAX + 1, /* { dg-warning "integer overflow in expression" } */
   /* { dg-warning "overflow in constant expression" "constant" { target *-*-* } 21 } */
   /* Again, overflow in evaluated subexpression.  */
@@ -30,8 +30,9 @@  enum e {
 struct s {
   int a;
   int : 0 * (1 / 0); /* { dg-warning "division by zero" } */
+  /* { dg-error "not an integer constant|not a constant.expression" "bit-field error" { target *-*-* } 32 } */
   int : 0 * (INT_MAX + 1); /* { dg-warning "integer overflow in expression" } */
-  /* { dg-warning "overflow in constant expression" "constant" { target *-*-* } 33 } */
+  /* { dg-warning "overflow in constant expression" "constant" { target *-*-* } 34 } */
 };
 
 void
@@ -40,7 +41,6 @@  f (void)
   /* This expression is not required to be a constant expression, so
      it should just involve undefined behavior at runtime.  */
   int c = INT_MAX + 1; /* { dg-warning "integer overflow in expression" } */
-
 }
 
 /* This expression is neither required to be constant.  */
@@ -56,7 +56,7 @@  void *p = 0 * (INT_MAX + 1); /* { dg-warning "integer overflow in expression" }
 /* { dg-warning "invalid conversion from 'int' to 'void" "null" { target *-*-* } 55 } */
 
 void *q = 0 * (1 / 0); /* { dg-warning "division by zero" } */
-/* { dg-warning "invalid conversion from 'int' to 'void*'" "null" { xfail *-*-* } 58 } */
+/* { dg-warning "invalid conversion from 'int' to 'void\\*'" "null" { target *-*-* } 58 } */
 void *r = (1 ? 0 : INT_MAX+1); /* { dg-bogus "integer overflow in expression" "" { xfail *-*-* } } */
 
 void
@@ -65,9 +65,10 @@  g (int i)
   switch (i)
     {
     case 0 * (1/0): /* { dg-warning "division by zero" } */
+      /* { dg-error "not a constant.expression" "case error" { target *-*-* } 67 } */
       ;
     case 1 + 0 * (INT_MAX + 1): /* { dg-warning "integer overflow in expression" } */
-      /* { dg-warning "overflow in constant expression" "constant" { target *-*-* } 69 } */
+      /* { dg-warning "overflow in constant expression" "constant" { target *-*-* } 70 } */
       ;
     }
 }
--- gcc/testsuite/g++.dg/warn/overflow-warn-4.C
+++ gcc/testsuite/g++.dg/warn/overflow-warn-4.C
@@ -17,7 +17,7 @@  enum e {
   /* But as in DR#031, the 1/0 in an evaluated subexpression means the
      whole expression violates the constraints.  */
   E4 = 0 * (1 / 0), /* { dg-warning "division by zero" } */
-  /* { dg-error "enumerator value for 'E4' is not an integer constant" "enum error" { xfail *-*-* } 19 } */
+  /* { dg-error "enumerator value for 'E4' is not an integer constant|not a constant.expression" "enum error" { target *-*-* } 19 } */
   E5 = INT_MAX + 1, /* { dg-warning "integer overflow in expression" } */
   /* { dg-error "overflow in constant expression" "constant" { target *-*-* } 21 } */
   /* { dg-error "enumerator value for 'E5' is not an integer constant" "enum error" { target *-*-* } 21 } */
@@ -32,9 +32,10 @@  enum e {
 struct s {
   int a;
   int : 0 * (1 / 0); /* { dg-warning "division by zero" } */
+  /* { dg-error "not an integer constant|not a constant.expression" "bit-field error" { target *-*-* } 34 } */
   int : 0 * (INT_MAX + 1); /* { dg-warning "integer overflow in expression" } */
-  /* { dg-error "overflow in constant expression" "constant" { target *-*-* } 35 } */
-  /* { dg-error "bit-field .* width not an integer constant" "" { target *-*-* } 35 } */
+  /* { dg-error "overflow in constant expression" "constant" { target *-*-* } 36 } */
+  /* { dg-error "bit-field .* width not an integer constant" "" { target *-*-* } 36 } */
 };
 
 void
@@ -43,7 +44,6 @@  f (void)
   /* This expression is not required to be a constant expression, so
      it should just involve undefined behavior at runtime.  */
   int c = INT_MAX + 1; /* { dg-warning "integer overflow in expression" } */
-
 }
 
 /* This expression is neither required to be constant.  */
@@ -59,7 +59,7 @@  void *p = 0 * (INT_MAX + 1); /* { dg-warning "integer overflow in expression" }
 /* { dg-error "invalid conversion from 'int' to 'void" "null" { target *-*-* } 58 } */
 
 void *q = 0 * (1 / 0); /* { dg-warning "division by zero" } */
-/* { dg-error "invalid conversion from 'int' to 'void*'" "null" { xfail *-*-* } 61 } */
+/* { dg-error "invalid conversion from 'int' to 'void\\*'" "null" { target *-*-* } 61 } */
 void *r = (1 ? 0 : INT_MAX+1); /* { dg-bogus "integer overflow in expression" "" { xfail *-*-* } } */
 
 void
@@ -68,9 +68,10 @@  g (int i)
   switch (i)
     {
     case 0 * (1/0): /* { dg-warning "division by zero" } */
+      /* { dg-error "not a constant.expression" "case error" { target *-*-* } 70 } */
       ;
     case 1 + 0 * (INT_MAX + 1): /* { dg-warning "integer overflow in expression" } */
-      /* { dg-error "overflow in constant expression" "constant" { target *-*-* } 72 } */
+      /* { dg-error "overflow in constant expression" "constant" { target *-*-* } 73 } */
       ;
     }
 }
--- gcc/testsuite/g++.dg/ubsan/div-by-zero-1.C
+++ gcc/testsuite/g++.dg/ubsan/div-by-zero-1.C
@@ -1,14 +1,10 @@ 
 /* { dg-do compile } */
 /* { dg-options "-fsanitize=integer-divide-by-zero" } */
 
-/* TODO: We expect an error on the invalid case here, because that
-   must be a constant-expression.  This will be fixed when we have
-   proper delayed folding.  */
-
 void
 foo (int i)
 {
   switch (i)
   case 0 * (1 / 0): /* { dg-warning "division by zero" } */
-    ;  /* { dg-error "division by zero" "" { xfail *-*-* } 10 } */
+    ;  /* { dg-error "not a constant.expression" "" { target *-*-* } 8 } */
 }