Possible bug in gcc 4.4.7

Possible bug in gcc 4.4.7

[Andy Falanga (afalanga)]

Hello,

Below is a driver program which I was using to test my approach for using enumerations as flags.  I have a couple of enumerations I'm using in C++ which are bitwise flags.  The language spec, so I have learned, states that when you bitwise for enumeration values together the result is no longer an enumeration value but simply an int.  As I thought of this, this makes sense and is a wise decision for the definition.

To work around this, I've overloaded the binary operators that I'm using in the code.  The driver program shows the case in which I'm using them.  When I compile with no optimizations, the program executes as it should.  When the driver program is compiled using -O2 optimizations, which our production code is, the function Test() runs the for loop twice for each bit in the Flag parameter.  I've stepped through with gdb and the result is *not* correct.  For example,

62              for(int i = 0; f & (one | two | four | eight); ++i)
(gdb) 
64                      if(f & one)
(gdb) p/x f
$3 = 0xf
(gdb) s
66                              std::cout << "Found one" << std::endl;
(gdb) 
Found one
67                              f &= ~one;
(gdb) p/x f
$4 = 0xf
(gdb) n
62              for(int i = 0; f & (one | two | four | eight); ++i)
(gdb) p/x f
$5 = 0xe
(gdb) n
64                      if(f & one)
(gdb) 
66                              std::cout << "Found one" << std::endl;
(gdb) p/x f
$6 = 0xe

As you can see, the second time the "if(f & one)" clause is executed, the value of f is 0xe (bit 0 is off), yet the result of the bitwise and is true.  The second time f &= ~one is executed the result is still 0xe but the next time it runs through the conditional, it passes as expected.

The output from this program, when optimized, is:
Found one
Found one
Found two
Found two
Found four
Found four
Found eight
Found eight
Found four
Found four
UInt32 version
Found eight
Found eight

which is not correct.  The output is correct when the code is not optimized.  I would like to know if it is generally agreed that this is a bug in 4.4.7, or, frankly, if I've done something incorrect in the code.  I happen to have 4.8 installed on my system and this driver program (the code is below), works as expected with or without optimizations.

Thanks
Andy

#include <iostream>
#include <type_traits>

typedef unsigned int UInt32;

enum Flag {
	one = 0x1,
	two = 0x2, 
	four = 0x4,
	eight = 0x8
};

template <typename T, typename S>
struct PodToEnum {
	S s;
	operator T() { return static_cast<T>(s); }
	PodToEnum(S t) : s(t) {
		if(!std::is_integral<S>::value) {
			std::cout << "Can't convert types that aren't integers" << std::endl;
			throw "up";
		}
	}
};

template <>
struct PodToEnum<Flag, UInt32>  {
	int I;
	operator Flag() { return static_cast<Flag>(I); };
	PodToEnum(int i) : I(i) {
		std::cout << "UInt32 version" << std::endl;
		if(i != 1 && i != 2 && i != 4 && i != 8) {
			throw "up";
		}
	}
};

Flag operator|(Flag f1, Flag f2) {
	return static_cast<Flag>(int(f1) | int(f2));
}

Flag operator~(Flag f) {
	return static_cast<Flag>(~(static_cast<unsigned int>(f)));
}

Flag operator &=(Flag& f1, Flag f2) {
	return static_cast<Flag>(reinterpret_cast<unsigned int&>(f1) &= static_cast<unsigned int>(f2));
}

void Test(Flag f) {
	for(int i = 0; f & (one | two | four | eight); ++i)
	{
		if(f & one)
		{
			std::cout << "Found one" << std::endl;
			f &= ~one;
		}
		else if(f & two)
		{
			std::cout << "Found two" << std::endl;
			f &= ~two;
		}
		else if(f & four)
		{
			std::cout << "Found four" << std::endl;
			f &= ~four;
		}
		else
		{
			std::cout << "Found eight" << std::endl;
			f &= ~eight;
		}
	}
}

int main() {
	Flag f = (one | two | four | eight);
	Test(f);

	Test(static_cast<Flag>(4));
	Test(PodToEnum<Flag, UInt32>(8));
	return 0;
}

Re: Possible bug in gcc 4.4.7

[Jonathan Wakely]

On 17 September 2014 17:16, Andy Falanga (afalanga) wrote:
> Flag operator &=(Flag& f1, Flag f2) {
>         return static_cast<Flag>(reinterpret_cast<unsigned int&>(f1) &= static_cast<unsigned int>(f2));
> }

That reinterpret_cast looks dodgy to me, accessing the object through
a different type is undefined behaviour. What's wrong with doing it
safely?

Flag operator &=(Flag& f1, Flag f2) {
  unsigned int i = f1;
  i &= static_cast<unsigned int>(f2);
  return f1 = static_cast<Flag>(i);
}

RE: Possible bug in gcc 4.4.7

[Andy Falanga (afalanga)]

