Questions before submitting a bug report

Questions before submitting a bug report

[Markus Henschel]

Hello,

I think I found a bug in gcc but before I submit it I'd like to have someone look at least briefly on the code to make sure I did not make some silly mistake.

*********************************************schnipp***************
struct Foo
{
	long long _foobar;
};

struct Foo * foo_instance()
{
	static struct Foo foo;
	return &foo;
}

void bar(struct Foo * foo)
{
	__sync_add_and_fetch(&foo->_foobar, 1LL);
}


int main(int argc, char * argv[])
{
	struct Foo * foo=foo_instance();
	foo->_foobar=argc;
	bar(foo);
	return (int)foo->_foobar;
}
*********************************************schnapp***************

Is there some obvious flaw in my code? 

My second question is if it makes any sense to submit this bug if it's against gcc version 4.2.4. I still have to use this compiler version but the bug seems to have vanished in version 4.3.3. Is 4.2.4 still maintained?

Thanks for your time.

Markus

Re: Questions before submitting a bug report

[Andrew Haley]

On 01/03/2012 03:02 PM, Markus Henschel wrote:
> struct Foo * foo_instance()
> {
> 	static struct Foo foo;
> 	return &foo;
> }

Uh, no.  You can't return a reference to a local
variable.

Andrew.

RE: Questions before submitting a bug report

[Markus Henschel]

> -----Original Message-----
> From: gcc-help-owner@gcc.gnu.org [mailto:gcc-help-owner@gcc.gnu.org] On
> Behalf Of Andrew Haley
> Sent: Dienstag, 3. Januar 2012 16:06
> To: gcc-help@gcc.gnu.org
> Subject: Re: Questions before submitting a bug report
> 
> On 01/03/2012 03:02 PM, Markus Henschel wrote:
> > struct Foo * foo_instance()
> > {
> > 	static struct Foo foo;
> > 	return &foo;
> > }
> 
> Uh, no.  You can't return a reference to a local variable.
> 
> Andrew.

But it's a static variable. Its lifetime extends across the runtime of the whole program. I'm not 100% sure for C but at least in C++ this is a well known pattern for singletons.

Re: Questions before submitting a bug report

[Andrew Haley]

On 01/03/2012 03:23 PM, Markus Henschel wrote:
> 
> 
>> -----Original Message-----
>> From: gcc-help-owner@gcc.gnu.org [mailto:gcc-help-owner@gcc.gnu.org] On
>> Behalf Of Andrew Haley
>> Sent: Dienstag, 3. Januar 2012 16:06
>> To: gcc-help@gcc.gnu.org
>> Subject: Re: Questions before submitting a bug report
>>
>> On 01/03/2012 03:02 PM, Markus Henschel wrote:
>>> struct Foo * foo_instance()
>>> {
>>> 	static struct Foo foo;
>>> 	return &foo;
>>> }
>>
>> Uh, no.  You can't return a reference to a local variable.
> 
> But it's a static variable.

My apologies.  I posted too soon.

Andrew.

Re: Questions before submitting a bug report

[Andrew Haley]

On 01/03/2012 03:02 PM, Markus Henschel wrote:
> Is there some obvious flaw in my code? 

What error did you get?  Not all CPUs can do
__sync_add_and_fetch on a long long.

Andrew.

RE: Questions before submitting a bug report

[Markus Henschel]

> -----Original Message-----
> From: gcc-help-owner@gcc.gnu.org [mailto:gcc-help-owner@gcc.gnu.org] On
> Behalf Of Andrew Haley
> Sent: Dienstag, 3. Januar 2012 16:26
> To: gcc-help@gcc.gnu.org
> Subject: Re: Questions before submitting a bug report
> 
> On 01/03/2012 03:02 PM, Markus Henschel wrote:
> > Is there some obvious flaw in my code?
> 
> What error did you get?  Not all CPUs can do __sync_add_and_fetch on a
> long long.

The resulting program crashes with a segmentation fault on the line with __sync_add_and_fetch. This is the complete command line I used for compiling:

"gcc -fPIC -march=i586 -fvisibility=hidden -O3 mytest.c -o mytest"

The error goes away if I remove "-fPIC", "-fvisibility=hidden" or if I use some less aggressive optimization. It also disappears if I use a long instead of a long long. 

