We have used a trial version of PVS-Studio for HPX previously, but I vaguely remembered it as being very verbose in its diagnostics. I have read a lot about the tool lately, and since it was a long time since we used it, we contacted the developers at Viva64 asking whether they would be willing to support our open source project. We were positively surprised that they agreed to provide us with a free license for one year in exchange for a blog post about our experience with the tool.
This article was originally published at STE||AR Group Blog. Republished by the editors' permission.
So I downloaded PVS-Studio V5.26 and installed it (without any problems) as an extension for my VS2013 Professional (update 4) setup. I would have preferred to test it with VS2015RC1, as this is currently my main development environment. Unfortunately, VS2015 is not supported yet, I however expect that it will be supported as soon as Microsoft releases the new version.
The integration of the PVS-Studio into the Visual Studio user interface made a very good impression on me. One additional top level menu item provides access to all of the commands and options. All of the generated diagnostics are placed into a special output window from where you can jump to the source code that a message refers to. You can also open a web-based, context sensitive help explaining each of the generated diagnostics in more detail. In short, all is as it should be.
The diagnostics generated have three severity levels (high, medium, and low) and are grouped into three categories (general analysis, optimization analysis, and 64-bit compatibility analysis). The user interface allows restriction of the shown diagnostics to only some (or one) of those and additionally allows for filtering to reduce the amount of messages to work through. For the main HPX module the tool generated about 70 diagnostics in about 1,000 C++ header and source files (~140,000 lines of code), which was not too bad (high severity: 5, medium: 44, low: 21). The initial analysis on my laptop took about 10 minutes.
I was eager to see what mistakes or problems the tool might reveal. The HPX team is very conscious about code quality and we have the policy that code has to be reviewed by at least one other developer before it can go into the main branch. So I was quite confident the tool wouldn't find anything.
Let's look at the high severity diagnostics first. Four of the diagnostics were very similar, and the context for one of them is shown below:
template <typename Archive>
void load(Archive& ar)
{
actions::manage_object_action_base* act = 0;
ar >> hpx::serialization::detail::raw_ptr(act);
// V522: Dereferencing of the null pointer 'act' might take place.
HPX_ASSERT(act->is_valid());
// ...
}This code is de-serializing a polymorphic object through its base pointer and we know that raw_ptr(act) is allocating a new object instance for the de-serialized object returning a pointer to it through its argument. We also know thatraw_ptr(act) would throw in case of any error. All of this should have been visible to PVS-Studio as all the related code is located in header files. The tool apparently was not able to see that, which is why it generated the diagnostic. Luckily you can tell PVS-Studio to ignore this particular error with a single mouse click, which adds a magic comment to the corresponding source code: //-V522, thus suppressing this message in the future - nifty. PVS-Studio gives you many more options for suppressing diagnostics - file or directory based, file name pattern based, or specific to a particular diagnostic globally - all of those easily accessible and self-explanatory.
A second diagnostic was really alarming to me. Here is the corresponding code:
#define HPX_VERSION_MAJOR 0
#define HPX_VERSION_MINOR 9
#define HPX_VERSION_SUBMINOR 11
std::string full_version_as_string()
{
// V609 Mod by zero. Denominator '0' == 0.
return boost::str(
boost::format("%d.%d.%d") %
HPX_VERSION_MAJOR % HPX_VERSION_MINOR %
HPX_VERSION_SUBMINOR);
}It took me a moment to understand what PVS-Studio was trying to convey, since for me the overloaded operator%()implemented by the Boost.Format library was totally inconspicuous. However, even after realizing that the code would have been problematic if the operator was not actually overloaded, the generated diagnostic itself still did not make too much sense to me. In the end, I 'resolved' this message by suppressing it as well.
The last 'high severity' diagnostic was an optimization analysis result:
// V808 'hostname' object of 'basic_string' type was created
// but was not utilized.
std::string hostname = boost::asio::ip::host_name();Sure enough, the tool was right, the variable 'hostname' was completely unused in this context. None of the compilers we use for our regular testing on more than 20 different platforms had reported this before - nice catch!
The generated diagnostics from lesser severity levels were mostly things worth looking at once, but almost all of those flagged benign issues related to 32bit/64bit compatibility, like implicit conversions of signed integers to larger unsigned representation widths (e.g. int32_t --> uint64_t).
Two diagnostics helped finding actual bugs, though. In one place we had this code:
int runtime_support::load_components(util::section& ini)
{
// load all components as described in the configuration information
if (!ini.has_section("hpx.components")) {
// V601 The 'true' value is implicitly cast to the integer type.
return true; // no components to load
}
// ...
}The generated diagnostic pointed us to a problem: a while back we had changed the return type of the function frombool to int (including a change to the semantics of the returned value), but we forgot to adapt one of the return statements. This could have created problems which are difficult to reproduce.
Another of the useful diagnostics actually revealed a possibly more serious problem:
struct when_each_frame
{
// ...
private:
// V690 Copy constructor is declared as private in the
// 'when_each_frame' class, but the default '=' operator
// will still be generated by compiler. It is dangerous
// to use such a class.
when_each_frame();
when_each_frame(when_each_frame const&);
public:
// ...
};This was a very nice catch indeed! Especially as we added the constructor declarations as a workaround for older versions of gcc which were incorrectly instantiating default implementations for those constructors.
In the end, I was happy to have invested my time into doing the work of running PVS-Studio on all of our files. I was also happy to see that no absolutely serious issues were diagnosed, another confirmation of the validity of our code review policy we put in place a while ago. As an action item, I noted to integrate running PVS-Studio as part of our contiguous integration system, which triggers builds on every commit to our development branch.
Static analysis definitely has its place. Running tools like PVS-Studio costs additional time and effort which in my book is absolutely well invested. Compilers can not always afford to perform such deep analysis that PVS-Studio attempts to do, as this would increase compile times even further.
Especially useful for us will be the ability to seamlessly integrate the tool into our contiguous integration build system. This will also have another nice side effect: since we run our daily tests on many platforms (including Windows), it will make the results of the static analysis produced by a Windows-only tool available to our developers who mainly work on other platforms (like Linux or Mac/OS).
Another nice option of the IDE integration of PVS-Studio is the ability to automatically run the analysis after each successful build. Surprisingly, this does not impose too much overhead onto the normal build process. This is a very useful feature which gives feedback for subtle problems very early on during development of new code. I will try to leave that option enabled for a while to see how effective that is.
When looking through all of the generated diagnostics, I was surprised to realize that - even while the tool has all the time it needs to do an analysis as deeply as possible - PVS-Studio seems not to be able to look through certain operator overloads to figure out that those actually implement semantics different from the default. The example I showed above demonstrates this: the operator%() overload in Boost.Format does everything but an integral modulo operation (it performs string formatting instead), but PVS-Studio still warns about a possible division by zero. Sure, this is a real corner case and I'm not sure if it is always possible to provide the right level of diagnostics. On the other hand, this is where the real value of static analysis could be: deep semantic checking of our codes.
In any case, if you are interested in trying out HPX, please fork it from our github site.