broken gcc optimization for facilitynames

broken gcc optimization for facilitynames

[Clément Vasseur]

Hello,

I might have found a gcc code generation problem with the facilitynames
implementation used in musl. See the following reduced test case:

$ cat test-facilitynames.c
#define SYSLOG_NAMES
#include <syslog.h>
int main(void)
{
    for (int i = 0; facilitynames[i].c_name; i++)
        if (facilitynames[i].c_name)
            return facilitynames[i].c_val;
}

$ musl-gcc -std=gnu99 -O1 test-facilitynames.c && ./a.out; echo $?
32

$ musl-gcc -std=gnu99 -O2 test-facilitynames.c && ./a.out; echo $?
1

I see similar results with gcc versions 4.6.1, 4.8.3 and 4.9.0 (with a
different return value with -O2).

Re: broken gcc optimization for facilitynames

[Rich Felker]

On Wed, Jun 18, 2014 at 04:31:58PM +0000, Clément Vasseur wrote:
> Hello,
> 
> I might have found a gcc code generation problem with the facilitynames
> implementation used in musl. See the following reduced test case:
> 
> $ cat test-facilitynames.c
> #define SYSLOG_NAMES
> #include <syslog.h>
> int main(void)
> {
>     for (int i = 0; facilitynames[i].c_name; i++)
>         if (facilitynames[i].c_name)
>             return facilitynames[i].c_val;
> }
> 
> $ musl-gcc -std=gnu99 -O1 test-facilitynames.c && ./a.out; echo $?
> 32
> 
> $ musl-gcc -std=gnu99 -O2 test-facilitynames.c && ./a.out; echo $?
> 1
> 
> I see similar results with gcc versions 4.6.1, 4.8.3 and 4.9.0 (with a
> different return value with -O2).

I can verify that I see this one on gcc 4.7.3. I don't see any UB in
the code so I'm pretty sure this is a gcc bug.

Note that we should really fix this horrible definition of
facilitynames (it bloats busybox by quite a bit), but it's a nice
demonstration of the gcc bug which we should also report and try to
get fixed..

Can you make a self-contained test case (copy the macros from musl's
syslog.h into the source file) and file a gcc bug report?

Rich

Re: broken gcc optimization for facilitynames

[Christopher Meng]

Hmm...

Clément, which gcc version are you using?

Re: broken gcc optimization for facilitynames

[Clément Vasseur]

On 2014-06-19, Rich Felker <dalias@libc.org> wrote:
> On Wed, Jun 18, 2014 at 04:31:58PM +0000, Clément Vasseur wrote:
>> Hello,
>> 
>> I might have found a gcc code generation problem with the facilitynames
>> implementation used in musl. See the following reduced test case:
>> 
>> $ cat test-facilitynames.c
>> #define SYSLOG_NAMES
>> #include <syslog.h>
>> int main(void)
>> {
>>     for (int i = 0; facilitynames[i].c_name; i++)
>>         if (facilitynames[i].c_name)
>>             return facilitynames[i].c_val;
>> }
>> 
>> $ musl-gcc -std=gnu99 -O1 test-facilitynames.c && ./a.out; echo $?
>> 32
>> 
>> $ musl-gcc -std=gnu99 -O2 test-facilitynames.c && ./a.out; echo $?
>> 1
>> 
>> I see similar results with gcc versions 4.6.1, 4.8.3 and 4.9.0 (with a
>> different return value with -O2).
>
> I can verify that I see this one on gcc 4.7.3. I don't see any UB in
> the code so I'm pretty sure this is a gcc bug.
>
> Note that we should really fix this horrible definition of
> facilitynames (it bloats busybox by quite a bit), but it's a nice
> demonstration of the gcc bug which we should also report and try to
> get fixed..
>
> Can you make a self-contained test case (copy the macros from musl's
> syslog.h into the source file) and file a gcc bug report?

I made some progress towards a standalone test case. Here it is:

struct s1 { int v; };
struct s2 { int v; };

#define a ((struct s2 *)(struct s1 []){{ 42 }})

int main(void)
{
	for (int i = 0; a[i].v; i++)
		if (a[i].v)
			return a[i].v;
}

I also found out which optimization flag causes the broken output, it's
-fstrict-aliasing. I guess the issue here is whether casting `struct s1
[]` to `struct s2 *` violates the strict aliasing rules or not. Indeed,
compiling with -Wstrict-aliasing=1 shows a warning at this location.

