Bugzilla – Bug 1868
Optimized builds are sensitive to -fstrict-overflow
Last modified: 2016-09-13 19:32:22 EDT
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.
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
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.
Patch r10636 111ac53de0e7 http://code.nsnam.org/ns-3-dev/rev/111ac53de0e7
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
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.
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.
I've finally bisected it down to r10139 17a71cd49da3. This is a big patch for 802.11n.
(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)
Created attachment 1817 [details] Patch to compile with -Wstrict-overflow=5 around rev 10139
(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
Created attachment 1818 [details] (revised) Patch to compile with -Wstrict-overflow=5 around rev 10139
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.
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.
Created attachment 1832 [details] patch to compile around ns-3-dev tip
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']
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']
(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.
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',
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
changeset 10769 2a65963e27ac
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
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.
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);
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
One thing for Vedran to try is: more = (low.GetLow () > 0) && (places < 20);
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.
(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).
(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.
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.
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.
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)?
(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
>> 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)?
(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) ^
Closing this issue-- discussion moved to bug 2499