Re: Questions before submitting a bug report

[Andrew Haley]

On 01/03/2012 03:39 PM, Markus Henschel wrote:
>> -----Original Message-----
>> From: gcc-help-owner@gcc.gnu.org [mailto:gcc-help-owner@gcc.gnu.org] On
>> Behalf Of Andrew Haley
>> Sent: Dienstag, 3. Januar 2012 16:26
>> To: gcc-help@gcc.gnu.org
>> Subject: Re: Questions before submitting a bug report
>>
>> On 01/03/2012 03:02 PM, Markus Henschel wrote:
>>> Is there some obvious flaw in my code?
>>
>> What error did you get?  Not all CPUs can do __sync_add_and_fetch on a
>> long long.
> 
> The resulting program crashes with a segmentation fault on the line with __sync_add_and_fetch. This is the complete command line I used for compiling:
> 
> "gcc -fPIC -march=i586 -fvisibility=hidden -O3 mytest.c -o mytest"
> 
> The error goes away if I remove "-fPIC", "-fvisibility=hidden" or if I use some less aggressive optimization. It also disappears if I use a long instead of a long long. 

I think you've found yourself a bug.  I can't reproduce it, nor can
I see anything wrong with your code.  It's be interesting to see what
instruction makes it crash.

Andrew.

RE: Questions before submitting a bug report

[Markus Henschel]

> -----Original Message-----
> From: gcc-help-owner@gcc.gnu.org [mailto:gcc-help-owner@gcc.gnu.org] On
> Behalf Of Andrew Haley
> Sent: Dienstag, 3. Januar 2012 16:52
> To: gcc-help@gcc.gnu.org
> Subject: Re: Questions before submitting a bug report
> 
> On 01/03/2012 03:39 PM, Markus Henschel wrote:
> >> -----Original Message-----
> >> From: gcc-help-owner@gcc.gnu.org [mailto:gcc-help-owner@gcc.gnu.org]
> >> On Behalf Of Andrew Haley
> >> Sent: Dienstag, 3. Januar 2012 16:26
> >> To: gcc-help@gcc.gnu.org
> >> Subject: Re: Questions before submitting a bug report
> >>
> >> On 01/03/2012 03:02 PM, Markus Henschel wrote:
> >>> Is there some obvious flaw in my code?
> >>
> >> What error did you get?  Not all CPUs can do __sync_add_and_fetch on
> >> a long long.
> >
> > The resulting program crashes with a segmentation fault on the line
> with __sync_add_and_fetch. This is the complete command line I used for
> compiling:
> >
> > "gcc -fPIC -march=i586 -fvisibility=hidden -O3 mytest.c -o mytest"
> >
> > The error goes away if I remove "-fPIC", "-fvisibility=hidden" or if
> I use some less aggressive optimization. It also disappears if I use a
> long instead of a long long.
> 
> I think you've found yourself a bug.  I can't reproduce it, nor can I
> see anything wrong with your code.  It's be interesting to see what
> instruction makes it crash.
> 
> Andrew.

Thanks for having a look at this. To satisfy your curiosity here is the asm output of gdb for an even simpler test case:
int main(int argc, char * argv[])
{
	static long long foo;
	return __sync_add_and_fetch(&foo, foo);
}

