Bug 50066 - [4.7 Regression] Bad signed int to unsigned long long conversion
Summary: [4.7 Regression] Bad signed int to unsigned long long conversion
Status: RESOLVED INVALID
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.7.0
: P3 normal
Target Milestone: 4.7.0
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-12 23:38 UTC by H.J. Lu
Modified: 2011-11-30 21:32 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2011-11-30 00:00:00


Attachments
A patch (650 bytes, patch)
2011-08-13 18:11 UTC, H.J. Lu
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description H.J. Lu 2011-08-12 23:38:41 UTC
On Linux/ia32, I got

[hjl@gnu-6 gmp-1]$ cat x.c
#include <stdio.h>
#include <stdlib.h>
#include <limits.h>

extern unsigned long long foo (long);

int
main ()
{
  unsigned long long val = foo (LONG_MIN);
  printf ("0x%llx\n", val);
  if (val != 0x80000000)
    abort ();
  return 0;
}
[hjl@gnu-6 gmp-1]$ cat foo.c
unsigned long long
foo (signed long int val)
{
  return (unsigned long long) (unsigned long int) (val >= 0 ? val : -val);
}
[hjl@gnu-6 gmp-1]$ make 
/export/build/gnu/gcc/build-x86_64-linux/gcc/xgcc -B/export/build/gnu/gcc/build-x86_64-linux/gcc/ -m32 -O2   -c -o foo.o foo.c
/export/build/gnu/gcc/build-x86_64-linux/gcc/xgcc -B/export/build/gnu/gcc/build-x86_64-linux/gcc/ -m32 -O2   -c -o x.o x.c
/export/build/gnu/gcc/build-x86_64-linux/gcc/xgcc -B/export/build/gnu/gcc/build-x86_64-linux/gcc/ -m32 -O2 -o x foo.o x.o
./x
0xffffffff80000000
make: *** [all] Aborted (core dumped)
[hjl@gnu-6 gmp-1]$
Comment 1 H.J. Lu 2011-08-12 23:45:55 UTC
It may be caused by revision 176154:

http://gcc.gnu.org/ml/gcc-cvs/2011-07/msg00419.html

When replacing long with int, LONG_MIN with INT_MIN,
the testcase also fails on Linux/x86-64.
Comment 2 Andrew Pinski 2011-08-13 04:32:16 UTC
- LONG_MIN is undefined.
Comment 3 Paolo Carlini 2011-08-13 08:53:47 UTC
Indeed. Seems invalid to me.
Comment 4 H.J. Lu 2011-08-13 12:09:16 UTC
This code comes from mpz/set_si.c in gmp:

void
mpz_set_si (mpz_ptr dest, signed long int val)
{
  mp_size_t size;
  mp_limb_t vl;

  vl = (mp_limb_t) (unsigned long int) (val >= 0 ? val : -val);

mp_limb_t is unsigned 64bit and long is 32bit. It works
with all released GCC versions.
Comment 5 H.J. Lu 2011-08-13 14:27:58 UTC
Does this patch

---
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index df7a9a2..f5e0a30 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -2065,6 +2065,12 @@ vrp_int_const_binop (enum tree_code code, tree val1, tree
 val2)
 	  && is_overflow_infinity (val2))
 	return NULL_TREE;
 