> -----Original Message-----
> From: gcc-help-owner@gcc.gnu.org [mailto:gcc-help-owner@gcc.gnu.org] On
> Behalf Of Jonathan Wakely
> Sent: Wednesday, September 17, 2014 11:01 AM
> To: Andy Falanga (afalanga)
> Cc: gcc-help@gcc.gnu.org
> Subject: Re: Possible bug in gcc 4.4.7
> 
> On 17 September 2014 17:16, Andy Falanga (afalanga) wrote:
> > Flag operator &=(Flag& f1, Flag f2) {
> >         return static_cast<Flag>(reinterpret_cast<unsigned int&>(f1)
> > &= static_cast<unsigned int>(f2)); }
> 
> That reinterpret_cast looks dodgy to me, accessing the object through a
> different type is undefined behaviour. What's wrong with doing it
> safely?
> 
> Flag operator &=(Flag& f1, Flag f2) {
>   unsigned int i = f1;
>   i &= static_cast<unsigned int>(f2);
>   return f1 = static_cast<Flag>(i);
> }

Ha, nothing except my thinking that, because I was casting a reference, I had to use reinterpret_cast<>.  Of course, that (your suggestion) worked for me.  Please point me to the correct section of the language spec so that I might better understand why this is undefined behavior.

Thanks Jonathan.

Andy

Re: Possible bug in gcc 4.4.7

[Andrew Haley]

On 09/17/2014 06:15 PM, Andy Falanga (afalanga) wrote:

> Ha, nothing except my thinking that, because I was casting a
> reference, I had to use reinterpret_cast<>.  Of course, that (your
> suggestion) worked for me.  Please point me to the correct section
> of the language spec so that I might better understand why this is
> undefined behavior.

Here's the C version:

"6.5 Expressions":

An object shall have its stored value accessed only by an lvalue
expression that has one of the following types:

    a type compatible with the effective type of the object,

    a qualified version of a type compatible with the effective type
    of the object,

    a type that is the signed or unsigned type corresponding to the
    effective type of the object,

    a type that is the signed or unsigned type corresponding to a
    qualified version of the effective type of the object,

    an aggregate or union type that includes one of the aforementioned
    types among its members (including, recursively, a member of a
    subaggregate or contained union), or

    a character type.

C++ uses the same rule.  If you access an object using an lvalue
expression of an incompatible type, all bets are off.

