Bug 1868 - Optimized builds are sensitive to -fstrict-overflow
Optimized builds are sensitive to -fstrict-overflow
Status: RESOLVED MOVED
Product: ns-3
Classification: Unclassified
Component: general
ns-3-dev
All All
: P5 normal
Assigned To: Peter Barnes
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-03-02 03:11 EST by Peter Barnes
Modified: 2016-09-13 19:32 EDT (History)
4 users (show)

See Also:


Attachments
Patch to compile with -Wstrict-overflow=5 around rev 10139 (15.09 KB, patch)
2014-04-15 14:42 EDT, Peter Barnes
Details | Diff
(revised) Patch to compile with -Wstrict-overflow=5 around rev 10139 (15.55 KB, patch)
2014-04-15 15:33 EDT, Peter Barnes
Details | Diff
patch to compile around ns-3-dev tip (13.24 KB, patch)
2014-04-28 23:36 EDT, Tom Henderson
Details | Diff
testcase that fails to build with -Werror -Wstrict-overflow=5 (217 bytes, application/binary)
2014-05-29 01:12 EDT, Tom Henderson
Details
patch for gcc-4.9.1 (5.76 KB, patch)
2014-10-02 14:12 EDT, Tom Henderson
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Barnes 2014-03-02 03:11:56 EST
Optimized builds on the fedora-32-20 build slave are sensitive to -fstrict-overflow issues.

This manifests as a test crash running src/mesh/examples/mesh.cc, but only in the optimized build profile.
Comment 1 Peter Barnes 2014-03-02 03:12:06 EST
This talk has a very good discussion of subtle optimization bugs:
Robert Seacord, "Dangerous Optimizations and the Loss of Causality"
https://securecoding.cert.org/confluence/download/attachments/111083784/Dangerous%20Optimizations%20-%20Complete%20Version.pdf
Comment 2 Peter Barnes 2014-03-02 03:40:59 EST
To discover the sensitive bits:

  $ CXXFLAGS="-fstrict-overflow -Wstrictoverflow=n" ./waf configure \
    --enable-examples --enable-tests --out=build/optimized

for n up to 4.  

Seacord's suggestion is to explicitly refactor the offending expression into the form the optimizer wants.

Fixing the warnings up to n = 3 was sufficient to fix the failing mesh example.

I also tried n = 5 but wasn't able to remove the warnings.
Comment 3 Peter Barnes 2014-03-02 04:05:41 EST
Patch r10636 111ac53de0e7
http://code.nsnam.org/ns-3-dev/rev/111ac53de0e7
Comment 4 Peter Barnes 2014-03-05 14:39:40 EST
That should be

  $ CXXFLAGS="-fstrict-overflow -Wstrict-overflow=n" ./waf configure \
    --enable-examples --enable-tests --out=build/optimized