+      /* Punt integer subtraction with overflow on -MIN.  */ 
+      if (code == MINUS_EXPR
+	  && INTEGRAL_TYPE_P (TREE_TYPE (res))
+	  && (sgn1 < 0 || sgn2 < 0))
+	return NULL_TREE;
+
       /* Don't try to handle division or shifting of infinities.  */
       if ((code == TRUNC_DIV_EXPR
 	   || code == FLOOR_DIV_EXPR
---

make any senses?
Comment 6 joseph@codesourcery.com 2011-08-13 15:28:22 UTC
On Sat, 13 Aug 2011, hjl.tools at gmail dot com wrote:

> --- Comment #4 from H.J. Lu <hjl.tools at gmail dot com> 2011-08-13 12:09:16 UTC ---
> This code comes from mpz/set_si.c in gmp:
> 
> void
> mpz_set_si (mpz_ptr dest, signed long int val)
> {
>   mp_size_t size;
>   mp_limb_t vl;
> 
>   vl = (mp_limb_t) (unsigned long int) (val >= 0 ? val : -val);

So that GMP code is buggy.  Change it to

vl = (mp_limb_t) (val >= 0 ? (unsigned long int) val : -(unsigned long int) val);

and it will be valid.
Comment 7 joseph@codesourcery.com 2011-08-13 15:31:05 UTC
(The original code is of course valid if you use -fwrapv, so hopefully the 
problem optimization does not occur in that case.)
Comment 8 H.J. Lu 2011-08-13 15:56:22 UTC
make_overflow_infinity sets to TYPE_MAX_VALUE/TYPE_MIN_VALUE.  Shouldn't
it set to TYPE_MAX_VALUE + 1/TYPE_MIN_VALUE - 1?
Comment 9 H.J. Lu 2011-08-13 16:33:19 UTC
Shouldn't we check TREE_OVERFLOW:

diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index df7a9a2..4ec7e5b 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -7263,6 +7263,8 @@ simplify_conversion_using_ranges (gimple stmt)
   /* Get the value-range of the inner operand.  */
   innervr = get_value_range (innerop);
   if (innervr->type != VR_RANGE
+      || TREE_OVERFLOW (innervr->min)
+      || TREE_OVERFLOW (innervr->max)
       || TREE_CODE (innervr->min) != INTEGER_CST
       || TREE_CODE (innervr->max) != INTEGER_CST)
     return false;
Comment 10 Andrew Pinski 2011-08-13 17:51:33 UTC
HJL, the code as written in comment #0 is undefined.  Does the rewrite in comment #6 work?  Also does adding -fwrapv work too?
Comment 11 H.J. Lu 2011-08-13 18:11:56 UTC
Created attachment 25006 [details]
A patch

GMP code may be buggy.  But it works with all other compilers,
including GCC 4.6.0 and older. Is there any particular good
reason to ignore range overflow?  This patch checks range
overflow and caused no regressions on Linux/x86.
Comment 12 Andrew Pinski 2011-08-13 18:17:24 UTC
(In reply to comment #11)
> Created attachment 25006 [details]
> A patch
> 
> GMP code may be buggy.  But it works with all other compilers,
> including GCC 4.6.0 and older. Is there any particular good
> reason to ignore range overflow?  This patch checks range
> overflow and caused no regressions on Linux/x86.

No Again we decided long ago to have overflow declared as being undefined and ignoring range overflow is not what we decided.  It might work with other compilers does not mean it is valid and well defined code.
Comment 13 H.J. Lu 2011-08-13 18:23:14 UTC
We should consider our users. GMP has been working with
GCC for a long time. Now it fails with GCC 4.7. It is
a very bad GCC 4.7 experience for user.
Comment 14 H.J. Lu 2011-08-13 18:32:53 UTC
What possible optimization do we gain by not checking
range overflow? Does anyone have a testcase to show it?
Comment 15 Andrew Pinski 2011-08-13 19:00:48 UTC
(In reply to comment #13)
> We should consider our users. GMP has been working with
> GCC for a long time. Now it fails with GCC 4.7. It is
> a very bad GCC 4.7 experience for user.

Yes and other packages failed when the original VPR and overflow was introduced.  Why should this case be any different than all the others?
Comment 16 Richard Biener 2011-08-14 09:49:01 UTC
Invalid.
Comment 17 H.J. Lu 2011-11-30 20:52:59 UTC
(In reply to comment #6)
> On Sat, 13 Aug 2011, hjl.tools at gmail dot com wrote:
> 
> > --- Comment #4 from H.J. Lu <hjl.tools at gmail dot com> 2011-08-13 12:09:16 UTC ---
> > This code comes from mpz/set_si.c in gmp:
> > 
> > void
> > mpz_set_si (mpz_ptr dest, signed long int val)
> > {
> >   mp_size_t size;
> >   mp_limb_t vl;
> > 
> >   vl = (mp_limb_t) (unsigned long int) (val >= 0 ? val : -val);
> 
> So that GMP code is buggy.  Change it to
> 
> vl = (mp_limb_t) (val >= 0 ? (unsigned long int) val : -(unsigned long int)
> val);
> 
> and it will be valid.

It doesn't work:

[hjl@gnu-6 tmp]$ cat x.c
#include <stdio.h>
#include <limits.h>

unsigned long long
__attribute__ ((noinline))
foo (signed int v_digit)
{
  unsigned long long x;

  x = (unsigned long long) (v_digit >= 0 ? (unsigned int) v_digit : (unsigned int) -v_digit);
  return x;
}

int
main ()
{
  unsigned long long x;

  x = foo (INT_MIN);
  printf ("%lld\n", x);
  x = foo (INT_MAX);
  printf ("%lld\n", x);
  return 0;
}
[hjl@gnu-6 tmp]$ /export/build/gnu/gcc-x32/release/usr/gcc-4.7.0-x32/bin/gcc -O2 x.c
[hjl@gnu-6 tmp]$ ./a.out 
-2147483648
2147483647
[hjl@gnu-6 tmp]$ /export/build/gnu/gcc-x32/release/usr/gcc-4.7.0-x32/bin/gcc -O x.c
[hjl@gnu-6 tmp]$ ./a.out 
2147483648
2147483647
[hjl@gnu-6 tmp]$
Comment 18 Andrew Pinski 2011-11-30 21:32:21 UTC
> (unsigned int) -v_digit

That is wrong it should be:
-(unsigned int) v_digit