Bug 15069 - [3.4 regression] a bit test on a variable of enum type is miscompiled
Summary: [3.4 regression] a bit test on a variable of enum type is miscompiled
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 3.4.0
: P3 critical
Target Milestone: 3.4.1
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
: 15277 (view as bug list)
Depends on:
Blocks:
 
Reported: 2004-04-22 12:52 UTC by bruno
Modified: 2004-10-30 21:10 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: 2.95.3 3.2.3 3.3.3 4.0.0
Known to fail: 3.4.0
Last reconfirmed: 2004-04-22 16:08:58


Attachments
Draft patch to provide C++-specific signed_type routine (1.24 KB, patch)
2004-05-29 00:04 UTC, Mark Mitchell
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description bruno 2004-04-22 12:52:16 UTC
When testing a bit of a variable with enum type, the test for the
highest bit (bit 2 in my case) is compiled to a bit test for bit 31.
But bit 31 is always zero for all possible values of the enum type.

Environment:
System: Linux honolulu.ilog.fr 2.4.21-0.13mdk #1 Fri Mar 14 15:08:06 EST 2003 i686 unknown unknown GNU/Linux
Architecture: i686

	
host: i686-pc-linux-gnu
build: i686-pc-linux-gnu
target: i686-pc-linux-gnu
configured with: ../gcc-3.4.0/configure --prefix=/home/haible/gnu/arch/linuxgcc34 --enable-shared --enable-threads --enable-__cxa_atexit --enable-languages=c,c++,f77,java,objc --enable-nls

How-To-Repeat:

============================== bug.cc ===================================
extern "C" void abort (void);

void* allocate_stream (unsigned char strmflags)
{
  if (strmflags == 0)
    abort ();
  return 0;
}

typedef enum {
  DIRECTION_PROBE = 0,
  DIRECTION_INPUT = 1,
  DIRECTION_INPUT_IMMUTABLE = 3,
  DIRECTION_OUTPUT = 4,
  DIRECTION_IO = 5
} direction_t;

void* make_unbuffered_stream (direction_t direction, int* eltype)
{
  unsigned char flags =
    (((direction) & (1L<<(0))) ? ((1L<<(4)) | (1L<<(6))) : 0)
    | (((direction) & (1L<<(2))) ? ((1L<<(5)) | (1L<<(7))) : 0)
    | (((direction) & (1L<<(1))) ? (1L<<(1)) : 0);
  if (*eltype == 0)
    flags &= ((1L<<(6)) | (1L<<(7))) | (1L<<(1));
  else
    flags &= ((1L<<(4)) | (1L<<(5))) | (1L<<(1));
  return allocate_stream (flags);
}

int main ()
{
  int x = 1;
  make_unbuffered_stream (DIRECTION_OUTPUT, &x);
  return 0;
}
=========================================================================

$ g++ bug.cc
$ ./a.out
Aborted

Here is the assembler code:
======================== g++ -O -S bug.cc ===============================
	.file	"bug.cc"
	.text
	.align 2
.globl _Z15allocate_streamh
	.type	_Z15allocate_streamh, @function
_Z15allocate_streamh:
.LFB2:
	pushl	%ebp
.LCFI0:
	movl	%esp, %ebp
.LCFI1:
	subl	$8, %esp
.LCFI2:
	cmpb	$0, 8(%ebp)
	jne	.L2
	call	abort
.L2:
	movl	$0, %eax
	leave
	ret
.LFE2:
	.size	_Z15allocate_streamh, .-_Z15allocate_streamh
	.align 2
.globl _Z22make_unbuffered_stream11direction_tPi
	.type	_Z22make_unbuffered_stream11direction_tPi, @function
_Z22make_unbuffered_stream11direction_tPi:
.LFB3:
	pushl	%ebp
.LCFI3:
	movl	%esp, %ebp
.LCFI4:
	subl	$8, %esp
.LCFI5:
	movl	8(%ebp), %edx
	movb	%dl, %al
	andb	$1, %al         	; test bit 0 - correct
	cmpb	$1, %al
	sbbl	%eax, %eax
	notl	%eax
	andl	$80, %eax
	testl	%edx, %edx		; test bit 31 instead of bit 2 - wrong
	jns	.L6
	orl	$160, %eax
