Bug #1135
closed2.0rc2 release doesn't set optimization flag on GCC
Description
./configure on 2.0rc2 generates a Makefile that sets CFLAGS without and -O flag. GCC defaults to -O0, so no optimization is done.
On the Git master, -O2 is included in CFLAGS.
Updated by Ken Steele over 10 years ago
I tested on both x86 and Tile, both don't add -O2 to CFLAGS.
Updated by Victor Julien over 10 years ago
Interesting, I had always assumed -O2 to be the gcc default. But you are right, it's not: http://gcc.gnu.org/onlinedocs/gcc-4.8.2/gcc/Optimize-Options.html#Optimize-Options
-O0 Reduce compilation time and make debugging produce the expected results. This is the default.
Updated by Victor Julien over 10 years ago
- Priority changed from High to Normal
- Target version changed from 2.0rc3 to 2.0.1rc1
It's not a release critical issue, so we can address this in 2.0.1
Updated by Ken Steele over 10 years ago
Respectfully, I disagree that this is not release critical. Compiling without any optimization is going to seriously hurt performance and thus likely limit people moving from previous releases. The git master uses -O2 by default. We need to at least document that you need to build with:
CFLAGS="-O2" ./configure
I would prefer that GCC defaulted to -O2, or some level of optimization, as many people assume that it does.
Updated by Victor Julien over 10 years ago
I agree the flag is important, however at this point in the RC cycle I consider really only crashes and glaring detection holes 'critical'. That said, this may not hurt to add, as much of our testing is done on these levels as well. If you prepare a minimalistic patch I can push it through QA to see if anything breaks.
Updated by Ken Steele over 10 years ago
What is done in the release process that is different from building from git master? I wasn't sure where the flag got lost.
Updated by Victor Julien over 10 years ago
To create a release I do a 'make distcheck' which produces the source tarball with configure etc.
Updated by Ken Steele over 10 years ago
But before the Makefile even exists, autogen and configure need to be run. What is the process for doing that before "make distcheck" to build a release?
Updated by Victor Julien over 10 years ago
Correct. I run autogen.sh and configure (w/o args). Then make distcheck.
Updated by Ken Steele over 10 years ago
When I run (on git master):
./autogen
./configure
make distcheck
cd tmp
tar xf ../suricata-2.0dev.tar.gz
./configure
The Makefile created does contain CFLAGS with -O2. So, I can't figure out how rc2 ended up with CFLAGS without -O2. Is it possible that the CFLAGS environment variable in the shell that created RC2 had CFLAGS defined without -O2?
Updated by Victor Julien over 10 years ago
- Status changed from New to Assigned
- Assignee set to Ken Steele
The bundled libhtp does explicitly set -O2, maybe that somehow ends up in that Makefile.
I just did a test here, and -O2 and -g are indeed not set by default. This makes sense if you look at configure.ac. It doesn't set them.
Updated by Ken Steele over 10 years ago
What I don't understand is why git master adds "-g -O2" to CFLAGS, but the release version doesn't.
There is a comment at the top of configure.ac about finding a better place for default CFLAGS.
Updated by Ken Steele over 10 years ago
In configure, I see these lines, which appear to add "-g -O2" to CFLAGS if CFLAGS was not set. I'm not sure how this gets into configure form configure.ac.
if test "$ac_test_CFLAGS" = set; then
CFLAGS=$ac_save_CFLAGS
elif test $ac_cv_prog_cc_g = yes; then
if test "$GCC" = yes; then
CFLAGS="-g -O2"
else
CFLAGS="-g"
fi
else
if test "$GCC" = yes; then
CFLAGS="-O2"
else
CFLAGS=
fi
fi
Looking at the CFLAGS in the two Makefiles, first from 2.0RC2:
CFLAGS = -DRELEASE -Wextra -Werror-implicit-function-declaration -fno-tree-pre -Wall -Wno-unused-parameter -std=gnu\
99 -march=native -DHAVE_LIBNET11 -D_BSD_SOURCE -D__BSD_SOURCE -D__FAVOR_BSD -DHAVE_NET_ETHERNET_H -I/usr/include -D\
LIBPCAP_VERSION_MAJOR=1 -DHAVE_PCAP_SET_BUFF -DHAVE_LIBCAP_NG
Then git master:
CFLAGS = -g -O2 -Wextra -Werror-implicit-function-declaration -fno-tree-pre -Wall -Wno-unused-parameter -std=gnu99 -\
march=native -DHAVE_LIBNET11 -D_BSD_SOURCE -D__BSD_SOURCE -D__FAVOR_BSD -DHAVE_NET_ETHERNET_H -I/usr/include -DLIBP\
CAP_VERSION_MAJOR=1 -DHAVE_PCAP_SET_BUFF -DHAVE_LIBCAP_NG -DREVISION="e04b5f0"
Is is possible the CFLAGS is set to "-DRELEASE" when building a release, which thus doesn't cause configure to add "-g -O2" to cflags?
Updated by Victor Julien over 10 years ago
Interesting, my release script indeed adds that by running:
sed -i "s/AC_INIT(suricata, 2.0dev)/AC_INIT(suricata, ${VERSION})\\n CFLAGS=\"\${CFLAGS} -DRELEASE\"/g" configure.ac
So it adds CFLAGS to the top of the script. Maybe it should get added to CFLAGS later.
Or perhaps it would be better to add it as an AC_DEFINE, e.g.:
AC_DEFINE([RELEASE], [1], [Official release])
Updated by Ken Steele over 10 years ago
- Assignee changed from Ken Steele to Victor Julien
When I run "make dist", the resulting tarball does not have "-DRELEASE" in CFLAGS, so I think there is something outside of the make dist target that is setting CFLAGS="-DRELEASE" when creating an official release. I don't have access to those scripts, so would someone please check them.
If that is the case, maybe we should create a new variable for adding DRELEASE to releases.
PS - Yes, I think a new variable is the right solution.
Updated by Ken Steele over 10 years ago
Or, have a new OFFICIAL_RELEASE environment variable that gets added to CFLAGS later. Then no sed of the script is required, but it then ends up in git master.
Updated by Victor Julien over 10 years ago
- Status changed from Assigned to Closed
- Target version changed from 2.0.1rc1 to 2.0rc3
I have updated my release script. Thanks for the thorough investigation Ken! I would never have guessed adding a CFLAGS had this effect :)