(In reply to Peter Barnes from comment #2)
> To discover the sensitive bits:
> 
>   $ CXXFLAGS="-fstrict-overflow -Wstrictoverflow=n" ./waf configure \
>     --enable-examples --enable-tests --out=build/optimized
Comment 5 Peter Barnes 2014-03-05 15:30:51 EST
Well, this needs more waf skill than I possess.

We need to add CXXFLAGS="-fstrict-overflow", but (so far) only in this specific case:

  optimized build
  gcc 4.8.2
  32-bit

If I'm reading the 3.19 release notes correctly, gcc-4.8.1 and below, 32- and 64-bit don't need this.  AFAIK we don't have a gcc-4.82 64-bit system, but it would be good to check this case.

I think the big question is do we add this option to *all* optimized builds?  (Meaning all gcc versions?)  I suppose the criteria should be:

  ns-3 still compiles
  all tests pass
  no significant performance impact 

There's also the question of other compiler support.  I can't tell if we actually have a clang buildbot (Mac OS X.9?).  It looks like we don't' have any other compilers being tested.
Comment 6 Tom Henderson 2014-03-10 00:21:17 EDT
I have a Fedora Core 20 64-bit buildslave ready to bring on line shortly.  I'll also enable the FreeBSD 9 buildslave once I get the Java support sorted out there.

I guess that the next step is to test clang systems and 64-bit gcc-4.8.2 to see the scope of necessary support for this type of patch.
Comment 7 Peter Barnes 2014-04-15 14:29:19 EDT
I've finally bisected it down to r10139 17a71cd49da3.  This is a big patch for 802.11n.
Comment 8 Peter Barnes 2014-04-15 14:29:58 EDT
(In reply to Peter Barnes from comment #2)
> I also tried n = 5 but wasn't able to remove the warnings.

I now have a patch that fixes all warnings up to n = 5 (the highest level)
Comment 9 Peter Barnes 2014-04-15 14:42:57 EDT
Created attachment 1817 [details]
Patch to compile with -Wstrict-overflow=5 around rev 10139
Comment 10 Peter Barnes 2014-04-15 14:43:41 EDT
(In reply to Peter Barnes from comment #7)
> I've finally bisected it down to r10139 17a71cd49da3.  This is a big patch
> for 802.11n.

Process for bisecting fedora-20:

# Apply -fstrict-overflow patch
$ hg import --no-commit patch-strict.diff 

# Configure, build, test:
$ cd $(hg root) && ./waf configure $NS3CONFIG $NS3OPT
$ cd $(hg root) && ./waf build --keep
$ cd $(hg root) && ./test.py

# Undo all edits to clean up
$ hg revert --all

# Report success or failure
$ hg bisect --bad | --good
Comment 11 Peter Barnes 2014-04-15 15:33:17 EDT
Created attachment 1818 [details]
(revised) Patch to compile with -Wstrict-overflow=5 around rev 10139
Comment 12 Peter Barnes 2014-04-15 16:06:06 EDT
Tom wrote:
> If I understand correctly, the status on this now is:
> 
> 1) your most recent patch can clear all of these errors on Fedora Core 20 if 
> CXXFLAGS="-fstrict-overflow" is added and you revert back to before r10139
> 
> 2) r10139 introduces an additional bug that your patch does not correct and 
> needs to be looked into separately
> 
> 3) the issue of exactly how we should modify CXXFLAGS in waf (limiting its 
> scope) is still open
> 
> If not, can you please correct the above?

1)  Correct

2)  Correct

3)  Correct, or alternatively:

4)  We could just apply the full patch (not posted), which fixes all -Wstrict errors, and should be transparent to other systems.
Comment 13 Peter Barnes 2014-04-15 16:18:49 EDT
A difficulty in bisecting (and dissecting r10139) is that we have many broken commits which are fixed later.

So some issues with r10139 may already be fixed.

Here are the commits since then that touch the same files:

10652   [doxygen] Revert r10410, r10411, r10412
10618   [wifi] reformat wifi-tx-vector.cc
10607   [wifi] moved IsModeSupported to WifiPhy
10600   Bug 1848 - yans-wifi-phy can receive frames sent using unsupported
        mode
10580   Update (most of) IEEE Std 802.11-2007 to IEEE Std 802.11-2012
10553   Move tests outside ns3 namespace
10507   [doxygen] wifi module
10483   [doxygen] wifi module
10459   base implementation of the IEEE 802.11p standard
10424   doxygen warnings for wifi module
10410   doxygen] Suppress "warning: Member NS_OBJECT_ENSURE_REGISTERED is
        not documented"
10402   Bug 1799 - Wrong ERP-OFDM PLCP preamble and header values
10218   Fix compilation with Clang 3.2 and newer versions, including Apple
        Clang 5.0

        Clang 3.2 requires all class members to be used in the code; this
        patch either removes class members that are not used or adds
        NS_UNUSED/NS_UNUSED_GLOBAL around them. Thanks to Andrey Mazo and
        Tommaso Pecorella for review.
10216   Bug 1691 - RTS/CTS NAV reset prematurely
10207   Fixed indentation in wifi-remote-station-manager.cc
10206   Bug 1722 - beacons transmitted concurrently
10204   bug 945: remove deprecated 802.11p code
10203   coding style: avoid NULL
10202   remove unimplemented functions
10201   avoid crash when packets sent by higher layer cause no RTS/CTS
        operation