Can someone pinpoint the exact location in the ISO C spec where there is
an explanation about the aliasing rules with this kind of pointer
compatibility?

Re: broken gcc optimization for facilitynames

[Clément Vasseur]

On 2014-06-20, Christopher Meng <cickumqt@gmail.com> wrote:
> Hmm...
>
> Clément, which gcc version are you using?

Like I said before: 4.6.1, 4.8.3 and 4.9.0.

Re: Re: broken gcc optimization for facilitynames

[Szabolcs Nagy]

* Cl?ment Vasseur <clement.vasseur@gmail.com> [2014-06-20 22:09:17 +0000]:
> 
> I made some progress towards a standalone test case. Here it is:
> 
> struct s1 { int v; };
> struct s2 { int v; };
> 
> #define a ((struct s2 *)(struct s1 []){{ 42 }})
> 
> int main(void)
> {
> 	for (int i = 0; a[i].v; i++)
> 		if (a[i].v)
> 			return a[i].v;
> }
> 
> I also found out which optimization flag causes the broken output, it's
> -fstrict-aliasing. I guess the issue here is whether casting `struct s1
> []` to `struct s2 *` violates the strict aliasing rules or not. Indeed,
> compiling with -Wstrict-aliasing=1 shows a warning at this location.
> 
> Can someone pinpoint the exact location in the ISO C spec where there is
> an explanation about the aliasing rules with this kind of pointer
> compatibility?

http://port70.net/~nsz/c/c11/n1570.html#6.5p7

different structs are not compatible types and
object with s1 effective type is accessed through
an expression with s2 type

(if a union of s1 and s2 were used then it would
be ok, because of the "common initial sequence"
rule, or if the first member of s1 had type s2,
because aggregate member can be aliased that way)

so yes this is an aliasing violation and syslog.h
should be fixed

Re: Re: broken gcc optimization for facilitynames

[Rich Felker]

On Fri, Jun 20, 2014 at 10:09:17PM +0000, Clément Vasseur wrote:
> On 2014-06-19, Rich Felker <dalias@libc.org> wrote:
> > On Wed, Jun 18, 2014 at 04:31:58PM +0000, Clément Vasseur wrote:
> >> Hello,
> >> 
> >> I might have found a gcc code generation problem with the facilitynames
> >> implementation used in musl. See the following reduced test case:
> >> 
> >> $ cat test-facilitynames.c
> >> #define SYSLOG_NAMES
> >> #include <syslog.h>
> >> int main(void)
> >> {
> >>     for (int i = 0; facilitynames[i].c_name; i++)
> >>         if (facilitynames[i].c_name)
> >>             return facilitynames[i].c_val;
> >> }
> >> 
> >> $ musl-gcc -std=gnu99 -O1 test-facilitynames.c && ./a.out; echo $?
> >> 32
> >> 
> >> $ musl-gcc -std=gnu99 -O2 test-facilitynames.c && ./a.out; echo $?
> >> 1
> >> 
> >> I see similar results with gcc versions 4.6.1, 4.8.3 and 4.9.0 (with a
> >> different return value with -O2).
> >
> > I can verify that I see this one on gcc 4.7.3. I don't see any UB in
> > the code so I'm pretty sure this is a gcc bug.
> >
> > Note that we should really fix this horrible definition of
> > facilitynames (it bloats busybox by quite a bit), but it's a nice
> > demonstration of the gcc bug which we should also report and try to
> > get fixed..
> >
> > Can you make a self-contained test case (copy the macros from musl's
> > syslog.h into the source file) and file a gcc bug report?
> 
> I made some progress towards a standalone test case. Here it is:
> 
> struct s1 { int v; };
> struct s2 { int v; };
> 
> #define a ((struct s2 *)(struct s1 []){{ 42 }})
> 
> int main(void)
> {
> 	for (int i = 0; a[i].v; i++)
> 		if (a[i].v)
> 			return a[i].v;
> }
> 
> I also found out which optimization flag causes the broken output, it's
> -fstrict-aliasing. I guess the issue here is whether casting `struct s1
> []` to `struct s2 *` violates the strict aliasing rules or not. Indeed,
> compiling with -Wstrict-aliasing=1 shows a warning at this location.

It's indeed an aliasing violation. And I don't see what the point of
it was in the first place. Whether c_name has type char * or const
char * has no bearing on whether the data can reside in .text/.rodata.

> Can someone pinpoint the exact location in the ISO C spec where there is
> an explanation about the aliasing rules with this kind of pointer
> compatibility?

C99 6.5 paragraph 7.

Rich