Bug 44828 - [4.3/4.4 Regression] possible integer wrong code bug
Summary: [4.3/4.4 Regression] possible integer wrong code bug
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c (show other bugs)
Version: 4.6.0
: P3 normal
Target Milestone: 4.5.1
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-05 22:52 UTC by John Regehr
Modified: 2011-04-26 07:40 UTC (History)
5 users (show)

See Also:
Host: i686-pc-linux-gnu
Target: i686-pc-linux-gnu
Build: i686-pc-linux-gnu
Known to work: 4.2.4, 4.5.1, 4.6.0
Known to fail: 4.3.4, 4.4.3, 4.5.0
Last reconfirmed: 2010-07-06 10:35:15


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description John Regehr 2010-07-05 22:52:09 UTC
regehr@john-home:~$ current-gcc -v
Using built-in specs.
COLLECT_GCC=current-gcc
COLLECT_LTO_WRAPPER=/home/regehr/z/compiler-install/gcc-r161813-install/libexec/gcc/i686-pc-linux-gnu/4.6.0/lto-wrapper
Target: i686-pc-linux-gnu
Configured with: ../configure --with-libelf=/usr/local --enable-lto --prefix=/home/regehr/z/compiler-install/gcc-r161813-install --program-prefix=r161813- --enable-languages=c,c++
Thread model: posix
gcc version 4.6.0 20100704 (experimental) (GCC) 

regehr@john-home:~$ current-gcc -O1 small.c -o small
regehr@john-home:~$ ./small
checksum g_40 = 274686410
regehr@john-home:~$ current-gcc -O2 small.c -o small
regehr@john-home:~$ ./small
checksum g_40 = -1

regehr@john-home:~$ cat small.c

extern int printf (__const char *__restrict __format, ...);

static char
foo (char si1, char si2)
{
  return si1* si2;
}

 const  volatile unsigned int g_2[8][3] = {{0L, 0L, 0L}, {0L, 0L, 0L}, {0L,
0L, 0L}, {0L, 0L, 0L}, {0L, 0L, 0L}, {0L, 0L, 0L}, {0L, 0L, 0L}, {0L, 0L,
0L}};
long long g_29 = 1;
int g_40 = 0x105F61CAL;
int *g_39 = &g_40;
 volatile int * volatile g_88[1] = {0};
 volatile int g_429[5] = {1L, 1L, 1L, 1L, 1L};

int  main(void)
{
    int * const l_353 = &g_40;
    int l_414 = 0xF5B296C2L;
    if (!(g_2[5][2]))
    {
        int l_420 = 0x0332F5C8L;
        if (((foo (l_420, (*l_353))) > (!-10L)))
        {
            for (l_414 = 0; l_414 < 1; l_414 += 1)
            {
                g_88[l_414] = &g_429[2];
            }
            (*g_39) = -1;
        }
    }
    printf("checksum g_40 = %d\n", g_40);
    return g_29;
}
Comment 1 Jakub Jelinek 2010-07-06 09:25:37 UTC
Shorter testcase:

extern void abort (void);

static char
foo (char si1, char si2)
{
  return si1 * si2;
}

int a = 0x105F61CA;

int
main (void)
{
  int b = 0x0332F5C8;
  if (foo (b, a) > 0)
    abort ();
  return 0;
}

Works with -O2 -fwrapv.
Comment 2 Jakub Jelinek 2010-07-06 09:33:14 UTC
Not sure whether the testcase is valid or not.  The multiplication using char variables on both sides (and likewise for result) is: -54 * -56 (= 3024),
but (char) 3024 is -48.  For int that would be clear undefined behavior, but for char the multiplication is promoted to int, so it is (char) (int * int).
*.ccp1 has:
  char D.2741;
  char D.2741;
  char si2;
  int D.2727;
  char D.2726;
  int a.0;

<bb 2>:
  a.0_2 = a;
  D.2726_3 = (char) a.0_2;
  D.2727_4 = (int) D.2726_3;
  si2_11 = (char) D.2727_4;
  D.2741_12 = si2_11 * -56;
  D.2741_13 = D.2741_12;
  D.2741_7 = D.2741_13;
  if (D.2741_7 > 0)

but forwprop1 uses if (si2_11 < 0) test instead.  If the testcase is valid, we shouldn't assume undefined overflow for char and short operations.
Comment 3 Richard Biener 2010-07-06 10:35:15 UTC
The bug is that we have in .original

{
  return si1 * si2;
}

while it should have been

{
  return (char)((unsigned char) si1 * (unsigned char) si2);
}

which is premature optimization by convert_to_integer, short-cutting
the correctly implemented fold of (signed char)((int) si1 * (int) si2)
to the above.

Index: gcc/convert.c
===================================================================
--- gcc/convert.c       (revision 161840)
+++ gcc/convert.c       (working copy)
@@ -774,7 +774,8 @@ convert_to_integer (tree type, tree expr
                        || ((!TYPE_OVERFLOW_WRAPS (TREE_TYPE (arg0))
                             || !TYPE_OVERFLOW_WRAPS (TREE_TYPE (arg1)))
                            && (ex_form == PLUS_EXPR
-                               || ex_form == MINUS_EXPR)))
+                               || ex_form == MINUS_EXPR
+                               || ex_form == MULT_EXPR)))
                      typex = unsigned_type_for (typex);
                    else
                      typex = signed_type_for (typex);

fixes this.
Comment 4 Richard Biener 2010-07-06 13:38:17 UTC
Subject: Bug 44828

Author: rguenth
Date: Tue Jul  6 13:37:58 2010
New Revision: 161869

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=161869
Log:
2010-07-06  Richard Guenther  <rguenther@suse.de>

	PR middle-end/44828
	* convert.c (convert_to_integer): Watch out for overflowing
	MULT_EXPR as well.

	* gcc.c-torture/execute/pr44828.c: New testcase.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/pr44828.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/convert.c
    trunk/gcc/testsuite/ChangeLog

Comment 5 Richard Biener 2010-07-06 13:40:50 UTC
Fixed on trunk.  The problem is latent everywhere but the optimization
doesn't happen for 4.2.4 or earlier.

