Bug 448743 - O2 optimization fails to generate code for statement
O2 optimization fails to generate code for statement
Product: Fedora
Classification: Fedora
Component: gcc (Show other bugs)
i386 Linux
medium Severity high
: ---
: ---
Assigned To: Jakub Jelinek
Fedora Extras Quality Assurance
Depends On:
Blocks: 446864
  Show dependency treegraph
Reported: 2008-05-28 11:19 EDT by John Dennis
Modified: 2008-05-28 18:30 EDT (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2008-05-28 18:30:54 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

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

  None (edit)
Description John Dennis 2008-05-28 11:19:13 EDT
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
--enable-languages=c,c++,objc,obj-c++,java,fortran,ada --enable-java-awt=gtk
--disable-dssi --enable-plugin
--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 11:20:47 EDT
Created attachment 306938 [details]
original C code
Comment 2 John Dennis 2008-05-28 11:21:28 EDT
Created attachment 306939 [details]
preprocessed C code
Comment 3 John Dennis 2008-05-28 11:21:55 EDT
Created attachment 306940 [details]
generated object code
Comment 4 John Dennis 2008-05-28 11:22:38 EDT
Created attachment 306941 [details]
disassembled machine code with annotations
Comment 5 Jakub Jelinek 2008-05-28 16:45:01 EDT
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 17:18:14 EDT
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:

  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.