Bug 2499 - resolving -Wstrict-overflow warnings
resolving -Wstrict-overflow warnings
Status: CONFIRMED
Product: ns-3
Classification: Unclassified
Component: general
unspecified
All All
: P5 normal
Assigned To: ns-bugs
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-09-09 16:17 EDT by Tom Henderson
Modified: 2016-09-13 19:35 EDT (History)
2 users (show)

See Also:


Attachments
warning list at level 5 (290.54 KB, text/plain)
2016-09-09 16:21 EDT, Tom Henderson
Details
warning list at level 4 (348.71 KB, text/plain)
2016-09-09 16:21 EDT, Tom Henderson
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Henderson 2016-09-09 16:17:06 EDT
a recent changeset enabed -Wstrict-overflow=5 for all gcc versions >= 4.8.2.  Previously, we had patched the codebase for all such warnings on gcc-4.8.2 (fedora core 20):
http://code.nsnam.org/ns-3-dev/rev/2a65963e27ac

We had set up a FC 20 buildslave to keep track of this, and disabled checking for gcc > 4.8.2 (since it was observed at the time that different compiler versions were detecting new warning issues).  That is, checking was done only on gcc version == 4.8.2 (on a FC 20 buildslave).

However, we are well past FC 20 now and are not checking gcc-4.8.2 any more.

-Wstrict-overflow warning levels are documented here; there are 5 levels, with level 5 being the most strict.

https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html

For gcc-5.3.1, I am observing 473 current warnings at level 5, 408 at level 4, 407 at level 3, and none at level 2.  Most of these warnings are not due to changes in our codebase, but changes to gcc to emit more warnings.

For the immediate release, we could consider to disable this (revert the level back to 2).
Comment 1 Tom Henderson 2016-09-09 16:21:05 EDT
Created attachment 2572 [details]
warning list at level 5
Comment 2 Tom Henderson 2016-09-09 16:21:30 EDT
Created attachment 2573 [details]
warning list at level 4
Comment 3 Peter Barnes 2016-09-09 20:01:28 EDT
The list of errors is certainly daunting.  A few even seem intractable.

For example, look at the error reported in src/core/examples/hash-example.cc.  
The error is
    ../src/core/examples/hash-example.cc: In member function ‘bool 
ns3::Hash::Example::DictFiles::Add(std::string)’:
    ../src/core/examples/hash-example.cc:465:8: error: assuming signed overflow 
does not occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 
[-Werror=strict-overflow]
       bool Add (const std::string file)
        ^

The Add function is:

      bool Add (const std::string file)
      {
        if (std::find (m_files.begin (), m_files.end (), file) == m_files.end 
())
          {
            m_files.push_back (file);
          }
        
        return true;
      }

m_files is a std::vector<std::string>.

Where is the arithmetic in a comparison?  I suspect it's in std::find.  How am 
I supposed to fix that?

A similar case, utils/print-introspected-doxygen.cc:

    template <typename T>
    void
    Uniquefy (T t)
    {
      std::sort (t.begin (), t.end ());
      t.erase (std::unique (t.begin (), t.end ()), t.end ());
    }

Both std::sort and std::unique use comparisons, but I can't fix them.
Comment 4 Peter Barnes 2016-09-09 20:15:23 EDT
This SO response is informative:
http://stackoverflow.com/questions/23020208/why-is-gcc-4-8-2-complaining-about-addition-under-strict-overflow

A function with a reasonable argument check: hi >= lo
The function gets inlined, with hi = u + 3, lo = u
GCC figures out the condition is now u + 3 >= u, but is worried u + 3 might 
overflow

The answer quotes GCC docs section 3.8: "this warning can easily give a false 
positive: a warning about code that is not actually a problem."
Comment 5 natale.patriciello 2016-09-10 04:23:43 EDT
Hi,

We can use #pragma around code that comes from standard libraries, even if I don't know why it hasn't been reported to GCC devel yet (or it has, but I can't find the upstream bug)
Comment 6 natale.patriciello 2016-09-10 04:34:29 EDT
BTW, I'm trying to reproduce these warnings. What configure params do you use? Thanks :)
Comment 7 Tom Henderson 2016-09-10 13:50:54 EDT
(In reply to natale.patriciello from comment #6)
> BTW, I'm trying to reproduce these warnings. What configure params do you
> use? Thanks :)

./waf configure -d optimized --enable-examples --enable-tests
Comment 8 Tom Henderson 2016-09-12 16:19:33 EDT
It is not clear we will ever want to enable level >2 without some suppressions in place, and we need to audit ~400 warnings, so I suggest that for now we try to reduce the warning level in the wscript to 2.  We could leave this open to perhaps audit at a future date the list of warnings at levels 4 and 5.
Comment 9 Peter Barnes 2016-09-12 19:20:36 EDT
Suggest we close 1868 to all current discussion here.
Comment 10 Peter Barnes 2016-09-12 19:59:18 EDT
I'm disturbed by the warnings on STL code.  The most helpful things I've found:

The three LLVM blog posts beginning here:
http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html

The starting assumption, rarely stated, is that compiler optimizations *assume* 
you didn't mean to invoke undefined behavior, so they don't have to consider 
those cases in optimizing.  Look at the "RNCE before DCE" example in part 2 of 
those posts.

http://stackoverflow.com/questions/18521501
Works through a nice example, showing how to refactor a conditional.

http://stackoverflow.com/questions/23020208
Another nice example, where function inlining enables optimizations. The error 
is reported on a perfectly sensible assert conditional, despite the fact that 
the addition which enables the optimization is in the caller site.  This 
matches our STL cases exactly.

Here's my question on SO:
http://stackoverflow.com/questions/39421416
which got some mildly useful replies.

Here's the pragma stanza to wrap a false positive:
 #ifdef GNUC 
#pragma GCC diagnostic push 
#pragma GCC diagnostic ignored "-Wstrict-overflow" 
source line 
#pragma GCC diagnostic pop 
#endif
Comment 11 Peter Barnes 2016-09-12 20:03:15 EDT
+1 to Tom's pragmatic immediate approach in comment #8: fix at level=2 for now, 
then revisit later

To help us triage false positives in the future I suggest that, instead of the 
five lines of preprocessor foo, we add a standard comment to the line of code. 
(Do in line comments survive to the warning output?)

source line  // 2016-09-12 false positive strict-overflow
Comment 12 Tom Henderson 2016-09-13 19:35:09 EDT
(In reply to Peter Barnes from comment #11)
> +1 to Tom's pragmatic immediate approach in comment #8: fix at level=2 for
> now, 
> then revisit later

I pushed the change to level 2, and closed 1868 as RESOLVED, MOVED.  

One lingering comment from that report was to consider (in the future) to raise the warnings level here:

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],
 	}

but that will trigger a new batch of warnings.



> 
> To help us triage false positives in the future I suggest that, instead of
> the 
> five lines of preprocessor foo, we add a standard comment to the line of
> code. 
> (Do in line comments survive to the warning output?)
> 
> source line  // 2016-09-12 false positive strict-overflow

In looking at the warnings report, it will typically preserve the line that it points to, but it may point to an unhelpful line number such as:

../src/flow-monitor/model/histogram.cc:149:1: error: assuming signed overflow does not occur when reducing constant in comparison [-Werror=strict-overflow]
 } // namespace ns3
 ^