Which makes it a regression.
Comment 6 John Regehr 2010-07-06 14:10:51 UTC
(In reply to comment #2)
> Not sure whether the testcase is valid or not.  The multiplication using char
> variables on both sides (and likewise for result) is: -54 * -56 (= 3024),
> but (char) 3024 is -48.  For int that would be clear undefined behavior, but
> for char the multiplication is promoted to int, so it is (char) (int * int).

My students and I had to argue about this and read the standard before submitting this bug report.  But I'm almost certain the testcase is valid.
Comment 7 Peter Bergner 2010-07-07 22:51:59 UTC
Jakub's test case still fails on powerpc*-linux because we default to unsigned char.  I think the obvious fix is to just pass -fsigned-char in dg-options.
Comment 8 Peter Bergner 2010-07-08 04:12:22 UTC
Subject: Bug 44828

Author: bergner
Date: Thu Jul  8 04:12:04 2010
New Revision: 161942

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=161942
Log:
	PR middle-end/44828
	* gcc.c-torture/execute/pr44828.x: New file.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/pr44828.x
Modified:
    trunk/gcc/testsuite/ChangeLog

Comment 9 Peter Bergner 2010-07-08 04:19:17 UTC
The fix in Comment #8 fixes the test case on systems that have a default of unsigned char (eg, powerpc*-linux).
Comment 10 Jakub Jelinek 2010-07-08 07:47:22 UTC
I'd say the test instead just should use signed char instead of char.
Comment 11 Richard Biener 2010-07-08 11:56:42 UTC
Subject: Bug 44828

Author: rguenth
Date: Thu Jul  8 11:56:08 2010
New Revision: 161951

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=161951
Log:
2010-07-08  Richard Guenther  <rguenther@suse.de>

	Backport from mainline
	2010-05-27  Richard Guenther  <rguenther@suse.de>

	PR tree-optimization/44284
	* tree-vect-stmts.c (vectorizable_assignment): Handle
	sign-changing conversions as simple copy.

	* gcc.dg/vect/vect-118.c: New testcase.
	* gcc.dg/vect/bb-slp-20.c: Adjust.
	* gcc.dg/vect/no-section-anchors-vect-36.c: Likewise.
	* gcc.dg/vect/slp-9.c: Likewise.
	* gcc.dg/vect/slp-reduc-4.c: Likewise.
	* gcc.dg/vect/vect-10.c: Likewise.
	* gcc.dg/vect/vect-109.c: Likewise.
	* gcc.dg/vect/vect-12.c: Likewise.
	* gcc.dg/vect/vect-36.c: Likewise.
	* gcc.dg/vect/vect-7.c: Likewise.
	* gcc.dg/vect/vect-iv-8.c: Likewise.
	* gcc.dg/vect/vect-multitypes-10.c: Likewise.
	* gcc.dg/vect/vect-multitypes-13.c: Likewise.
	* gcc.dg/vect/vect-multitypes-14.c: Likewise.
	* gcc.dg/vect/vect-multitypes-15.c: Likewise.
	* gcc.dg/vect/vect-multitypes-7.c: Likewise.
	* gcc.dg/vect/vect-multitypes-8.c: Likewise.
	* gcc.dg/vect/vect-multitypes-9.c: Likewise.
	* gcc.dg/vect/vect-reduc-dot-s16b.c: Likewise.
	* gcc.dg/vect/vect-reduc-dot-s8a.c: Likewise.
	* gcc.dg/vect/vect-reduc-dot-s8b.c: Likewise.
	* gcc.dg/vect/vect-reduc-dot-u16b.c: Likewise.
	* gcc.dg/vect/vect-strided-a-u32-mult.c: Likewise.
	* gcc.dg/vect/vect-strided-u32-mult.c: Likewise.
	* gcc.dg/vect/vect-widen-mult-s16.c: Likewise.
	* gcc.dg/vect/vect-widen-mult-s8.c: Likewise.
	* gcc.dg/vect/vect-widen-mult-sum.c: Likewise.
	* gcc.dg/vect/vect-widen-mult-u16.c: Likewise.

	2010-07-06  Richard Guenther  <rguenther@suse.de>

	PR middle-end/44828
	* convert.c (convert_to_integer): Watch out for overflowing
	MULT_EXPR as well.

	* gcc.c-torture/execute/pr44828.c: New testcase.

Added:
    branches/gcc-4_5-branch/gcc/testsuite/gcc.c-torture/execute/pr44828.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-118.c
Modified:
    branches/gcc-4_5-branch/gcc/ChangeLog
    branches/gcc-4_5-branch/gcc/convert.c
    branches/gcc-4_5-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/bb-slp-20.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/no-section-anchors-vect-36.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/slp-9.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-10.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-109.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-12.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-36.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-7.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-iv-8.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-multitypes-10.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-multitypes-13.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-multitypes-14.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-multitypes-15.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-multitypes-7.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-multitypes-8.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-multitypes-9.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-reduc-dot-s16a.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-reduc-dot-s16b.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-reduc-dot-s8a.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-reduc-dot-s8b.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-reduc-dot-s8c.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-reduc-dot-u16b.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-strided-a-u32-mult.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-strided-u32-mult.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-widen-mult-s16.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-widen-mult-s8.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-widen-mult-sum.c
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/vect-widen-mult-u16.c
    branches/gcc-4_5-branch/gcc/tree-vect-stmts.c

Comment 12 Richard Biener 2010-07-08 11:57:16 UTC
Fixed for 4.5.1 as well.  Backporting to older branches will regress vectorization
if the fix for PR44284 isn't backported as well which IMHO isn't appropriate.
Comment 13 Peter Bergner 2010-07-08 14:18:07 UTC
Subject: Bug 44828

Author: bergner
Date: Thu Jul  8 14:17:52 2010
New Revision: 161956

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=161956
Log:
	PR middle-end/44828
	* gcc.c-torture/execute/pr44828.c (foo): Use signed char.
	* gcc.c-torture/execute/pr44828.x: Revert.

Removed:
    trunk/gcc/testsuite/gcc.c-torture/execute/pr44828.x
Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.c-torture/execute/pr44828.c

Comment 14 Jakub Jelinek 2011-04-26 07:40:15 UTC
Fixed for 4.5.1+.