10195   make wifi tests more robust to random variable perturbations
10179   bug 1584: Association Request Timeouts not canceled. - Fixed copy-
        paste error in 831ad8abd1c0.
10178   bug 1584: Association Request Timeouts not canceled (reported by
        Pedro Fortuna, extended by Tommaso Pecorella)
10157   Link to bug num in bug database with \bugid{num}
10144   Remove unused wifiMode variable, fix build
10140   remove unimplemented declaration

About half of these should be irrelevant (doxygen, formatting), but the others are significant.
Comment 14 Tom Henderson 2014-04-28 23:36:28 EDT
Created attachment 1832 [details]
patch to compile around ns-3-dev tip
Comment 15 Tom Henderson 2014-04-29 00:21:11 EDT
I created this patch by starting with the r10139 patch and doing similar things as needed to remaining issues with ns-3-dev tip.

It seems to clear the issues for gcc but I hit an issue tonight when testing with clang-3.3.

To reproduce:  FreeBSD-9.2, 64-bit buildslave

CXX=clang ./waf configure -d optimized --enable-examples --enable-tests --disable-python
CXX=clang ./waf build

eventually fails with:
Assertion failed: ((isUIntN(8 * Size, Value) || isIntN(8 * Size, Value)) && "Invalid size"), function EmitIntValue, file /usr/src/lib/clang/libllvmmc/../../../contrib/llvm/lib/MC/MCStreamer.cpp, line 93.
Stack dump:
0.	Program arguments: /usr/bin/clang -cc1 -triple x86_64-unknown-freebsd9.2 -emit-obj -disable-free -main-file-name lte-enb-rrc.cc -mrelocation-model pic -pic-level 2 -mdisable-fp-elim -masm-verbose -mconstructor-aliases -munwind-tables -target-cpu corei7 -g -coverage-file /home/buildslave/scratch/ns-3-dev/src/lte/model/lte-enb-rrc.cc.1.o -resource-dir /usr/bin/../lib/clang/3.3 -D HAVE_SYS_IOCTL_H=1 -D HAVE_IF_NETS_H=1 -I . -I .. -O3 -Wall -Werror -Wstrict-overflow=5 -Wno-error=deprecated-declarations -Wstrict-aliasing -fdeprecated-macro -fdebug-compilation-dir /usr/home/buildslave/scratch/ns-3-dev/build -ferror-limit 19 -fmessage-length 0 -pthread -mstackrealign -fobjc-runtime=gnustep -fobjc-default-synthesize-properties -fcxx-exceptions -fexceptions -fdiagnostics-show-option -backend-option -vectorize-loops -o src/lte/model/lte-enb-rrc.cc.1.o -x c++ ../src/lte/model/lte-enb-rrc.cc 
1.	<eof> parser at end of file
2.	Code generation
clang: error: unable to execute command: Abort trap: 6 (core dumped)
clang: error: clang frontend command failed due to signal (use -v to see invocation)
FreeBSD clang version 3.3 (tags/RELEASE_33/final 183502) 20130610
Target: x86_64-unknown-freebsd9.2
Thread model: posix
clang: note: diagnostic msg: PLEASE submit a bug report to http://llvm.org/bugs/ and include the crash backtrace, preprocessed source, and associated run script.
clang: note: diagnostic msg: 
********************

PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
Preprocessed source(s) and associated run script(s) are located at:
clang: note: diagnostic msg: /tmp/lte-enb-rrc-egHUiZ.cpp
clang: note: diagnostic msg: /tmp/lte-enb-rrc-egHUiZ.sh
clang: note: diagnostic msg: 

