Bug 2938

Summary:	change compiler flags for build profiles
Product:	ns-3	Reporter:	Tom Henderson <tomh>
Component:	build system	Assignee:	Gustavo J. A. M. Carneiro <gjcarneiro>
Status:	PATCH WANTED ---
Severity:	normal	CC:	ns-bugs, pdbarnes
Priority:	P3
Version:	ns-3.28
Hardware:	All
OS:	All
See Also:	https://www.nsnam.org/bugzilla/show_bug.cgi?id=2936
Attachments:	patch to change buildflags disable-werror patch

Description Tom Henderson 2018-06-24 14:23:43 UTC

Created attachment 3118 [details]
patch to change buildflags

Background previous discussion on these topics on ns-developers:

http://mailman.isi.edu/pipermail/ns-developers/2015-September/013128.html
http://mailman.isi.edu/pipermail/ns-developers/2015-July/012981.html

We have discussed in the past but not come to agreement or taken any action on changing some of our build configurations (debug, release, optimized).

However, we continue to have the problem that older releases do not build on newer compilers because we use -Werror everywhere.

Attached is a patch that makes the following changes.

1) Remove Werror from 'optimized' and 'release' builds

2) Add a new --disable-werror argument to Waf to allow users to optionally disable -Werror even from debug builds, and document this in our tutorial (start to better document to users how they can use old releases on newer compilers)

3) Peter pointed out in the past that the debugging options for optimized and release seemed backwards (optimized was using -g, and release -g0).  This patch reverses that (optimized now uses -g0 and release -g).

A second step (post release) will be to add 'Wextra' to debug build.  This will trigger new warnings, some of which relate to the Visual Studio port.

Comment 1 Peter Barnes 2018-06-29 14:22:32 UTC

+1, I guess.

1.  In wscript I would prefer to list the settings in this order:
  debug
  release
  optimized

This matches the hierarchy (if not the names) in waf-tools/cflags.py

Just to note, these new settings match cflags.py; we just change the names.  


2.  It looks like the flags come from two places; perhaps we should document that somewhere?  The two places are 
  waf-tools/cflags.py, by compiler (gcc, icc, msvc), starting at line 4
  wscript, also by compiler (gcov, gcc, icc, clang), starting at line 365

Note that the compilers lists are different.

3.  The documentation table is incomplete.  Grep for CXXFLAGS in wscript:
        env.append_value('CXXFLAGS', flag)
        env.append_value('CXXFLAGS', '-fprofile-arcs')
        env.append_value('CXXFLAGS', '-ftest-coverage')
            env.append_value('CXXFLAGS', '-fomit-frame-pointer') 
                env.append_value('CXXFLAGS', '-march=native') 
            env.append_value('CXXFLAGS', '-fstrict-overflow')
                env.append_value('CXXFLAGS', '-Wstrict-overflow=2')
                env.append_value('CXXFLAGS', '-Wno-unused-local-typedefs')
                env.append_value('CXXFLAGS', '-Wno-potentially-evaluated-expression')
                env.append_value('CXXFLAGS', '-Wno-unused-local-typedefs')
                env.append_value('CXXFLAGS', '-Wno-potentially-evaluated-expression')

4.  Oddities:

  optimized has '-fstrict-overflow'; shouldn't this be in debug?
  if '-fomit-frame-pointer' is needed shouldn't it be in all builds?
  Shouldn't '-Wstrict-overflow=2' be in debug?  It's only in gcc optimized
  
5.  The -Wno-... address a clang bug/feature; see our bug 2181.  Should we document this in the table?

Comment 2 Tom Henderson 2018-07-22 16:47:05 UTC

Created attachment 3149 [details]
disable-werror patch

Patch for the --disable-werror addition only

Comment 3 Tom Henderson 2018-07-22 17:20:51 UTC

I agree with Peter's observations that things could be cleaned up and aligned with the cflags settings and documentation.  I propose to add the --disable-werror option in this ns-3.29 release cycle (and also put out an ns-3.28.1 maintenance release including this patch) to better future-proof these releases, and return to a cleanup of cflags and wscript (and documentation) as a second step.

Comment 4 Tom Henderson 2018-08-05 20:26:18 UTC

The --disable-werror patch was committed in changeset 13719:d0280d83412e.  I'm leaving this open in PATCH WANTED state now to complete other cleanup suggested by Peter.