Static code analyzers on the example of ClickHouse

A little more than a month ago, an article was published containing an analysis of the ClickHouse source code using PVS-Studio. The article was quite successful: for example, the link to it was sent to me at least ten times on the day of its publication. The overall tone of the article is positive, and traffic on the site clickhouse.yandex on the day of its release has increased markedly.

I really respect when a company or a person does its work exhaustively. So, PVS-Studio has a comprehensive approach to promotion: there are only 337 articles on Habré alone. They hold reports at almost all Russian C ++ conferences. In any case, it is worth noting: people try and by their work benefit other people.

That article aroused our interest in static analyzers, and we decided to test the operation of several publicly available PVS-Studio analogues on the ClickHouse code base. In today's article we will share with you the results of this study.

On the day the article was published, we fixed all the bugs noted in it. The result can be seen in this commit . Some bugs at the time of publication were no longer in master , since we fixed them earlier.

There are no doubts about the usefulness of static analyzers. Some commentators say that it is much more useful to have code review, tests, launches under sanitizers. The benefits of these solutions are obvious. Moreover, we already have a code review (mediocre), subcommittee tests (with insufficient coverage) and launches under sanitizers (only ASan, once a day Valgrind). Static analyzers can find other bugs - for example, copy-paste in code that is never executed and which should have been removed a year ago. And something more serious also has a chance to find (read on). That is, the benefits of static analyzers are non-zero.

The article did not indicate all the detected defects, and we were offered to use the trial license of PVS-Studio to search for the rest. But before requesting a time-limited license, we first wanted to make sure that we were fairly well prepared and did not waste time.

We decided to first explore several alternatives of PVS-Studio, as a result of which we tried Clang-Tidy, ppcheck, Coverity and Svace. For each of them, we managed to compile instructions on how to properly use them for ClickHouse code.

Cppcheck

Instructions for use: cppcheck.txt .

This tool has several important advantages: it is very easy to use and works extremely fast. So, our repository was analyzed in a few seconds. The standard launch mode resulted in only a few false positive warnings. About 200 warnings were received in the --enable-all mode, some of which were meaningful:

• Someone forgot to enter one character . Lucky - the error does not affect the behavior of the program.
• Non-use of the function log1p - did not know that such exists. In this case also does not affect the behavior of the program.

In addition, the analyzer also found some non-optimalities when passing parameters to functions, forgotten explicit-definitions, too wide areas of visibility for some variables, and so on.

Many false positives are due to the fact that the analyzer is very primitive. Apparently, it does not even fully parse C ++. For example, the following code

 int x = 0; auto lambda = [&]{ x = 1; }; 

treated in the same way as the code:

 int x = 0; { x = 1; }; 

Nevertheless, a large number of false positives is the norm for static code analyzers. The scenario of using them is to look at a few hundred errors and, possibly, to find among them several really significant ones.

Clang-tidy

Instructions for use: clang-tidy.txt

An important advantage of this tool is that it is located in the Clang ecosystem. This means that if your system is compiled with Clang, then Clang-Tidy will correctly take into account all the features of the assembly of your project. There are instructions on how to add your own checks.

Clang-Tidy spends about a dozen seconds analyzing each translation unit. It is slow enough.

Clang-Tidy differs from Clang static analyzer (also known as scan-build) by the presence of style checks. Some of the style checks let you automatically rewrite code. It is worth noting that the Clang static analyzer, unlike the Clang-Tidy, can generate HTML pages with the results, whereas the Clang-Tidy only displays messages about the problems found in the terminal window.

There is no point in using all types of checks. For example, one of the CERT Secure Coding Standards checks, C ++ Core Guidelines or HTC ++ says “do not use pointer arithmetic”. It is in some sense reasonable, but it is too painful to explicitly mark all the sources for which this check should be turned off.