static_cast<Flag>(reinterpret_cast<unsigned int&>(f1) is an example
of such an expression.

Andrew.

RE: Possible bug in gcc 4.4.7

[Andy Falanga (afalanga)]

> -----Original Message-----
> From: gcc-help-owner@gcc.gnu.org [mailto:gcc-help-owner@gcc.gnu.org] On
> Behalf Of Andrew Haley
> Sent: Wednesday, September 17, 2014 12:06 PM
> To: Andy Falanga (afalanga); Jonathan Wakely
> Cc: gcc-help@gcc.gnu.org
> Subject: Re: Possible bug in gcc 4.4.7
> 
> On 09/17/2014 06:15 PM, Andy Falanga (afalanga) wrote:
> 
> > Ha, nothing except my thinking that, because I was casting a
> > reference, I had to use reinterpret_cast<>.  Of course, that (your
> > suggestion) worked for me.  Please point me to the correct section of
> > the language spec so that I might better understand why this is
> > undefined behavior.
> 
> Here's the C version:
> 
> "6.5 Expressions":
> 
> An object shall have its stored value accessed only by an lvalue
> expression that has one of the following types:
> 
>     a type compatible with the effective type of the object,
> 
>     a qualified version of a type compatible with the effective type
>     of the object,
> 
>     a type that is the signed or unsigned type corresponding to the
>     effective type of the object,
> 
>     a type that is the signed or unsigned type corresponding to a
>     qualified version of the effective type of the object,
> 
>     an aggregate or union type that includes one of the aforementioned
>     types among its members (including, recursively, a member of a
>     subaggregate or contained union), or
> 
>     a character type.
> 
> C++ uses the same rule.  If you access an object using an lvalue
> expression of an incompatible type, all bets are off.
> 
> static_cast<Flag>(reinterpret_cast<unsigned int&>(f1) is an example of
> such an expression.
> 
> Andrew.

Thank you for the reference.  I'm going to have to think on this for a bit.  I read a bit of the definition for reinterpret_cast in the C++ Draft I have too.  I want to figure this out for sure.

Andy

Re: Possible bug in gcc 4.4.7

[Jonathan Wakely]

On 17 September 2014 18:15, Andy Falanga (afalanga) wrote:
>> That reinterpret_cast looks dodgy to me, accessing the object through a
>> different type is undefined behaviour. What's wrong with doing it
>> safely?
>>
>> Flag operator &=(Flag& f1, Flag f2) {
>>   unsigned int i = f1;
>>   i &= static_cast<unsigned int>(f2);
>>   return f1 = static_cast<Flag>(i);
>> }
>
> Ha, nothing except my thinking that, because I was casting a reference, I had to use reinterpret_cast<>.

Well yes, if you want to cast T& to X& and T and X are not related by
inheritance then you do need to use reinterpret_cast, but that should
make you think "maybe this is not a valid cast, maybe I shouldn't do
it" rather than "if I use reinterpret_cast the compiler doesn't
complain so it must be the way to go".

Re: Possible bug in gcc 4.4.7

[Jonathan Wakely]

On 17 September 2014 21:34, Andy Falanga (afalanga) wrote:
> Thank you for the reference.  I'm going to have to think on this for a bit.  I read a bit of the definition for reinterpret_cast in the C++ Draft I have too.  I want to figure this out for sure.

I think you're barking up the wrong tree if you're trying to
understand when reinterpret_cast is suitable ... because it is almost
never a good idea!  reinterpret_cast basically means "just shut up and
do this cast, I know what I'm doing", but that means you've lost the
advantages of having the compiler do type checking.

The rules Andrew quoted say when it's OK to violate the language's
type system. If you try to break those rules you can use
reinterpret_cast to make the compiler shut up and do what it's told,
but you're still breaking the rules.

RE: Possible bug in gcc 4.4.7

[Andy Falanga (afalanga)]

> -----Original Message-----
> From: Jonathan Wakely [mailto:jwakely.gcc@gmail.com]
> Sent: Wednesday, September 17, 2014 6:13 PM
> To: Andy Falanga (afalanga)
> Cc: Andrew Haley; gcc-help@gcc.gnu.org
> Subject: Re: Possible bug in gcc 4.4.7
> 
> On 17 September 2014 21:34, Andy Falanga (afalanga) wrote:
> > Thank you for the reference.  I'm going to have to think on this for
> a bit.  I read a bit of the definition for reinterpret_cast in the C++
> Draft I have too.  I want to figure this out for sure.
> 
> I think you're barking up the wrong tree if you're trying to understand
> when reinterpret_cast is suitable ... because it is almost never a good
> idea!  reinterpret_cast basically means "just shut up and do this cast,
> I know what I'm doing", but that means you've lost the advantages of
> having the compiler do type checking.

Thank you for the warning.  In this case, I'm not trying to understand when to violate the rules but, rather, to understand why what I did resulted in undefined behavior.  I would have blissfully thought I'd found a bug had I not posted here.  I would have thought this, not because of hubris, but because the 4.8 compiler produced the result I was looking for.  After being corrected, I want to understand my error.

> 
> The rules Andrew quoted say when it's OK to violate the language's type
> system. If you try to break those rules you can use reinterpret_cast to
> make the compiler shut up and do what it's told, but you're still
> breaking the rules.

Ok, I think I'm beginning to understand this better.  There is really much to this language and I have much to learn.

Re: Possible bug in gcc 4.4.7

[Jonathan Wakely]

On 18 September 2014 15:24, Andy Falanga (afalanga) <afalanga@micron.com> wrote:
>> -----Original Message-----
>> From: Jonathan Wakely [mailto:jwakely.gcc@gmail.com]
>> Sent: Wednesday, September 17, 2014 6:13 PM
>> To: Andy Falanga (afalanga)
>> Cc: Andrew Haley; gcc-help@gcc.gnu.org
>> Subject: Re: Possible bug in gcc 4.4.7
>>
>> On 17 September 2014 21:34, Andy Falanga (afalanga) wrote:
>> > Thank you for the reference.  I'm going to have to think on this for
>> a bit.  I read a bit of the definition for reinterpret_cast in the C++
>> Draft I have too.  I want to figure this out for sure.
>>
>> I think you're barking up the wrong tree if you're trying to understand
>> when reinterpret_cast is suitable ... because it is almost never a good
>> idea!  reinterpret_cast basically means "just shut up and do this cast,
>> I know what I'm doing", but that means you've lost the advantages of
>> having the compiler do type checking.
>
> Thank you for the warning.  In this case, I'm not trying to understand when to violate the rules but, rather, to understand why what I did resulted in undefined behavior.

So looking into reinterpret_cast is not very relevant.

You are using a reference of type unsigned int& to access something
that is not an unsigned int, that's the problem.

The fact you used reinterpret_cast to create the reference is not
relevant, it's what you did with the reference after you created it
that is the problem. This is undefined in the same way, but doesn't
use reinterpret_cast:

Flag f1;
void* vp = &f1;
*static_cast<unsigned int*>(vp) = 1;

RE: Possible bug in gcc 4.4.7

[Andy Falanga (afalanga)]

> 
> So looking into reinterpret_cast is not very relevant.
> 
> You are using a reference of type unsigned int& to access something
> that is not an unsigned int, that's the problem.
> 
> The fact you used reinterpret_cast to create the reference is not
> relevant, it's what you did with the reference after you created it
> that is the problem. This is undefined in the same way, but doesn't use
> reinterpret_cast:
> 
> Flag f1;
> void* vp = &f1;
> *static_cast<unsigned int*>(vp) = 1;


Ok, thanks.  I think I get it now.