Closed matthiaskrgr closed 7 years ago
Visual Studio agrees on many of those. I think it caught a couple not listed. Most of the notes from Visual Studio, though, were in upstream packages which we don't want to fix unless they can be shown to be actual problems.
I ran across an online tool, Coverity, which purports to run exhaustive static analysis. Wesnoth is set up there but hasn't been run in a year. I sent a request to be added so I could check it out but whoever manages Wesnoth there has not acted on my request, yet.
My opinion is any notes about Wesnoth code should be carefully considered and corrected.
I didn't look through all of that, but it seemed like at least one report in a Boost header is likely related to a problem at the call site of that Boost function.
The warnings in Lua should probably be ignored, as well.
The Lua warnings were the ones I dug into. They're mainly due to struct member alignment and padding. I could fix them (using unspecified/undefined behaviors) or convert the members to another type. Lua, however, is specified to be solely ISO Standard C and any fix using unspecified or undefined behavior would be rejected upstream.
Many static analyzers, remember, are simply suggestions. While a goal of zero messages is laudable, it is often unreasonable and even unattainable. Each suggestion, however, should be carefully considered (even if it is in an upstream package, like Boost). A note about memcpy, for example, probably appears because, over the decades, memcpy has been such a security headache. While it might not be an issue for Wesnoth, the comment should be carefully considered. We might not be able to replace the function, and might not be able to add call-site checks, we can, at least, audit each use and document the issue and why it's not a problem for Wesnoth.
Here's the output with the Boost, Lua, tests, SDL_SavePNG, and compiler output stuff stripped:
/home/matthias/vcs/github/wesnoth/src/sdl/utils.cpp:488:7: warning: Value stored to 'avg_a' is never read
avg_a /= count;
^ ~~~~~
/home/matthias/vcs/github/wesnoth/src/sdl/utils.cpp:578:5: warning: Variable 'xloc' with floating point type 'float' should not be used as a loop counter
for(float xloc = xsrc; xloc < xsrc+xratio; xloc += 1) {
^ ~~~~ ~~~~
/home/matthias/vcs/github/wesnoth/src/sdl/utils.cpp:581:6: warning: Variable 'yloc' with floating point type 'float' should not be used as a loop counter
for(float yloc = ysrc; yloc < ysrc+yratio; yloc += 1) {
^ ~~~~ ~~~~
/home/matthias/vcs/github/wesnoth/src/sdl/utils.cpp:1504:21: warning: Division by zero
| (std::min(red/avg,ff) << 16)
~~~^~~~
^
/home/matthias/vcs/github/wesnoth/src/serialization/preprocessor.cpp:331:8: warning: Use of memory after it is freed
if (!current_->get_chunk()) {
^~~~~~~~~~~~~~~~~~~~~
/home/matthias/vcs/github/wesnoth/src/generators/default_map_generator_job.cpp:556:4: warning: Value stored to 'avg_distance' is never read
avg_distance = 0;
^ ~
/home/matthias/vcs/github/wesnoth/src/generators/default_map_generator_job.cpp:889:2: warning: Value stored to 'ticks' is never read
ticks = SDL_GetTicks();
^ ~~~~~~~~~~~~~~
/home/matthias/vcs/github/wesnoth/src/generators/default_map_generator_job.cpp:1158:2: warning: Value stored to 'ticks' is never read
ticks = SDL_GetTicks();
^ ~~~~~~~~~~~~~~
In file included from /usr/include/boost/algorithm/string/predicate.hpp:22:
/usr/include/boost/algorithm/string/compare.hpp:43:30: warning: Out of bound memory access (access exceeds upper limit of memory block)
return Arg1==Arg2;
^~~~
/home/matthias/vcs/github/wesnoth/src/sound_music_track.cpp:99:4: warning: Opened File never closed. Potential Resource leak
LOG_AUDIO << "track does not appear to be an Ogg file '"
^~~~~~~~~
/home/matthias/vcs/github/wesnoth/src/sound_music_track.cpp:28:19: note: expanded from macro 'LOG_AUDIO'
#define LOG_AUDIO LOG_STREAM(info, log_audio)
^~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/matthias/vcs/github/wesnoth/src/log.hpp:187:39: note: expanded from macro 'LOG_STREAM'
#define LOG_STREAM(level, domain) if (lg::level().dont_log(domain)) ; else lg::level()(domain)
^~~~~~~~~
/home/matthias/vcs/github/wesnoth/src/sound_music_track.cpp:105:30: warning: Opened File never closed. Potential Resource leak
vorbis_comment* comments = ov_comment(&vf, -1);
^~~~~~~~~~
Refer to --analyzer-output for valid output formats
/home/matthias/vcs/github/wesnoth/src/terrain/builder.cpp:168:48: warning: Division by zero
img_list.back().set_animation_time(ri.rand % img_list.back().get_animation_duration());
~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/matthias/vcs/github/wesnoth/src/terrain/builder.cpp:1191:30: warning: Forming reference to null pointer
const map_location loc = legacy_difference(*itor, min_constraint->loc);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/matthias/vcs/github/wesnoth/src/terrain/translation.cpp:740:9: warning: Value stored to 'end' during its initialization is never read
size_t end = str.size();
^~~ ~~~~~~~~~~
/home/matthias/vcs/github/wesnoth/src/widgets/scrollbar.cpp:353:54: warning: Division by zero
int dep = y_dep * int(full_height_ - grip_height_)/ (groove.h - grip.h);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
/home/matthias/vcs/github/wesnoth/src/actions/attack.cpp:1377:4: warning: Value stored to 'hits' is never read
hits = results_hits;
^ ~~~~~~~~~~~~
/home/matthias/vcs/github/wesnoth/src/actions/attack.cpp:1388:4: warning: Value stored to 'damage' is never read
damage = results_damage;
^ ~~~~~~~~~~~~~~
/home/matthias/vcs/github/wesnoth/src/ai/default/attack.cpp:244:20: warning: Called C++ object pointer is null
chance_to_kill = prev_def->hp_dist[0];
^~~~~~~~~~~~~~~~~~~~
/home/matthias/vcs/github/wesnoth/src/editor/palette/tristate_button.cpp:102:8: warning: Value stored to 'new_state' during its initialization is never read
STATE new_state = state_;
^~~~~~~~~ ~~~~~~
/home/matthias/vcs/github/wesnoth/src/game_initialization/connect_engine.cpp:248:6: warning: Value stored to 'side_assigned' is never read
side_assigned = true;
^ ~~~~
/home/matthias/vcs/github/wesnoth/src/gui/dialogs/multiplayer/mp_options_helper.cpp:68:6: warning: Value stored to 'pos' during its initialization is never read
int pos = remove_nodes_for_type("campaign");
^~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/matthias/vcs/github/wesnoth/src/gui/widgets/settings.cpp:531:2: warning: Returning null reference
return *best_resolution;
^~~~~~~~~~~~~~~~~~~~~~~
/home/matthias/vcs/github/wesnoth/src/gui/widgets/tree_view_node.cpp:753:9: warning: Called C++ object pointer is null
while(!cur->is_folded() && cur->count_children() > 0) {
^~~~~~~~~~~~~~~~
/home/matthias/vcs/github/wesnoth/src/server/server.cpp:2355:3: warning: Value stored to 'banned' is never read
banned = true;
^ ~~~~
/home/matthias/vcs/github/wesnoth/src/server/server.cpp:2382:7: warning: conditions of the inner and outer statements are identical
if(!banned) {
^~~~~~~
/home/matthias/vcs/github/wesnoth/src/server/server.cpp:2424:3: warning: Value stored to 'banned' is never read
banned = true;
^ ~~~~
/home/matthias/vcs/github/wesnoth/src/server/server.cpp:2458:7: warning: conditions of the inner and outer statements are identical
if(!banned) {
^~~~~~~
/home/matthias/vcs/github/wesnoth/src/server/server.cpp:2509:3: warning: Value stored to 'banned' is never read
banned = true;
^ ~~~~
/home/matthias/vcs/github/wesnoth/src/server/server.cpp:2536:7: warning: conditions of the inner and outer statements are identical
if(!banned) {
^~~~~~~
You're using the SVN / Development build for Clang 6.0?
I reran using the current release and get NO messages at all.
A glance through the messages @Vultraz pulled out and most do look like they deserve attention. But I'm not sure I want to rely upon a development version of the compiler. Since I run Arch, and keep fairly up-to-date, I tend to run into these sorts of warnings within a few days of the compiler releasing. I'll do some research and if I can do a side-by-side install I'll take a look at these warnings; otherwise I'll wait for 6.0 to come out.
Yeah, I'm compiling llvm + tools from git sources myself.
Anyway, I also had this situation where I would not get any messages at all, which happened when I only ran scan-build with make but not with cmake.
scan-build cmake ...
scan-build make ...
worked though.
5.0 was just branched a few days ago, so the next version is major going to be 5.0, with 6.0 being released in a few months.
I can get 6.0 from Arch AUR on a 6-hour refresh cycle from the SVN but don't want to commit to it unless I can side-by-side with the release version.
I saw your commands and tried them but not in an out-of-tree. I'll try again.
For the record, I was able to reproduce all the findings that Vultraz extracted with 4.0.
Well, I wish I could. I did rm -fR on ~/build and ~/CMakeFiles after doing make/scons cleans. And then followed your process above using cut-and-paste. I don't get any messages at all.
I just sync'd to master and hit a couple new minor errors; but still cannot get the static analysis to run. I'll keep looking into it.
Did it say "scan-build: Using <compiler ..>" before giving any build output? Did you pass the analyzer flags for both, the build-sys and the make invocation?
I copied and pasted the command lines you used, in the OP. And yes it says it's using clang 4.0. I'm looking at the VC15 static analysis output right now and will get back to clang in a bit.
can you see scan-build and its flags being used when running make in verbose mode?
with VERBOSE=1 I do not see scan-build and all those flags.
Odd, I don't understand what is wrong :/ can you please just for the record post the commands you used?
$ scan-build -enable-checker alpha.core.BoolAssignment,alpha.core.CastSize,alpha.core.FixedAddr,alpha.core.IdenticalExpr,alpha.core.PointerArithm,alpha.core.PointerSub,alpha.core.SizeofPtr,alpha.security.ArrayBoundV2,alpha.security.MallocOverflow,alpha.security.ReturnPtrRange,alpha.unix.Chroot,alpha.unix.PthreadLock,alpha.unix.SimpleStream,alpha.unix.Stream,alpha.unix.cstring.BufferOverlap,alpha.unix.cstring.NotNullTerminated,alpha.unix.cstring.OutOfBounds,security.FloatLoopCounter,security.insecureAPI.rand cmake .
$ scan-build -enable-checker alpha.core.BoolAssignment,alpha.core.CastSize,alpha.core.FixedAddr,alpha.core.IdenticalExpr,alpha.core.PointerArithm,alpha.core.PointerSub,alpha.core.SizeofPtr,alpha.security.ArrayBoundV2,alpha.security.MallocOverflow,alpha.security.ReturnPtrRange,alpha.unix.Chroot,alpha.unix.PthreadLock,alpha.unix.SimpleStream,alpha.unix.Stream,alpha.unix.cstring.BufferOverlap,alpha.unix.cstring.NotNullTerminated,alpha.unix.cstring.OutOfBounds,security.FloatLoopCounter,security.insecureAPI.rand make -j2 -B VERBOSE=1 wesnoth wesnothd campaignd test
It's working here with these commands.
Apparnetly I was wrong about the the scan-build flags (security.insecureAPI.rand etc) showing up in the call by make but I can see that it does call c++-analyzer instead of clang++.
s'ok now that I know where to look I'll probably figure it out. Most likely it's related to my system having both GCC and CLang installed.
Hmm right. On most linuxes, CXX points to g++ but I have set my CXX to clang++ via zshrc.
maybe try CXX=clang++ scan-build ....
scan-build.txt Hi, I ran clang static analyzer aka scan-build on wesnoth, these are the results. Unfortunately it failed to generated the reports as html for some reason, but I have the raw text.
Wesnoth @ 34d13addadb25595b1be6b6fe4d092fdbdfbd557
results: