[PATCH] Fix folding of -(unsigned)(a * -b)

[PATCH] Fix folding of -(unsigned)(a * -b)

[Richard Guenther]

Folding of $subject is currently broken (noticed that when playing
with types in pointer_int_sum).  We happily ignore the fact
that the negate operates on an unsigned type and change it to
operate on a signed one - which may cause new undefined overflow.
Seen with the testcase below which aborts with current trunk.

The fix is to not strip sign-changing conversions as already
done for ABS_EXPR.

Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.

Richard.

2011-07-07  Richard Guenther  <rguenther@suse.de>

	* fold-const.c (fold_unary_loc): Do not strip sign-changes
	for NEGATE_EXPR.

	* gcc.dg/ftrapv-3.c: New testcase.

Index: gcc/fold-const.c
===================================================================
--- gcc/fold-const.c	(revision 175962)
+++ gcc/fold-const.c	(working copy)
@@ -7561,7 +7561,7 @@ fold_unary_loc (location_t loc, enum tre
   if (arg0)
     {
       if (CONVERT_EXPR_CODE_P (code)
-	  || code == FLOAT_EXPR || code == ABS_EXPR)
+	  || code == FLOAT_EXPR || code == ABS_EXPR || code == NEGATE_EXPR)
 	{
 	  /* Don't use STRIP_NOPS, because signedness of argument type
 	     matters.  */
Index: gcc/testsuite/gcc.dg/ftrapv-3.c
===================================================================
--- gcc/testsuite/gcc.dg/ftrapv-3.c	(revision 0)
+++ gcc/testsuite/gcc.dg/ftrapv-3.c	(revision 0)
@@ -0,0 +1,16 @@
+/* { dg-do run } */
+/* { dg-options "-ftrapv" } */
+
+extern void abort (void);
+unsigned long
+foo (long i, long j)
+{
+  /* We may not fold this to (unsigned long)(i * j).  */
+  return -(unsigned long)(i * -j);
+}
+int main()
+{
+  if (foo (-__LONG_MAX__ - 1, -1) != -(unsigned long)(-__LONG_MAX__ - 1))
+    abort ();
+  return 0;
+}

Re: [PATCH] Fix folding of -(unsigned)(a * -b)

[Michael Matz]

Hi,

On Thu, 7 Jul 2011, Richard Guenther wrote:

> Index: gcc/fold-const.c
> ===================================================================
> --- gcc/fold-const.c	(revision 175962)
> +++ gcc/fold-const.c	(working copy)
> @@ -7561,7 +7561,7 @@ fold_unary_loc (location_t loc, enum tre
>    if (arg0)
>      {
>        if (CONVERT_EXPR_CODE_P (code)
> -	  || code == FLOAT_EXPR || code == ABS_EXPR)
> +	  || code == FLOAT_EXPR || code == ABS_EXPR || code == NEGATE_EXPR)
>  	{
>  	  /* Don't use STRIP_NOPS, because signedness of argument type
>  	     matters.  */

Um, so why would stripping a signchange ever be okay?  There are many 
other unary codes that behave similar enough to FLOAT_EXPR, or 
CONVERT_EXPR that it's not obvious to me why those would allow sign 
stripping but the above not.  When the operand is float or fixed point 
types then STRIP_SIGN_NOPS and STRIP_NOPS aren't different, and when the 
operands are integer types I don't see how we can ignore sign-changing 
nops.  I'm thinking about:
VEC_UNPACK_HI_EXPR, VEC_UNPACK_LO_EXPR and PAREN_EXPR
Perhaps BIT_NOT_EXPR.  Perhaps also NON_LVALUE_EXPR.  All these can 
conceivably have integer operands, where signedness seems to matter.

I think these are harmless: CONJ_EXPR, FIXED_CONVERT_EXPR, FIX_TRUNC_EXPR, 
ADDR_SPACE_CONVERT_EXPR as their operands are either float/fixed-point 
types or pointers, but as said in those cases STRIP_NOPS and 
STRIP_SIGN_NOPS are equivalent.

So, why not simply always use STRIP_SIGN_NOPS?


Ciao,
Michael.