[Bug c/48197] New: possible wrong code bug at -O0

[Bug c/48197] New: possible wrong code bug at -O0

[regehr at cs dot utah.edu]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48197

           Summary: possible wrong code bug at -O0
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: regehr@cs.utah.edu
                CC: chenyang@cs.utah.edu
              Host: x86_64-unknown-linux-gnu
            Target: x86_64-unknown-linux-gnu
             Build: x86_64-unknown-linux-gnu


Here gcc's output is the same at all optimization levels.  We think the correct
answer for this function is "x = 0" because all of the following statements are
equivalent:

   x = (long)0 > ((unsigned int)0 ^ (signed short)y);
   x = (long)0 > ((unsigned int)0 ^ (signed short)((int)0x8000));
   x = (long)0 > ((unsigned int)0 ^ (signed short)0x8000);
   x = (long)0 > ((unsigned)0 ^ (unsigned)0x8000);
   x = (long)0 > ((unsigned)0 ^ (unsigned)32768);
   x = (long)0 > (unsigned)32768;
   x = (long)0 > (long)32768;
   x = 0;

[regehr@gamow ~]$ current-gcc -O0 small.c -o small
[regehr@gamow ~]$ ./small
x = 1
[regehr@gamow ~]$ cat small.c


static int x = 0;
static int y = 0x8000;

int printf(const char *format, ...);

int main (void)
{
   x = (long)0 > ((unsigned int)0 ^ (signed short)y);
   printf("x = %d\n", x);
   return 0;
} 
[regehr@gamow ~]$ current-gcc -v

Using built-in specs.
COLLECT_GCC=current-gcc
COLLECT_LTO_WRAPPER=/uusoc/exports/scratch/regehr/z/compiler-install/gcc-r171139-install/bin/../libexec/gcc/x86_64-unknown-linux-gnu/4.7.0/lto-wrapper
Target: x86_64-unknown-linux-gnu
Configured with: ../configure --with-libelf=/usr/local --enable-lto
--prefix=/home/regehr/z/compiler-install/gcc-r171139-install
--program-prefix=r171139- --enable-languages=c,c++
Thread model: posix
gcc version 4.7.0 20110318 (experimental) (GCC)

[Bug c/48197] possible wrong code bug at -O0

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48197

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> 2011-03-18 19:56:42 UTC ---
   x = (long)0 > ((unsigned int)0 ^ (signed short)0x8000);
   x = (long)0 > ((unsigned)0 ^ (unsigned)0x8000);
I think you missed something here (unsigned)(signed short) still sign extends.

[Bug c/48197] possible wrong code bug at -O0

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48197

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-03-18 20:09:18 UTC ---
That's true, the step from the 3rd to 4th line is wrong.
But that doesn't mean that on LP64 targets it should print 1.

On:

extern void abort (void);
static int y = 0x8000;

int
main ()
{
  if (0LL > (0U ^ (short)0x8000))
    abort ();
  if (0LL > (0U ^ (short)y))
    abort ();
  return 0;
}

the first test doesn't abort, the second one does.

[Bug c/48197] possible wrong code bug at -O0

[regehr at cs dot utah.edu]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48197

