[backport proposed but not yet approved or applied fix for PR57656 ]
List-Archive:
Date: Mon, 24 Jun 2013 16:56:08 +0200 (CEST)
From: Richard Biener
Subject: [PATCH][RFC] Fix PR57656
This fixes the miscompile in PR57656 - folding of
int t = 1 - (a - b) / c;
to
int t = (b - a) / c + 1;
which introduces intermediate undefined overflow for
a == -1 and b == INT_MAX.
There seems to be a mix of assumptions of the context
negate_expr_p is called in - for example the comments
and the code suggests that when checking whether
negate_expr_p (A - B) is true then we assume there
is a user-written - (A - B) expression (and thus
we'll at most remove an overflow). But we happily
recurse for binary operands (which means re-associating
the negate with the operation) for multiplications
and divisions. Considering - ((A - B) * C) then the
folding to (B - A) * C _introduces_ an intermediate
overflow if C is zero and A == -1 and B == INT_MAX.
So with the notion that the input is always a negation
present in user code the recursion is wrong. And
if not, then at least the MINUS_EXPR case is wrong.
For maximum benefit the API should probably be split
so that it specifies whether there is a negation in
place that we want to remove or not which we then
can use for the recursion.
Anyway - here is a patch that fixes the testcase by
simply avoiding the recursion in the division case,
handling only two obvious cases as any re-org as
outlined above is certainly not good for backporting.
(and a re-org attempt should probably try to unify
the three functions negate_expr_p, negate_expr and
fold_negate_expr again)
Bootstrapped and tested on x86_64-unknown-linux-gnu.
Any comments? Is the patch below ok for trunk and branches
in the mean time?
Thanks,
Richard.
2013-06-24 Richard Biener
PR middle-end/57656
* fold-const.c (negate_expr_p): Fix division case.
(negate_expr): Likewise.
* gcc.dg/torture/pr57656.c: New testcase.
--- gcc-4.7.3/gcc/fold-const.c.~1~ 2013-02-19 18:26:04.000000000 +0100
+++ gcc-4.7.3/gcc/fold-const.c 2013-06-29 13:00:51.831191143 +0200
@@ -486,11 +486,24 @@ negate_expr_p (tree t)
and actually traps on some architectures. But if overflow is
undefined, we can negate, because - (INT_MIN / 1) is an
overflow. */
- if (INTEGRAL_TYPE_P (TREE_TYPE (t))
- && !TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (t)))
- break;
- return negate_expr_p (TREE_OPERAND (t, 1))
- || negate_expr_p (TREE_OPERAND (t, 0));
+ if (INTEGRAL_TYPE_P (TREE_TYPE (t)))
+ {
+ if (!TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (t)))
+ break;
+ /* If overflow is undefined then we have to be careful because
+ we ask whether it's ok to associate the negate with the
+ division which is not ok for example for
+ -((a - b) / c) where (-(a - b)) / c may invoke undefined
+ overflow because of negating INT_MIN. So do not use
+ negate_expr_p here but open-code the two important cases. */
+ if (TREE_CODE (TREE_OPERAND (t, 0)) == NEGATE_EXPR
+ || (TREE_CODE (TREE_OPERAND (t, 0)) == INTEGER_CST
+ && may_negate_without_overflow_p (TREE_OPERAND (t, 0))))
+ return true;
+ }
+ else if (negate_expr_p (TREE_OPERAND (t, 0)))
+ return true;
+ return negate_expr_p (TREE_OPERAND (t, 1));
case NOP_EXPR:
/* Negate -((double)float) as (double)(-float). */
@@ -670,16 +683,20 @@ fold_negate_expr (location_t loc, tree t
return fold_build2_loc (loc, TREE_CODE (t), type,
TREE_OPERAND (t, 0), negate_expr (tem));
}
+ /* If overflow is undefined then we have to be careful because
+ we ask whether it's ok to associate the negate with the
+ division which is not ok for example for
+ -((a - b) / c) where (-(a - b)) / c may invoke undefined
+ overflow because of negating INT_MIN. So do not use
+ negate_expr_p here but open-code the two important cases. */
tem = TREE_OPERAND (t, 0);
- if (negate_expr_p (tem))
- {
- if (INTEGRAL_TYPE_P (type)
- && (TREE_CODE (tem) != INTEGER_CST
- || tree_int_cst_equal (tem, TYPE_MIN_VALUE (type))))
- fold_overflow_warning (warnmsg, WARN_STRICT_OVERFLOW_MISC);
- return fold_build2_loc (loc, TREE_CODE (t), type,
- negate_expr (tem), TREE_OPERAND (t, 1));
- }
+ if ((INTEGRAL_TYPE_P (type)
+ && (TREE_CODE (tem) == NEGATE_EXPR
+ || (TREE_CODE (tem) == INTEGER_CST
+ && may_negate_without_overflow_p (tem))))
+ || !INTEGRAL_TYPE_P (type))
+ return fold_build2_loc (loc, TREE_CODE (t), type,
+ negate_expr (tem), TREE_OPERAND (t, 1));
}
break;
--- gcc-4.7.3/gcc/testsuite/gcc.dg/torture/pr57656.c.~1~ 1970-01-01 01:00:00.000000000 +0100
+++ gcc-4.7.3/gcc/testsuite/gcc.dg/torture/pr57656.c 2013-06-29 13:00:51.831191143 +0200
@@ -0,0 +1,13 @@
+/* { dg-do run } */
+/* { dg-options "-fstrict-overflow" } */
+
+int main (void)
+{
+ int a = -1;
+ int b = __INT_MAX__;
+ int c = 2;
+ int t = 1 - ((a - b) / c); // t = 1 - ( __INT_MIN__ / 2 )
+ if (t != (1 - (-1 - __INT_MAX__) / 2))
+ __builtin_abort();
+ return 0;
+}