Helpful messages were found in the static analyzer and performance test groups. Performance found two types of errors: post iterations of iterators and passing arguments by value instead of passing by reference. Unfortunately, checking the transmission of arguments by reference works even for small structures, for example:

 struct S { int x; }; 

or STRONG_TYPEDEF .

In this case, replacing a transfer by value to a transfer by reference may turn out to be useless or even harmful (provided that the function is not inline and not cloned, because in these cases there is no difference). In other situations, the check turned out to be more useful, see the commit with corrections. Of course, such errors should be detected at the code review stage - provided that you were attentive enough. Note that after correcting all the performance warnings, the actual performance of ClickHouse has not changed. This is expected: if the corresponding errors were in narrow places, we would have noticed it the first time you run sudo perf top .

The warnings from static analyzer are quite useful:

• Call a virtual function in the constructor . It turned out that virtual was written by mistake.
• Loss of precision in calculations
• Null pointer dereferencing .

Coverity

Instructions for use: coverity.txt .

Coverity is a closed commercial product, but for open source projects it can be used for free. Among the famous products that use Coverity are, for example, Linux.

The scheme of work: a utility is downloaded from the site, which wraps any build system (instead of make simply indicates cov-build make ) and intercepts the compiler calls. It was not the first time we managed to make it work - I had to go through several options how to tell it which compiler was being used.

For some translation units, the tool’s running time reaches an hour, and the total analysis time of the entire project is 4 hours. The result is a tarball that can be downloaded to the Coverity website. Archive size 1.5 GB booted successfully. You can download and more voluminous results.

Then, in order to gain access to the results of the analyzer, it is necessary to pass a test for the ability to work with the source code of the analyzed project. For example, they gave me access for one and a half days.

After that, a beautiful web-interface became available, where you can interactively study the code with annotations of defects, as well as note resolutions (false positive, intentional, fix required, etc.).

Immediately they gave an achivka, although I did not understand why.

Found 312 some defects.

• An uninitialized variable if the user in the config does not specify a single element in one of the sections.
• The dereferencing of the end iterator is only to turn it into an address.
• Every little thing .

Some of the defects found are actually part of the intended design.

• Tautological conditions in if in template code. This is completely normal, is a common practice and code style. Moreover, the code is written on the assumption that the compiler knows this very well and will remove unnecessary code.
• An uninitialized class member in the default constructor. Sometimes this is really a mistake, but in some cases it is acceptable.
• Raw exception in the main function in the test program. This allows our style. If an exception crashes out of the main function, it will be processed using std :: terminate, the standard library will do a few useful things for you (verbose terminate handler): it will output an exception type, for successors, std :: exception will output what, call abort; the program will receive an abort signal, the operating system will write a core dump, and the program will end with a non-zero code. All this is very useful.

In addition, one of the errors looked like an Unrecoverable parse error at the sight of std :: shared_mutex. This means that Coverity does not support C ++ 17.

Svace

November 30, 2016 I got to the conference "Database Technology" , where I was able to agree with colleagues from ISP RAS about using their static analysis tool Svace. This tool is implemented in the Samsung Corporation, but is little known among domestic programmers.

The analysis results are available in a convenient web interface with viewing the source code and annotations to it. The most serious error detected is the absence of freeaddrinfo in one of the code branches . It is worth noting that both the results of the analysis and the sampling of defects for study were provided by colleagues from ISP RAS.

findings

Using static analyzers is not the first important measure to improve the code of your product. For example, if you do not have auto-assembly with running tests at least under Address Sanitizer, it will be more useful to ensure this first. Analysis of the results of the work of static analyzers requires a large amount of time, at least at the very beginning. Proper use requires integration into the CI system. Only if you have done simpler things - you can try such a wonderful thing as static analyzers. But why should I discourage you - try it for sure!

Finally, I would like to thank once again the colleagues from PVS-Studio for the work they are doing, in particular, for the article dedicated to ClickHouse. They not only helped us by finding a few important problems in our code, but also inspired research that this article is about.