--- Comment #2 from John Regehr <regehr at cs dot utah.edu> 2011-03-18 20:04:04 UTC ---
(In reply to comment #1)
>    x = (long)0 > ((unsigned int)0 ^ (signed short)0x8000);
>    x = (long)0 > ((unsigned)0 ^ (unsigned)0x8000);
> I think you missed something here (unsigned)(signed short) still sign extends.

Thanks Andrew, I'll look more closely.

If GCC is right, then Clang and Intel CC are wrong (assuming all three
compilers make the same implementation-dependent decisions for integers on
x86-64, which I was under the impression they did).

[Bug c/48197] possible wrong code bug at -O0

[regehr at cs dot utah.edu]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48197

--- Comment #4 from John Regehr <regehr at cs dot utah.edu> 2011-03-18 20:12:31 UTC ---
Thanks Jakub, I was just about to send the same example!

[Bug c/48197] possible wrong code bug at -O0

[regehr at cs dot utah.edu]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48197

--- Comment #5 from John Regehr <regehr at cs dot utah.edu> 2011-03-18 20:14:51 UTC ---
Here's a test case:

int printf(const char *format, ...);

int main (void)
{
  int y = 0x8000;
  int x1 = (long)0 > ((unsigned int)0 ^ (signed short)y);
  int x2 = (long)0 > ((unsigned int)0 ^ (signed short)((int)0x8000));
  int x3 = (long)0 > ((unsigned int)0 ^ (signed short)0x8000);
  int x4 = (long)0 > ((unsigned)0 ^ (unsigned)y);
  int x5 = (long)0 > ((unsigned)0 ^ (unsigned)32768);
  int x6 = (long)0 > (unsigned)32768;
  int x7 = (long)0 > (long)32768;
  int x8 = 0;

  printf("%d %d %d %d %d %d %d %d\n",
         x1, x2, x3, x4, x5, x6, x7, x8);

  return 0;
}

And here's the output:

[regehr@gamow ~]$ current-gcc -O0 small.c -o small -Wall
[regehr@gamow ~]$ ./small 
1 0 0 0 0 0 0 0

I think 0 should be assigned into all of x1..x8.

[Bug c/48197] possible wrong code bug at -O0

[regehr at cs dot utah.edu]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48197

--- Comment #6 from John Regehr <regehr at cs dot utah.edu> 2011-03-18 20:23:32 UTC ---
Bleh... nevermind the longer test, it carries along my misunderstanding of the
sign extension.  Anyway, thanks!

[Bug c/48197] possible wrong code bug at -O0

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48197

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-03-18 20:54:07 UTC ---
I think the bug is in shorten_compare.
We are called for GT_EXPR with op0 0LL and op1 (unsigned) (short) ((short) 0 ^
(short) y)
with long long result type. ^ above is BIT_XOR_EXPR with short type (shortened
earlier, but that's fine, (unsigned) (short) ((short) 0 ^ (short y)) is still
(unsigned) (short) y, i.e. sign extending y from 16 bits to 32 bits.  In the
above the (short) cast doesn't really exist, it is simply a NOP_EXPR with
unsigned int type of BIT_XOR_EXPR with short int type.
get_narrower gives us 0LL (no change, unsignedp0 set to 0) and the short int
BIT_XOR_EXPR (again, with unsignedp1 set to 0).  That is IMHO also fine,
unsignedp1 says that the BIT_XOR_EXPR needs to be sign extended to the type.

But in the end shorten_compare says that the comparison just should be done in
short int, which is wrong.

[Bug c/48197] possible wrong code bug at -O0

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48197

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2011.03.19 11:41:51
         AssignedTo|unassigned at gcc dot       |jakub at gcc dot gnu.org
                   |gnu.org                     |
     Ever Confirmed|0                           |1

[Bug c/48197] possible wrong code bug at -O0

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48197

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
   Last reconfirmed|2011-03-19 11:41:51         |

[Bug c/48197] possible wrong code bug at -O0

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48197

--- Comment #8 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-03-19 12:34:55 UTC ---
(In reply to comment #7)
> I think the bug is in shorten_compare.
> We are called for GT_EXPR with op0 0LL and op1 (unsigned) (short) ((short) 0 ^
> (short) y)
> with long long result type. ^ above is BIT_XOR_EXPR with short type (shortened
> earlier, but that's fine, (unsigned) (short) ((short) 0 ^ (short y)) is still
> (unsigned) (short) y, i.e. sign extending y from 16 bits to 32 bits.  In the
> above the (short) cast doesn't really exist, it is simply a NOP_EXPR with
> unsigned int type of BIT_XOR_EXPR with short int type.
> get_narrower gives us 0LL (no change, unsignedp0 set to 0) and the short int
> BIT_XOR_EXPR (again, with unsignedp1 set to 0).  That is IMHO also fine,
> unsignedp1 says that the BIT_XOR_EXPR needs to be sign extended to the type.
> 
> But in the end shorten_compare says that the comparison just should be done in
> short int, which is wrong.

Ah, one of my special friends and endless source of wrong-code bugs ;)

Can we just remove this premature frontend optimization?

[Bug c/48197] possible wrong code bug at -O0

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48197

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-03-19 15:06:14 UTC ---
Created attachment 23723
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=23723
gcc47-pr48197.patch

Maybe, but it might not be very easy, as it is related to diagnostics too (both
this routine issues warnings and I believe some warnings like -Wconversion rely
on the shortening too).

In the mean time, here is an untested fix.

[Bug c/48197] possible wrong code bug at -O0

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48197

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-03-19 15:17:06 UTC ---
BTW, this failed already in gcc 2.7.2.3, so doesn't seem to be a regression.
I think we want to fix it in 4.6 nevertheless, but only after 4.6.0 is
released.

[Bug c/48197] possible wrong code bug at -O0

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48197

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-03-21 17:57:41 UTC ---
Author: jakub
Date: Mon Mar 21 17:57:34 2011
New Revision: 171252

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=171252
Log:
    PR c/42544
    PR c/48197
    * c-common.c (shorten_compare): If primopN is first sign-extended
    to opN and then zero-extended to result type, set primopN to opN.

    * gcc.c-torture/execute/pr42544.c: New test.
    * gcc.c-torture/execute/pr48197.c: New test.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/pr42544.c
    trunk/gcc/testsuite/gcc.c-torture/execute/pr48197.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/c-family/c-common.c
    trunk/gcc/testsuite/ChangeLog

[Bug c/48197] possible wrong code bug at -O0

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48197

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-03-26 09:23:03 UTC ---
Author: jakub
Date: Sat Mar 26 09:23:01 2011
New Revision: 171548

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=171548
Log:
    Backport from mainline
    2011-03-20  Jakub Jelinek  <jakub@redhat.com>

    PR c/42544
    PR c/48197
    * c-common.c (shorten_compare): If primopN is first sign-extended
    to opN and then zero-extended to result type, set primopN to opN.

    * gcc.c-torture/execute/pr42544.c: New test.
    * gcc.c-torture/execute/pr48197.c: New test.

Added:
    branches/gcc-4_6-branch/gcc/testsuite/gcc.c-torture/execute/pr42544.c
    branches/gcc-4_6-branch/gcc/testsuite/gcc.c-torture/execute/pr48197.c
Modified:
    branches/gcc-4_6-branch/gcc/ChangeLog
    branches/gcc-4_6-branch/gcc/c-family/c-common.c
    branches/gcc-4_6-branch/gcc/testsuite/ChangeLog

[Bug c/48197] possible wrong code bug at -O0

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48197

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-05-05 10:27:16 UTC ---
Fixed for 4.6+.