Bug 37103 - [4.3/4.4 Regression] possible integer codegen bug
Summary: [4.3/4.4 Regression] possible integer codegen bug
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.4.0
: P3 normal
Target Milestone: 4.3.2
Assignee: Jakub Jelinek
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2008-08-13 05:10 UTC by John Regehr
Modified: 2008-08-14 09:20 UTC (History)
2 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
Known to fail: 4.3.1 4.4.0
Last reconfirmed: 2008-08-13 18:30:25


Attachments
potential test program for gcc testsuite (43.86 KB, text/x-csrc)
2008-08-13 22:52 UTC, John Regehr
Details

Note You need to log in before you can comment on or make changes to this bug.
Description John Regehr 2008-08-13 05:10:08 UTC
This is an interesting one...

Compiling the code below at all common optimization levels, gcc r139046 generates code that prints hello, as does 4.3.1.  Pre-4.3 versions of gcc emit code that prints nothing.  I'm pretty sure that the older versions are correct.

#include <stdio.h>

int func_72 (void);
int func_72 (void)
{
  printf ("hello\n");
  return 0;
}

void func_58 (unsigned short p_65);
void func_58 (unsigned short p_65)
{
  char g_99 = -1;
  (p_65 != g_99) || (func_72 ());
}

int main (void)
{
  func_58 (-1);
  return 0;
}
Comment 1 pinskia@gmail.com 2008-08-13 05:44:48 UTC
Subject: Re:   New: possible integer codegen bug

Note for most targets not printing is correct as char is signed by  
default but for most powerpc targets the opposite is true. You should  
have explicted included signed for g_99.

Sent from my iPhone

On Aug 12, 2008, at 22:10, "regehr at cs dot utah dot edu" <gcc-bugzilla@gcc.gnu.org 
 > wrote:

> This is an interesting one...
>
> Compiling the code below at all common optimization levels, gcc  
> r139046
> generates code that prints hello, as does 4.3.1.  Pre-4.3 versions  
> of gcc emit
> code that prints nothing.  I'm pretty sure that the older versions  
> are correct.
>
> #include <stdio.h>
>
> int func_72 (void);
> int func_72 (void)
> {
>  printf ("hello\n");
>  return 0;
> }
>
> void func_58 (unsigned short p_65);
> void func_58 (unsigned short p_65)
> {
>  char g_99 = -1;
>  (p_65 != g_99) || (func_72 ());
> }
>
> int main (void)
> {
>  func_58 (-1);
>  return 0;
> }
>
>
> -- 
>           Summary: possible integer codegen bug
>           Product: gcc
>           Version: 4.4.0
>            Status: UNCONFIRMED
>          Severity: normal
>          Priority: P3
>         Component: c
>        AssignedTo: unassigned at gcc dot gnu dot org
>        ReportedBy: regehr at cs dot utah dot edu
> GCC build triplet: i686-pc-linux-gnu
>  GCC host triplet: i686-pc-linux-gnu
> GCC target triplet: i686-pc-linux-gnu
>
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=37103
>
Comment 2 Richard Biener 2008-08-13 09:16:14 UTC
The C standard specifies that the comparison is done in type int which means
that for both signed and unsigned char

  (char)-1 != (unsigned short)-1

should evaluate to false.  Testcase:

extern void abort (void);

void func_1 (unsigned short p_65)
{
  signed char g_99 = -1;
  if (p_65 == g_99)
    abort ();
}

void func_2 (unsigned short p_65)
{
  unsigned char g_99 = -1;
  if (p_65 == g_99)
    abort ();
}

int main (void)
{
  func_1 (-1);
  func_2 (-1);
  return 0;
}

This looks like yet another folding bug as we end up with the comparison
narrowed.
Comment 3 Jakub Jelinek 2008-08-13 13:37:16 UTC
I bet 132269 or 134108 are the cause, will test later.
Comment 4 Jakub Jelinek 2008-08-13 18:30:24 UTC
Verified the regression was introduced by PR35163 fix.
http://gcc.gnu.org/ml/gcc-patches/2008-04/msg00720.html
should have been IMNSHO applied for shorter_type bigger or equal than arg1_unw's type, not just when they are equal.  If shorter_type is bigger and the signs are different, then the bits above shorter_type bits might be all 1's in one case and all 0's in the second case.

