Bug 448743 - O2 optimization fails to generate code for statement
Summary: O2 optimization fails to generate code for statement
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: gcc
Version: 9
Hardware: i386
OS: Linux
medium
high
Target Milestone: ---
Assignee: Jakub Jelinek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 446864
TreeView+ depends on / blocked
 
Reported: 2008-05-28 15:19 UTC by John Dennis
Modified: 2008-05-28 22:30 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-05-28 22:30:54 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
original C code (18.49 KB, text/plain)
2008-05-28 15:20 UTC, John Dennis
no flags Details
preprocessed C code (165.06 KB, text/plain)
2008-05-28 15:21 UTC, John Dennis
no flags Details
generated object code (31.34 KB, application/octet-stream)
2008-05-28 15:21 UTC, John Dennis
no flags Details
disassembled machine code with annotations (5.83 KB, text/plain)
2008-05-28 15:22 UTC, John Dennis
no flags Details

Description John Dennis 2008-05-28 15:19:13 UTC
This problem was first seen in the F-9 build of FreeRADIUS when the server
failed to listen on the correct port (bug #446864). It was observed the program
behaved correctly when compiled without optimization.

I have tracked the problem down by examining generating a preprocessed source
file, compiling that under the same options and then looking at the disassembled
code. It appears as though the following statement in the original C code in the
function fr_socket(), line 208, file packet.c):

sa->sin_port = htons((uint16_t) port);

is not generating any code when compiled with -O2 on i386, this leaves the port
unspecified leading to incorrect program behavior. That line when preprocessed
expands to:

sa->sin_port = (__extension__ ({ register unsigned short int __v, __x =
((uint16_t) port); if (__builtin_constant_p (__x)) __v = ((((__x) >> 8) & 0xff)
| (((__x) & 0xff) << 8)); else __asm__ ("rorw $8, %w0" : "=r" (__v) : "0" (__x)
: "cc"); __v; }));

Examination of the disassembled code in GDB shows sa->sin_port is never assigned
(or at least I believe that is the case, it's not always easy to correctly
interpret generated machine code).

I have attached the original C source file (packet.c), the preprocessed file
(packet.i with some annotations correlating it to the original C code), the
object file (packet.o), and the disassembled function (packet.asm).

The compiler version used was (Note this may not be the compiler used on the
build systems when the problem was first seen, but it reproduces with this):

[jdennis@localhost F-9]$ gcc -v
Using built-in specs.
Target: i386-redhat-linux
Configured with: ../configure --prefix=/usr --mandir=/usr/share/man
--infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla
--enable-bootstrap --enable-shared --enable-threads=posix
--enable-checking=release --with-system-zlib --enable-__cxa_atexit
--disable-libunwind-exceptions
--enable-languages=c,c++,objc,obj-c++,java,fortran,ada --enable-java-awt=gtk
--disable-dssi --enable-plugin
--with-java-home=/usr/lib/jvm/java-1.5.0-gcj-1.5.0.0/jre
--enable-libgcj-multifile --enable-java-maintainer-mode
--with-ecj-jar=/usr/share/java/eclipse-ecj.jar --disable-libjava-multilib
--with-cpu=generic --build=i386-redhat-linux
Thread model: posix
gcc version 4.3.0 20080428 (Red Hat 4.3.0-8) (GCC) 

The command line to compile the original C codes was:

gcc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i686 -mtune=generic
-fasynchronous-unwind-tables -fpic -D_LIBRADIUS
-I/home/jdennis/src/fedora/freeradius/F-9/freeradius-server-2.0.3/src -c
packet.c  -fPIC -DPIC -o .libs/packet.o

The command line used to generate the preprocessed file (packet.i) was:

gcc -E -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables -fpic -D_LIBRADIUS 
-I/home/jdennis/src/fedora/freeradius/F-9/freeradius-server-2.0.3/src -c
packet.c  -fPIC -DPIC > ../../../packet.i

The command line used to complile the preprocessed file into machine code was:

gcc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables -fpic -D_LIBRADIUS 
-I/home/jdennis/src/fedora/freeradius/F-9/freeradius-server-2.0.3/src -c
packet.i  -fPIC -DPIC

I wonder if the culprit is the __extension__ pragma, that is the only statement
which seems to omit generated code.

Comment 1 John Dennis 2008-05-28 15:20:47 UTC
Created attachment 306938 [details]
original C code

Comment 2 John Dennis 2008-05-28 15:21:28 UTC
Created attachment 306939 [details]
preprocessed C code

Comment 3 John Dennis 2008-05-28 15:21:55 UTC
Created attachment 306940 [details]
generated object code

Comment 4 John Dennis 2008-05-28 15:22:38 UTC
Created attachment 306941 [details]
disassembled machine code with annotations

Comment 5 Jakub Jelinek 2008-05-28 20:45:01 UTC
The problem is an aliasing violation.
struct sockaddr_storage salocal;
...
struct sockaddr_in *sa = (struct sockaddr_in *)&salocal;
...
sa->sin_port = XXX;
...
bind (..., (struct sockaddr *) &salocal, ...);

As sockaddr_storage and sockaddr_in types aren't compatible, the salocal object
has effective type sockaddr_storage (as per ISO C99, 6.5 (6)), the store to
sa->sin_port is invalid.  Now, I don't know what is the POSIX intended usage of
struct sockaddr_storage.  Either this is supposed to be a valid POSIX use (in
that case we'd need to make struct sockaddr_storage effectively a union of all
known non-AF_UNIX sockaddrs), or it is not and users need to use a union
themselves (union { sockaddr_storage salocal; struct sockaddr_in sa; struct
sockaddr_in6 sa6; }; in this case).

Comment 6 Ulrich Drepper 2008-05-28 21:18:14 UTC
Regardless of whether we try to work around this kind of problem in future, the
fr_socket function is wrong.  The invalid casts are forbidden by ISO C.  POSIX
does not and cannot guarantee anything about this type of use of sockaddr_storage.

What you should do is this:

union
{
  struct sockaddr_storage ss;
  struct sockaddr_in s4;
  struct sockaddr_in6 s6;
} u;

- replace salocal references with u.ss

- in the IPv4 branch, use u.s4 instead of the local sa variable

- in the IPv6 branch, use u.s6 instead of the local sa variable


Note You need to log in before you can comment on or make changes to this bug.