.L6:
	testb	$2, %dl			; test bit 1 - correct
	je	.L7
	orl	$2, %eax
.L7:
	movb	%al, %dl
	movl	12(%ebp), %eax
	cmpl	$0, (%eax)
	jne	.L8
	andb	$-62, %dl
	jmp	.L9
.L8:
	andb	$50, %dl
.L9:
	movzbl	%dl, %eax
	movl	%eax, (%esp)
	call	_Z15allocate_streamh
	leave
	ret
.LFE3:
	.size	_Z22make_unbuffered_stream11direction_tPi, .-_Z22make_unbuffered_stream11direction_tPi
	.align 2
.globl main
	.type	main, @function
main:
.LFB4:
	pushl	%ebp
.LCFI6:
	movl	%esp, %ebp
.LCFI7:
	subl	$24, %esp
.LCFI8:
	andl	$-16, %esp
	subl	$16, %esp
	movl	$1, -4(%ebp)
	leal	-4(%ebp), %eax
	movl	%eax, 4(%esp)
	movl	$4, (%esp)
	call	_Z22make_unbuffered_stream11direction_tPi
	movl	$0, %eax
	leave
	ret
.LFE4:
	.size	main, .-main
	.section	.eh_frame,"a",@progbits
.Lframe1:
	.long	.LECIE1-.LSCIE1
.LSCIE1:
	.long	0x0
	.byte	0x1
	.string	"zP"
	.uleb128 0x1
	.sleb128 -4
	.byte	0x8
	.uleb128 0x5
	.byte	0x0
	.long	__gxx_personality_v0
	.byte	0xc
	.uleb128 0x4
	.uleb128 0x4
	.byte	0x88
	.uleb128 0x1
	.align 4
.LECIE1:
.LSFDE1:
	.long	.LEFDE1-.LASFDE1
.LASFDE1:
	.long	.LASFDE1-.Lframe1
	.long	.LFB2
	.long	.LFE2-.LFB2
	.uleb128 0x0
	.byte	0x4
	.long	.LCFI0-.LFB2
	.byte	0xe
	.uleb128 0x8
	.byte	0x85
	.uleb128 0x2
	.byte	0x4
	.long	.LCFI1-.LCFI0
	.byte	0xd
	.uleb128 0x5
	.align 4
.LEFDE1:
.LSFDE3:
	.long	.LEFDE3-.LASFDE3
.LASFDE3:
	.long	.LASFDE3-.Lframe1
	.long	.LFB3
	.long	.LFE3-.LFB3
	.uleb128 0x0
	.byte	0x4
	.long	.LCFI3-.LFB3
	.byte	0xe
	.uleb128 0x8
	.byte	0x85
	.uleb128 0x2
	.byte	0x4
	.long	.LCFI4-.LCFI3
	.byte	0xd
	.uleb128 0x5
	.align 4
.LEFDE3:
.LSFDE5:
	.long	.LEFDE5-.LASFDE5
.LASFDE5:
	.long	.LASFDE5-.Lframe1
	.long	.LFB4
	.long	.LFE4-.LFB4
	.uleb128 0x0
	.byte	0x4
	.long	.LCFI6-.LFB4
	.byte	0xe
	.uleb128 0x8
	.byte	0x85
	.uleb128 0x2
	.byte	0x4
	.long	.LCFI7-.LCFI6
	.byte	0xd
	.uleb128 0x5
	.align 4
.LEFDE5:
	.section	.note.GNU-stack,"",@progbits
	.ident	"GCC: (GNU) 3.4.0"
=========================================================================
Comment 1 bruno 2004-04-22 12:52:16 UTC
Fix:

My workaround is to use the C compiler instead of the C++ compiler.
Comment 2 Andrew Pinski 2004-04-22 14:44:46 UTC
IIRC the rules for enums in C++ is different than C.  Also sizes outside of the enums are undefined for 
C++ which makes this invalid and correct behavior, IIRC.
Comment 3 Wolfgang Bangerth 2004-04-22 15:53:50 UTC
To be precise, values outside the bit-range of the enum are undefined. 
I.e. in this case, values of type direction_t must be in the range 
0...7 including end points. 
Comment 4 Andrew Pinski 2004-04-22 15:58:20 UTC
Yes I am correct in that you are invoking undefined behavior as direction_t can only have values 
between 0 and 5 because C++ rules are different than C.  In C enum types have the same size of the 
underlining type but C++ is different in that respect.
Comment 5 bruno 2004-04-22 16:03:50 UTC
> IIRC the rules for enums in C++ is different than C.  
> Also sizes outside of the enums are undefined for C++ which makes this invalid and correct  
behavior, IIRC.  
  
Look at the code. Nowhere is a value outside the range 0..5 used. The argument that is being  
passed is not only one in the range bmin..bmax (speak of [dcl.enum] paragraph 6). It is also  
one of the values in the enumator-list.  
  
The expression being miscompiled is: (((direction) & (1L<<(2))). [expr.bit.and] paragraph 1  
says that enumeration operands are allowed for binary '&', and that the usual arithmetic  
conversions are performed. The value (long)(direction) is 4, as expected, as I can see by  
adding a printf statement. Therefore the value of (((direction) & (1L<<(2))) should be nonzero  
in the case where direction = DIRECTION_OUTPUT.  
  
Btw, why did you change this as reported against g++ 2.95? I reported it against g++ 3.4.0  
after verifying that it behaves differently than g++ 3.2.2. It is a regression.  
  
Comment 6 Wolfgang Bangerth 2004-04-22 16:08:57 UTC
This is clearly a serious bug. Here's something smaller: 
----------- 
extern "C" void abort (void); 
 
typedef enum { 
  FOUR = 4, 
  FIVE = 5 
} direction_t; 
 
int main () 
{ 
  direction_t four = FOUR; 
  int flags = (four & 4L) ? (32L | 128L) : 0; 
  flags &= 32L; 
 
  if (flags == 0) 
    abort (); 
} 
----------------- 
It is obvious that flag should be 32 at the end, but gcc3.4/3.5 aborts: 
 
g/x> /home/bangerth/bin/gcc-3.5-pre/bin/c++ x.cc -W -Wall ; ./a.out  
Aborted 
g/x> /home/bangerth/bin/gcc-3.4-pre/bin/c++ x.cc -W -Wall ; ./a.out  
Aborted 
g/x> /home/bangerth/bin/gcc-3.3.4-pre/bin/c++ x.cc -W -Wall ; ./a.out  
g/x> /home/bangerth/bin/gcc-3.2.3/bin/c++ x.cc -W -Wall ; ./a.out  
g/x> /home/bangerth/bin/gcc-2.95.3/bin/c++ x.cc -W -Wall ; ./a.out  
 
This should absolutely be fixed for 3.4.1 if possible! 
 
As a sidenote: the bug goes away when one uses 
  const direction_t four = FOUR; 
instead of the non-const version. Maybe that gives a hint as to what 
is going wrong... 
W. 
Comment 7 Andrew Pinski 2004-04-22 16:24:04 UTC
Yes this is a bug in fold.
with:
       int flags = (four) < 0 ? 160 : 0
with:
       int flags = ((four) >> 2u & 1) ? 160 : 0
Comment 8 roger 2004-04-24 03:27:03 UTC
The issue is that fold believes that the enumerated type, which is a signed
quantity that's three bits wide, is sign extended to fill the 32 bit integer.
Testing the most significant bit of the unextended type, direction_t, which is
DIRECTION_OUTPUT, should then be equivalent to testing whether the sign-extended
value is negative.

Clearly the C++ front-end and the middle-end are miscomunicating about how these
enums are to be stored, but it isn't immediately clear which one is at fault.
Should this enum type be unsigned, or have a type that's one bit wider in order
to potentially represent negative values.  Clearly, a signed type thats only
three bits wide can't represent the value 4.
Comment 9 Andrew Pinski 2004-05-04 05:16:44 UTC
*** Bug 15277 has been marked as a duplicate of this bug. ***
Comment 10 Andrew Pinski 2004-05-09 20:46:20 UTC
*** Bug 15354 has been marked as a duplicate of this bug. ***
Comment 11 Mark Mitchell 2004-05-28 22:01:46 UTC
Roger --

The front end is marking the enumeration type as unsigned.

I think that the problem is that fold is simplifying 

  (NOP_EXPR long (CONVERT_EXPR int enum_expr))

to:

  (NOP_EXPR long enum_expr)

which is incorrect; that CONVERT_EXPR needs to be preserved, given that the enum
type is unsigned.

Would you please look into that?

-- Mark
Comment 12 Andrew Pinski 2004-05-28 22:05:52 UTC
I think this is because STRIP_NOP looks into CONVERT_EXPR which it has done for a long time now.
Comment 13 roger 2004-05-28 22:17:34 UTC
A middle-end fix for this was proposed back on 24th April in
http://gcc.gnu.org/ml/gcc-patches/2004-04/msg01657.html
However, RTH agreed with my analysis that this is actually a C++ issue
http://gcc.gnu.org/ml/gcc-patches/2004-04/msg01950.html

This is also intertwined with Jospeph Myers' proposals to overhaul
bit-field enumerations in the C family front-ends.
Comment 14 Andrew Pinski 2004-05-28 22:19:36 UTC
Why are we removing CONVERT_EXPR anyways, the definition of CONVERT_EXPR is that it can generate 
code.
Comment 15 roger 2004-05-28 22:33:03 UTC
The definition of CONVERT_EXPR is that it *may* generate code to perform
the conversion.  It is ultimately the middle-end that decides whether
code needs to be generated or not.  However, this is a red-herring, if
you look at my patch to fold_single_bit_test, you'll notice that the
cause of the failure is unrelated to conversion operations and affects
code without NOP_EXPRs or CONVERT_EXPRs.

It's actually in the code that optimizes "((unsigned char)C & 128) != 0"
into "(char)C < 0"!  When C is a bit-field enumeration, creating a signed
type, returns a type of a different width.  Doh!
Comment 16 Mark Mitchell 2004-05-29 00:03:36 UTC
Roger --

I implemented Richard's suggestion and it doesn't seem to help.

Patch attached.

-- Mark
Comment 17 Mark Mitchell 2004-05-29 00:04:29 UTC
Created attachment 6423 [details]
Draft patch to provide C++-specific signed_type routine
Comment 18 CVS Commits 2004-05-31 17:01:22 UTC
Subject: Bug 15069

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	sayle@gcc.gnu.org	2004-05-31 17:01:17

Modified files:
	gcc            : ChangeLog fold-const.c 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/g++.dg/opt: fold3.C 

Log message:
	PR middle-end/15069
	* fold-const.c (fold_single_bit_test): Only perform "(X & C) != 0"
	into "X < 0" (where C is the signbit) if X's type is a full mode.
	
	* g++.dg/opt/fold3.C: New test case.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.3804&r2=2.3805
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/fold-const.c.diff?cvsroot=gcc&r1=1.386&r2=1.387
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&r1=1.3796&r2=1.3797
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/opt/fold3.C.diff?cvsroot=gcc&r1=NONE&r2=1.1

Comment 19 CVS Commits 2004-05-31 23:15:33 UTC
Subject: Bug 15069

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-3_4-branch
Changes by:	sayle@gcc.gnu.org	2004-05-31 23:15:22

Modified files:
	gcc            : ChangeLog fold-const.c 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/g++.dg/opt: fold3.C 

Log message:
	PR middle-end/15069
	* fold-const.c (fold_single_bit_test): Only perform "(X & C) != 0"
	into "X < 0" (where C is the signbit) if X's type is a full mode.
	
	* g++.dg/opt/fold3.C: New test case.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=2.2326.2.459&r2=2.2326.2.460
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/fold-const.c.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.322.2.10&r2=1.322.2.11
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.3389.2.191&r2=1.3389.2.192
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/opt/fold3.C.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=NONE&r2=1.1.2.1

Comment 20 Andrew Pinski 2004-05-31 23:21:39 UTC
Fixed.
Comment 21 Andrew Pinski 2004-06-02 13:18:52 UTC
*** Bug 15776 has been marked as a duplicate of this bug. ***