I don't know if this is meaningful because I don't know much about assembler.

   ┌───────────────────────────────────────────────────────────────────────────┐
   │0x8048350 <main>        lea    0x4(%esp),%ecx                              │
   │0x8048354 <main+4>      and    $0xfffffff0,%esp                            │
   │0x8048357 <main+7>      pushl  -0x4(%ecx)                                  │
   │0x804835a <main+10>     push   %ebp                                        │
   │0x804835b <main+11>     mov    %esp,%ebp                                   │
   │0x804835d <main+13>     push   %edi                                        │
   │0x804835e <main+14>     push   %esi                                        │
   │0x804835f <main+15>     push   %ebx                                        │
   │0x8048360 <main+16>     push   %ecx                                        │
   │0x8048361 <main+17>     call   0x8048366 <main+22>                         │
   │0x8048366 <main+22>     pop    %ebx                                        │
   │0x8048367 <main+23>     add    $0x1206,%ebx                                │
   │0x804836d <main+29>     sub    $0x8,%esp                                   │
   │0x8048370 <main+32>     mov    0x2c(%ebx),%eax                             │
   │0x8048376 <main+38>     mov    0x30(%ebx),%edx                             │
   │0x804837c <main+44>     mov    %eax,%esi                                   │
   │0x804837e <main+46>     mov    %edx,%edi                                   │
   │0x8048380 <main+48>     add    0x2c(%ebx),%esi                             │
   │0x8048386 <main+54>     adc    0x30(%ebx),%edi                             │
   │0x804838c <main+60>     mov    %esi,-0x18(%ebp)                            │
   │0x804838f <main+63>     mov    %edi,-0x14(%ebp)                            │
   │0x8048392 <main+66>     mov    %esi,%edi                                   │
   │0x8048394 <main+68>     mov    -0x14(%ebp),%ecx                            │
   │0x8048397 <main+71>     xchg   %ebx,%edi                                   │
  >│0x8048399 <main+73>     lock cmpxchg8b 0x2c(%ebx)                          │
   │0x80483a1 <main+81>     xchg   %ebx,%edi                                   │
   │0x80483a3 <main+83>     jne    0x804837c <main+44>                         │
   │0x80483a5 <main+85>     mov    -0x18(%ebp),%eax                            │
   │0x80483a8 <main+88>     pop    %edx                                        │
   └───────────────────────────────────────────────────────────────────────────┘
child process 15488 In: main                           Line: ??   PC: 0x8048399 
(gdb) run
Starting program: /home/ubuntu/mytest 

Program received signal SIGSEGV, Segmentation fault.
0x08048399 in main ()

If gcc 4.2 isn't maintained anymore I'll use a workaround and see that we can switch to a newer gcc release in the future. 

Thank you for your time.

Markus

Re: Questions before submitting a bug report

[Kalle Olavi Niemitalo]

Markus Henschel <markus.henschel@yager.de> writes:

>    │0x8048397 <main+71>     xchg   %ebx,%edi                                   │
>   >│0x8048399 <main+73>     lock cmpxchg8b 0x2c(%ebx)                          │
>    │0x80483a1 <main+81>     xchg   %ebx,%edi                                   │

It looks like GCC 4.2.4 has used the wrong register in the
cmpxchg8b instruction.  The earlier part of this function uses
EBX to point to the Global Offset Table.  Because the cmpxchg8b
instruction requires that the ECX:EBX register pair contains the
64-bit value to be stored in memory, GCC has temporarily moved
the address of the GOT from EBX to EDI; so it should have used
0x2c(%edi) in the cmpxchg8b instruction.  The generated code
crashes because it tries to use the low 32 bits of the data value
as the address of the GOT.

A similar bug has already been reported:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=37651
"__sync_bool_compare_and_swap creates wrong code with -fPIC"
It too is about using %ebx as a pointer in cmpxchg8b.
I can reproduce that bug with 4.2.4 but not with 4.4.6 or 4.6.2;
so I guess the report could be closed now.

Omitting -fPIC avoids the bug because accessing the foo variable
then does not need the GOT and the cmpxchg8b instruction uses a
literal address that does not involve any register.

With your original source, omitting -fvisibility=hidden avoids
the bug because it prevents foo_instance and bar from being
inlined and the out-of-line code generated for bar accesses
foo->_foobar via the pointer parameter, rather than via the GOT.
With the simplified source, -fvisibility=hidden does not matter
for the bug.

For a workaround, I suggest

int main(int argc, char * argv[])
{
	static long long foo;
	long long *volatile fooptr = &foo;
	return __sync_add_and_fetch(fooptr, foo);
}

as this seems to force GCC 4.2.4 to compute the address first,
rather than try to access the variable via the GOT as part of the
cmpxchg8b instruction.

Re: Questions before submitting a bug report

[Jonathan Wakely]

On 3 January 2012 15:02, Markus Henschel wrote:
> My second question is if it makes any sense to submit this bug if it's against gcc version 4.2.4. I still have to use this compiler version but the bug seems to have vanished in version 4.3.3. Is 4.2.4 still maintained?

No, as shown on the gcc.gnu.org home page, the oldest maintained
release series is GCC 4.4