********************
Waf: Leaving directory `/usr/home/buildslave/scratch/ns-3-dev/build'
Build failed
 -> task in 'ns3-lte' failed (exit status 254): 
	{task 34459041872: cxx lte-enb-rrc.cc -> lte-enb-rrc.cc.1.o}
['clang', '-O3', '-g', '-Wall', '-Werror', '-march=native', '-fstrict-overflow', '-Wstrict-overflow=5', '-Wno-error=deprecated-declarations', '-fstrict-aliasing', '-Wstrict-aliasing', '-fPIC', '-pthread', '-I.', '-I..', '-DHAVE_SYS_IOCTL_H=1', '-DHAVE_IF_NETS_H=1', '../src/lte/model/lte-enb-rrc.cc', '-c', '-o', 'src/lte/model/lte-enb-rrc.cc.1.o']
Comment 16 Peter Barnes 2014-04-29 09:39:33 EDT
I see -Wstrict-overflow=5 but no -fstrict-overflow in the clang command line.  (It's in the waf list, though.)  Does this matter?

(In reply to Tom Henderson from comment #15)
> It seems to clear the issues for gcc but I hit an issue tonight when testing
> with clang-3.3.
> 0.	Program arguments: /usr/bin/clang -cc1 -triple x86_64-unknown-freebsd9.2
> -emit-obj -disable-free -main-file-name lte-enb-rrc.cc -mrelocation-model
> pic -pic-level 2 -mdisable-fp-elim -masm-verbose -mconstructor-aliases
> -munwind-tables -target-cpu corei7 -g -coverage-file
> /home/buildslave/scratch/ns-3-dev/src/lte/model/lte-enb-rrc.cc.1.o
> -resource-dir /usr/bin/../lib/clang/3.3 -D HAVE_SYS_IOCTL_H=1 -D
> HAVE_IF_NETS_H=1 -I . -I .. -O3 -Wall -Werror -Wstrict-overflow=5
> -Wno-error=deprecated-declarations -Wstrict-aliasing -fdeprecated-macro
> -fdebug-compilation-dir /usr/home/buildslave/scratch/ns-3-dev/build
> -ferror-limit 19 -fmessage-length 0 -pthread -mstackrealign
> -fobjc-runtime=gnustep -fobjc-default-synthesize-properties -fcxx-exceptions
> -fexceptions -fdiagnostics-show-option -backend-option -vectorize-loops -o
> src/lte/model/lte-enb-rrc.cc.1.o -x c++ ../src/lte/model/lte-enb-rrc.cc 

> Waf: Leaving directory `/usr/home/buildslave/scratch/ns-3-dev/build'
> Build failed
>  -> task in 'ns3-lte' failed (exit status 254): 
> 	{task 34459041872: cxx lte-enb-rrc.cc -> lte-enb-rrc.cc.1.o}
> ['clang', '-O3', '-g', '-Wall', '-Werror', '-march=native',
> '-fstrict-overflow', '-Wstrict-overflow=5',
> '-Wno-error=deprecated-declarations', '-fstrict-aliasing',
> '-Wstrict-aliasing', '-fPIC', '-pthread', '-I.', '-I..',
> '-DHAVE_SYS_IOCTL_H=1', '-DHAVE_IF_NETS_H=1',
> '../src/lte/model/lte-enb-rrc.cc', '-c', '-o',
> 'src/lte/model/lte-enb-rrc.cc.1.o']
Comment 17 Tom Henderson 2014-04-29 13:23:07 EDT
(In reply to Peter Barnes from comment #16)
> I see -Wstrict-overflow=5 but no -fstrict-overflow in the clang command
> line.  (It's in the waf list, though.)  Does this matter?

It is in there:

> > ['clang', '-O3', '-g', '-Wall', '-Werror', '-march=native',
> > '-fstrict-overflow', '-Wstrict-overflow=5',
> > '-Wno-error=deprecated-declarations', '-fstrict-aliasing',
> > '-Wstrict-aliasing', '-fPIC', '-pthread', '-I.', '-I..',
> > '-DHAVE_SYS_IOCTL_H=1', '-DHAVE_IF_NETS_H=1',
> > '../src/lte/model/lte-enb-rrc.cc', '-c', '-o',
> > 'src/lte/model/lte-enb-rrc.cc.1.o']

But hold on, I just reverted the patch and tried on the current ns-3-dev, and got a similar error:

 -> task in 'ns3-lte' failed (exit status 254): 
	{task 34463418384: cxx lte-enb-rrc.cc -> lte-enb-rrc.cc.1.o}
['clang', '-O3', '-g', '-Wall', '-Werror', '-march=native', '-Wno-error=deprecated-declarations', '-fstrict-aliasing', '-Wstrict-aliasing', '-fPIC', '-pthread', '-I.', '-I..', '-DHAVE_SYS_IOCTL_H=1', '-DHAVE_IF_NETS_H=1', '../src/lte/model/lte-enb-rrc.cc', '-c', '-o', 'src/lte/model/lte-enb-rrc.cc.1.o']


So this problem may lie outside of the strict aliasing patch.
Comment 18 Peter Barnes 2014-04-29 13:33:07 EDT
It's in the waf-reported command line options, but not reported by clang.

(In reply to Tom Henderson from comment #17)
> (In reply to Peter Barnes from comment #16)
> > I see -Wstrict-overflow=5 but no -fstrict-overflow in the clang command
> > line.  (It's in the waf list, though.)  Does this matter?
> 
> It is in there:
> 
> > > ['clang', '-O3', '-g', '-Wall', '-Werror', '-march=native',
> > > '-fstrict-overflow', '-Wstrict-overflow=5',
Comment 19 Peter Barnes 2014-05-01 00:37:02 EDT
Modified patch applied, which fixes all warnings at -Wstrict-overflow=5 on fedora-32-20.

r10769 2a65963e27ac
http://code.nsnam.org/ns-3-dev/rev/2a65963e27ac
Comment 20 Tommaso Pecorella 2014-05-02 17:49:21 EDT
changeset 10769	2a65963e27ac
Comment 21 Tom Henderson 2014-05-29 01:12:37 EDT
Created attachment 1842 [details]
testcase that fails to build with -Werror -Wstrict-overflow=5

It turns out that -Wstrict-overflow is causing problems for older compilers, such as this testcase which fails on Centos 6.4 and Ubuntu 12.04.  In ns-3-dev, this manifests itself in simple conditional tests such as
if (mylist.size() > 0)

[buildslave@localhost ~]$ g++ -v
Using built-in specs.
Target: x86_64-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-gnu-unique-object --enable-languages=c,c++,objc,obj-c++,java,fortran,ada --enable-java-awt=gtk --disable-dssi --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-ppl --with-cloog --with-tune=generic --with-arch_32=i686 --build=x86_64-redhat-linux
Thread model: posix
gcc version 4.4.7 20120313 (Red Hat 4.4.7-4) (GCC)

[buildslave@localhost ~]$ g++ -O3 -g -Wall -Werror -fstrict-overflow -Wstrict-overflow=5 testcase.cc
cc1plus: warnings being treated as errors
testcase.cc: In function ‘int main()’:
testcase.cc:9: error: assuming signed overflow does not occur when simplifying conditional to constant
Comment 22 Tom Henderson 2014-05-29 02:09:08 EDT
changeset 10790:b445436fd51f restricts the warning to gcc-4.8.2 or higher.  since we have FC 19 and 20 buildslaves checking this to find legitimate overflow issues, there is no need to enable it on older compilers that may produce noise.

This can be closed again shortly if no other issues found in the ns-3.20 release process.
Comment 23 Tom Henderson 2014-06-04 17:32:35 EDT
Based on Vedran's reported problems with gcc-4.9, I would propose to remove -Wstrict-overflow=5 in wscript and handle this for the moment with CXXFLAGS in our build system.

Any comment on this proposal, or patch to address this issue?

../src/core/model/int64x64.cc: In function ‘std::ostream&
ns3::operator<<(std::ostream&, const ns3::int64x64_t&)’:
../src/core/model/int64x64.cc:87:25: error: assuming signed overflow
does not occur when changing X +- C1 cmp C2 to X cmp C1 +- C2
[-Werror=strict-overflow]
    more = low.GetLow () && (places < 20);
Comment 24 Peter Barnes 2014-06-04 17:46:04 EDT
Vedran's note to the list:

Date: Tue, 3 Jun 2014 10:12:46 +0200
From: Vedran Mileti? <rivanvx@gmail.com>
Subject: Re: [Ns-developers] ns-3.20 release candidate 1 is posted
To: Tom Henderson <tomh@tomh.org>
Cc: ns-developers list <ns-developers@isi.edu>
Message-ID:
	<CA+oUA20-PKnmPAVzV1cQ+BCbg5R3GVn0dC_m-mW2K9VVPScXdw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8

Hi,

I still experience issues with optimized build, namely:

../src/core/model/int64x64.cc: In function ?std::ostream&
ns3::operator<<(std::ostream&, const ns3::int64x64_t&)?:
../src/core/model/int64x64.cc:87:25: error: assuming signed overflow
does not occur when changing X +- C1 cmp C2 to X cmp C1 +- C2
[-Werror=strict-overflow]
   more = low.GetLow () && (places < 20);
                        ^
cc1plus: all warnings being treated as errors

This on our lab test machine with Debian sid amd64 and

g++ (Debian 4.9.0-5) 4.9.0
Copyright (C) 2014 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

I see that strict-overflow warning is disabled for g++ below 4.8.2, so
4.9 should probably support it. Is there anything particular I should try?

Regards,
Vedran
Comment 25 Peter Barnes 2014-06-04 17:51:44 EDT
One thing for Vedran to try is:

    more = (low.GetLow () > 0) && (places < 20);
Comment 26 Peter Barnes 2014-06-04 17:58:31 EDT
By "handle ... in our build system" I assume you mean in our build slaves.

I don't like this approach because it means users will hit the same issue on their own, with the newer gcc versions.  This seems silly when we have a patch (for 4.8.2, at least).

(In reply to Tom Henderson from comment #23)
> Based on Vedran's reported problems with gcc-4.9, I would propose to remove
> -Wstrict-overflow=5 in wscript and handle this for the moment with CXXFLAGS
> in our build system.
Comment 27 Tom Henderson 2014-06-04 18:04:09 EDT
(In reply to Peter Barnes from comment #26)
> By "handle ... in our build system" I assume you mean in our build slaves.

Yes, we have one buildslave that will use custom CXXFLAGS to enable Wstrict-overflow.

> 
> I don't like this approach because it means users will hit the same issue on
> their own, with the newer gcc versions.  This seems silly when we have a
> patch (for 4.8.2, at least).

I was thinking that if the warning was not on by default, users would not hit the issue.

On the other hand, we could simply try to fix now for gcc-4.9 if it is easily remedied (note, gcc-4.9 is not even a supported compiler for ns-3.20 release, but we know it will be coming later this year).
Comment 28 Tom Henderson 2014-06-05 15:49:50 EDT
(In reply to Peter Barnes from comment #25)
> One thing for Vedran to try is:
> 
>     more = (low.GetLow () > 0) && (places < 20);


I created a Debian sid VM to look at this.  The above suggestion didn't resolve it, however:

../src/core/model/int64x64.cc: In function ‘std::ostream& ns3::operator<<(std::ostream&, const ns3::int64x64_t&)’:
../src/core/model/int64x64.cc:87:31: error: assuming signed overflow does not occur when changing X +- C1 cmp C2 to X cmp C1 +- C2 [-Werror=strict-overflow]
    more = (low.GetLow () > 0) && (places < 20);
                               ^
cc1plus: all warnings being treated as errors

------

In general, there are several new issues cropping up with this warning under gcc-4.9; I haven't been able to complete an optimized build yet, despite stubbing some of these out.

I'll try to produce a list of issues, and make the VM available on our build farm.
Comment 29 Tom Henderson 2014-06-07 12:03:17 EDT
ns-3.20 status:  In wscript, restricted usage of Wstrict-overflow to gcc-4.8.2.  Will stand up a gcc-4.9 buildslave to fix those discovered issues.
Comment 30 Tom Henderson 2014-10-02 14:12:21 EDT
Created attachment 1889 [details]
patch for gcc-4.9.1

This patch fixes the additional issues discovered with gcc-4.9.1 (tested on Debian sid 32-bit machine).

It also relaxes the constraint for applying -Wstrict-overflow=5.  In ns-3.20, we limited this to gcc-4.8.2 only.  The wscript is modified such that this warning is in effect for gcc >= 4.8.2.
Comment 31 Peter Barnes 2014-10-15 18:28:23 EDT
I don't understand several of changes here:

src/core/model/int64x64.cc:
  os.precision() returns std::streamsize (which is *signed* in gcc-4.2.1)
  while std::size_t is unsigned.
  
  Changing places to streamsize doesn't fix the warning?
  
src/fd-net-device/model/fd-net-device.cc:
    ssize_t is signed, size_t is unsigned
    
    Change these signatures to match?
    FdNetDevice::ReceiveCallback (uint8_t *buf, ssize_t len);
    FdNetDevice::ForwardUp (uint8_t *buf, ssize_t len);
    RemovePIHeader
    
    ssize_t also widely used elsewhere in:
	src/core/model/system-*
    	src/core/model/unix-*
    	src/fd-net-device/*
    	src/network/model/socket.h
    	src/tap-bridge/model/*

    Should we change these as well?

src/lte/model/lte-ue-phy.cc:
	760:  for (uint32_t i = 0; i < nbSubChannels; i++)
		nbSubChannels is std::size_t as it should be.
		Shouldn't i have the same type?
		
src/stats/model/basic-data-calculators.h:
	MinMaxAvgTotalCalculator<T>::Update (const T i)
	l167:  if (i - m_min < 0)
	
	If T is unsigned, how does this work? 
	Why was gcc complaining in the first place about if (i < m_min)?
Comment 32 Tom Henderson 2014-10-15 19:35:34 EDT
(In reply to Peter Barnes from comment #31)
> I don't understand several of changes here:
> 
> src/core/model/int64x64.cc:
>   os.precision() returns std::streamsize (which is *signed* in gcc-4.2.1)
>   while std::size_t is unsigned.

I changed to unsigned because of the docmentation on this that I read:  "Except in the constructors of std::strstreambuf, negative values of std::streamsize are never used." so I tried to leverage that to avoid signed arithmetic.

>   
>   Changing places to streamsize doesn't fix the warning?

No, it still results in:

../src/core/model/int64x64.cc: In function ‘std::ostream& ns3::operator<<(std::ostream&, const ns3::int64x64_t&)’:
../src/core/model/int64x64.cc:88:25: error: assuming signed overflow does not occur when changing X +- C1 cmp C2 to X cmp C1 +- C2 [-Werror=strict-overflow]
    more = low.GetLow () && (places < 20);
                         ^
cc1plus: all warnings being treated as errors

>   
> src/fd-net-device/model/fd-net-device.cc:
>     ssize_t is signed, size_t is unsigned

I couldn't tell why ssize_t was being used in AddPIHeader; it seemed to unnecessarily make it signed.

>     
>     Change these signatures to match?
>     FdNetDevice::ReceiveCallback (uint8_t *buf, ssize_t len);
>     FdNetDevice::ForwardUp (uint8_t *buf, ssize_t len);
>     RemovePIHeader
>     
>     ssize_t also widely used elsewhere in:
> 	src/core/model/system-*
>     	src/core/model/unix-*
>     	src/fd-net-device/*
>     	src/network/model/socket.h
>     	src/tap-bridge/model/*
> 
>     Should we change these as well?

Perhaps for good measure, but I wasn't trying to increase the scope of the patch.  

> 
> src/lte/model/lte-ue-phy.cc:
> 	760:  for (uint32_t i = 0; i < nbSubChannels; i++)
> 		nbSubChannels is std::size_t as it should be.
> 		Shouldn't i have the same type?

OK

> 		
> src/stats/model/basic-data-calculators.h:
> 	MinMaxAvgTotalCalculator<T>::Update (const T i)
> 	l167:  if (i - m_min < 0)
> 	
> 	If T is unsigned, how does this work? 

probably not very well; good catch (we probably should add an unsigned test on this class method)

> 	Why was gcc complaining in the first place about if (i < m_min)?

In file included from ../src/stats/test/basic-data-calculators-test-suite.cc:24:0:
./ns3/basic-data-calculators.h: In member function ‘virtual void FiveIntegersTestCase::DoRun()’:
./ns3/basic-data-calculators.h:163:11: error: assuming signed overflow does not occur when simplifying conditional to constant [-Werror=strict-overflow]
           if (i < m_min)
           ^
cc1plus: all warnings being treated as errors
Comment 33 Peter Barnes 2014-10-15 19:45:33 EDT
>> src/stats/model/basic-data-calculators.h:
>> 	MinMaxAvgTotalCalculator<T>::Update (const T i)
>> 	l167:  if (i - m_min < 0)
>> 	
>> 	If T is unsigned, how does this work? 
> 
> 
> probably not very well; good catch (we probably should add an unsigned test
> on this class method)

What I meant was, Why doesn't gcc complain that the test is always false with
T = unsigned?

>> 	Why was gcc complaining in the first place about if (i < m_min)?
> 
> 
> In file included from ../src/stats/test/basic-data-calculators-test-suite.cc:24:0:
> ./ns3/basic-data-calculators.h: In member function ‘virtual void FiveIntegersTestCase::DoRun()’:
> ./ns3/basic-data-calculators.h:163:11: error: assuming signed overflow does not occur when simplifying conditional to constant [-Werror=strict-overflow]
>            if (i < m_min)
>            ^

In this case T = int.

Why doesn't (i - m_min < 0) work?  or (0 < m_min - i)?
Comment 34 Tom Henderson 2014-10-15 23:15:39 EDT
(In reply to Peter Barnes from comment #33)
> >> src/stats/model/basic-data-calculators.h:
> >> 	MinMaxAvgTotalCalculator<T>::Update (const T i)
> >> 	l167:  if (i - m_min < 0)
> >> 	
> >> 	If T is unsigned, how does this work? 
> > 
> > 
> > probably not very well; good catch (we probably should add an unsigned test
> > on this class method)
> 
> What I meant was, Why doesn't gcc complain that the test is always false with
> T = unsigned?

I found that the lack of warning is due to -Wextra not being enabled.  It can be enabled with:

diff -r 0114887f7644 wscript
--- a/wscript	Mon Sep 29 22:07:14 2014 +0100
+++ b/wscript	Wed Oct 15 19:57:04 2014 -0700
@@ -45,7 +45,7 @@
 
 cflags.profiles = {
 	# profile name: [optimization_level, warnings_level, debug_level]
-	'debug':     [0, 2, 3],
+	'debug':     [0, 3, 3],
 	'optimized': [3, 2, 1],
 	'release':   [3, 2, 0],
 	}

(which will trigger various other warnings elsewhere...)

> 
> >> 	Why was gcc complaining in the first place about if (i < m_min)?
> > 
> > 
> > In file included from ../src/stats/test/basic-data-calculators-test-suite.cc:24:0:
> > ./ns3/basic-data-calculators.h: In member function ‘virtual void FiveIntegersTestCase::DoRun()’:
> > ./ns3/basic-data-calculators.h:163:11: error: assuming signed overflow does not occur when simplifying conditional to constant [-Werror=strict-overflow]
> >            if (i < m_min)
> >            ^
> 
> In this case T = int.
> 
> Why doesn't (i - m_min < 0) work?  or (0 < m_min - i)?

Not sure; here is the output from (0 < m_min - i)

In file included from ../src/stats/test/basic-data-calculators-test-suite.cc:24:0:
./ns3/basic-data-calculators.h: In member function ‘virtual void FiveIntegersTestCase::DoRun()’:
./ns3/basic-data-calculators.h:163:11: error: assuming signed overflow does not occur when changing X +- C1 cmp C2 to X cmp C1 +- C2 [-Werror=strict-overflow]
           if (0 < m_min - i)
           ^
Comment 35 Tom Henderson 2016-09-13 19:32:22 EDT
Closing this issue-- discussion moved to bug 2499