On Mon, 24 Jun 2013, Richard Biener wrote:

> 
> 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?

Re-bootstrapped and tested on x86_64-unknown-linux-gnu and
committed on trunk sofar.

Richard.

> 
> Thanks,
> Richard.
> 
> 2013-06-24  Richard Biener  <rguent...@suse.de>
> 
>       PR middle-end/57656
>       * fold-const.c (negate_expr_p): Fix division case.
>       (negate_expr): Likewise.
> 
>       * gcc.dg/torture/pr57656.c: New testcase.
> 
> Index: gcc/fold-const.c
> ===================================================================
> *** gcc/fold-const.c  (revision 200363)
> --- gcc/fold-const.c  (working copy)
> *************** negate_expr_p (tree t)
> *** 483,493 ****
>        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));
>   
>       case NOP_EXPR:
>         /* Negate -((double)float) as (double)(-float).  */
> --- 483,506 ----
>        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)))
> !     {
> !       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).  */
> *************** fold_negate_expr (location_t loc, tree t
> *** 682,697 ****
>             return fold_build2_loc (loc, TREE_CODE (t), type,
>                                 TREE_OPERAND (t, 0), negate_expr (tem));
>           }
>             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));
> !         }
>           }
>         break;
>   
> --- 695,714 ----
>             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 ((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;
>   
> Index: gcc/testsuite/gcc.dg/torture/pr57656.c
> ===================================================================
> *** gcc/testsuite/gcc.dg/torture/pr57656.c    (revision 0)
> --- gcc/testsuite/gcc.dg/torture/pr57656.c    (working copy)
> ***************
> *** 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;
> + }
> 

-- 
Richard Biener <rguent...@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend

Reply via email to