Patch I'll be regtesting:

2008-08-13  Jakub Jelinek  <jakub@redhat.com>

PR middle-end/37103
* fold-const.c (fold_widened_comparison): Do not allow
sign changes that change the result even if shorter type
is wider than arg1_unw's type.

* gcc.c-torture/execute/20080813-1.c: New test.

--- gcc/fold-const.c.jj2008-08-13 19:46:11.000000000 +0200
+++ gcc/fold-const.c2008-08-13 20:18:21.000000000 +0200
@@ -6733,10 +6733,8 @@ fold_widened_comparison (enum tree_code 
   if ((code == EQ_EXPR || code == NE_EXPR
        || TYPE_UNSIGNED (TREE_TYPE (arg0)) == TYPE_UNSIGNED (shorter_type))
       && (TREE_TYPE (arg1_unw) == shorter_type
-  || (TYPE_PRECISION (shorter_type)
-      > TYPE_PRECISION (TREE_TYPE (arg1_unw)))
   || ((TYPE_PRECISION (shorter_type)
-       == TYPE_PRECISION (TREE_TYPE (arg1_unw)))
+       >= TYPE_PRECISION (TREE_TYPE (arg1_unw)))
       && (TYPE_UNSIGNED (shorter_type)
   == TYPE_UNSIGNED (TREE_TYPE (arg1_unw))))
   || (TREE_CODE (arg1_unw) == INTEGER_CST
--- gcc/testsuite/gcc.c-torture/execute/20080813-1.c.jj2008-08-13 20:22:56.000000000 +0200
+++ gcc/testsuite/gcc.c-torture/execute/20080813-1.c2008-08-13 20:22:10.000000000 +0200
@@ -0,0 +1,30 @@
+/* PR middle-end/37103 */
+
+extern void abort (void);
+
+void
+foo (unsigned short x)
+{
+  signed char y = -1;
+  if (x == y)
+    abort ();
+}
+
+void
+bar (unsigned short x)
+{
+  unsigned char y = -1;
+  if (x == y)
+    abort ();
+}
+
+int
+main (void)
+{
+  if (sizeof (int) == sizeof (short))
+    return 0;
+  foo (-1);
+  if (sizeof (short) > 1)
+    bar (-1);
+  return 0;
+}
Comment 5 rguenther@suse.de 2008-08-13 21:05:16 UTC
Subject: Re:  [4.3/4.4 Regression] possible integer
 codegen bug

On Wed, 13 Aug 2008, jakub at gcc dot gnu dot org wrote:

> ------- Comment #4 from jakub at gcc dot gnu dot org  2008-08-13 18:30 -------
> Verified the regression was introduced by PR35163 fix.
> http://gcc.gnu.org/ml/gcc-patches/2008-04/msg00720.html
> should have been IMNSHO applied for shorter_type bigger or equal than
> arg1_unw's type, not just when they are equal.  If shorter_type is bigger and
> the signs are different, then the bits above shorter_type bits might be all 1's
> in one case and all 0's in the second case.

Yeah, you are right.  Patch is pre-approved.

Richard.

> Patch I'll be regtesting:
> 
> 2008-08-13  Jakub Jelinek  <jakub@redhat.com>
> 
> PR middle-end/37103
> * fold-const.c (fold_widened_comparison): Do not allow
> sign changes that change the result even if shorter type
> is wider than arg1_unw's type.
> 
> * gcc.c-torture/execute/20080813-1.c: New test.
> 
> --- gcc/fold-const.c.jj2008-08-13 19:46:11.000000000 +0200
> +++ gcc/fold-const.c2008-08-13 20:18:21.000000000 +0200
> @@ -6733,10 +6733,8 @@ fold_widened_comparison (enum tree_code 
>    if ((code == EQ_EXPR || code == NE_EXPR
>         || TYPE_UNSIGNED (TREE_TYPE (arg0)) == TYPE_UNSIGNED (shorter_type))
>        && (TREE_TYPE (arg1_unw) == shorter_type
> -  || (TYPE_PRECISION (shorter_type)
> -      > TYPE_PRECISION (TREE_TYPE (arg1_unw)))
>    || ((TYPE_PRECISION (shorter_type)
> -       == TYPE_PRECISION (TREE_TYPE (arg1_unw)))
> +       >= TYPE_PRECISION (TREE_TYPE (arg1_unw)))
>        && (TYPE_UNSIGNED (shorter_type)
>    == TYPE_UNSIGNED (TREE_TYPE (arg1_unw))))
>    || (TREE_CODE (arg1_unw) == INTEGER_CST
> --- gcc/testsuite/gcc.c-torture/execute/20080813-1.c.jj2008-08-13
> 20:22:56.000000000 +0200
> +++ gcc/testsuite/gcc.c-torture/execute/20080813-1.c2008-08-13
> 20:22:10.000000000 +0200
> @@ -0,0 +1,30 @@
> +/* PR middle-end/37103 */
> +
> +extern void abort (void);
> +
> +void
> +foo (unsigned short x)
> +{
> +  signed char y = -1;
> +  if (x == y)
> +    abort ();
> +}
> +
> +void
> +bar (unsigned short x)
> +{
> +  unsigned char y = -1;
> +  if (x == y)
> +    abort ();
> +}
> +
> +int
> +main (void)
> +{
> +  if (sizeof (int) == sizeof (short))
> +    return 0;
> +  foo (-1);
> +  if (sizeof (short) > 1)
> +    bar (-1);
> +  return 0;
> +}
> 
> 
> 

Comment 6 John Regehr 2008-08-13 22:52:08 UTC
Created attachment 16067 [details]
potential test program for gcc testsuite

I wrote a little script to make a C program that exhausts the possible integer comparisons for -1, 0, 1.  Interested in adding this to the test suite?
Comment 7 John Regehr 2008-08-13 22:57:12 UTC
(In reply to comment #6)
> Created an attachment (id=16067) [edit]
> potential test program for gcc testsuite
> 
> I wrote a little script to make a C program that exhausts the possible integer
> comparisons for -1, 0, 1.  Interested in adding this to the test suite?

The answers are only valid for ia32 of course... if a platform-independent version of this would be interesting, let me know.
Comment 8 Jakub Jelinek 2008-08-14 09:04:11 UTC
Subject: Bug 37103

Author: jakub
Date: Thu Aug 14 09:02:46 2008
New Revision: 139093

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=139093
Log:
	PR middle-end/37103
	* fold-const.c (fold_widened_comparison): Do not allow
	sign changes that change the result even if shorter type
	is wider than arg1_unw's type.

	* gcc.c-torture/execute/20080813-1.c: New test.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/20080813-1.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/fold-const.c
    trunk/gcc/testsuite/ChangeLog

Comment 9 Jakub Jelinek 2008-08-14 09:12:26 UTC
Subject: Bug 37103

Author: jakub
Date: Thu Aug 14 09:11:03 2008
New Revision: 139094

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=139094
Log:
	PR middle-end/37103
	* fold-const.c (fold_widened_comparison): Do not allow
	sign changes that change the result even if shorter type
	is wider than arg1_unw's type.

	* gcc.c-torture/execute/20080813-1.c: New test.

Added:
    branches/gcc-4_3-branch/gcc/testsuite/gcc.c-torture/execute/20080813-1.c
Modified:
    branches/gcc-4_3-branch/gcc/ChangeLog
    branches/gcc-4_3-branch/gcc/fold-const.c
    branches/gcc-4_3-branch/gcc/testsuite/ChangeLog

Comment 10 Jakub Jelinek 2008-08-14 09:20:58 UTC
Fixed in CVS.
Regarding your testcase, I think it is too big, but its size could be very well decreased just by using preprocessor extensively.  That said, I'm not sure if some embedded targets won't be upset about 5401 functions.