Project

General

Profile

Actions

Bug #1135

closed

2.0rc2 release doesn't set optimization flag on GCC

Added by Ken Steele almost 11 years ago. Updated almost 11 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Target version:
Affected Versions:
Effort:
Difficulty:
Label:

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.

Actions #1

Updated by Ken Steele almost 11 years ago

I tested on both x86 and Tile, both don't add -O2 to CFLAGS.

Actions #2

Updated by Victor Julien almost 11 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. 

Actions #3

Updated by Victor Julien almost 11 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

Actions #4

Updated by Ken Steele almost 11 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.

Actions #5

Updated by Victor Julien almost 11 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.

Actions #6

Updated by Ken Steele almost 11 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.

Actions #7

Updated by Victor Julien almost 11 years ago

To create a release I do a 'make distcheck' which produces the source tarball with configure etc.

Actions #8

Updated by Ken Steele almost 11 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?

Actions #9

Updated by Victor Julien almost 11 years ago

Correct. I run autogen.sh and configure (w/o args). Then make distcheck.

Actions #10

Updated by Ken Steele almost 11 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?

Actions #11

Updated by Ken Steele almost 11 years ago

CFLAGS is also missing -g.

Actions #12

Updated by Victor Julien almost 11 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.

Actions #13

Updated by Ken Steele almost 11 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.

Actions #14

Updated by Ken Steele almost 11 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?

Actions #15

Updated by Victor Julien almost 11 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])

Actions #16

Updated by Ken Steele almost 11 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.

Actions #17

Updated by Ken Steele almost 11 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.

Actions #18

Updated by Victor Julien almost 11 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 :)

Actions

Also available in: Atom PDF