Integration of PVS-Studio into the CI process

Hello, dear readers.

My name is Stas, I am an engineer of the DevOps Tooling team at Align Technology.
In this article I will try to briefly describe how we implemented static code analysis based on PVS-Studio in our company.

Introduction

About a year ago, we thought about introducing static analysis into our company.
We have used various tools for this, including for C / C ++ projects. Therefore, it was interesting to try a new tool for a known task.
A tool that is more advanced than the built-in VS, cpp-check, integrated into Sonar.

As it always happens, an initiative group of developers first arose (from one developer, and his manager generally supported the idea).
He came to our team and said he wanted to try.
We contacted the developers of the PVS studio, they offered to use the trial free version.
The developer checked the code of his project and arranged a presentation for the fellow developers, showing what errors PVS-Studio helps to reveal.
Moreover, it was shown at the presentation that serious errors were encountered in the relatively new code.
The presentation was very successful, the developers laughed at their colleagues' code and their code.
The ranks of etuziast were replenished, moreover, which is valuable, including by managers.

After that, the managers (in conjunction with the developers etuziasta) calculated the ROI for the implementation of this product.
As they considered ROI, there may be a separate article on this.
In general, we bought PVS-Studio.
And our team was tasked to implement static analysis and error correction in the product development process.

Here is how we solved this problem:

Process

The process of eliminating the identified warnings is jerky, mainly due to enthusiasts.
From time to time, when someone from the initiative developers finds time, he eliminates any one type of warning.
This is more convenient, because as a rule, warnings are easily fixed without changing the code logic, and therefore suspicious
parts of the code can be easily refactored. In addition, many warnings are eliminated with one or more
more or less standard techniques, so eliminating one type of warning throughout the project is more productive,
than, for example, eliminating the whole variety of warnings in a separate file.

Technical implementation

To make life easier for etuziastam developers, we have integrated the PVS analysis into the CI process.
Atlassian Bamboo is our CI server, so I’ll use its terms in the description.
We have created special assembly plans that run automatically on a schedule. One plan for each project.

This plan starts a standalone-check of the head project for the current version of the code via the command line.
The analysis continues for quite a long time (2-4 hours), because such a test we do not take more than once a day.
After checking, the PVS analyzer generates a plog file — essentially xml with a list of warnings found.
After this file is processed a bit (absolute paths are replaced with relative ones), and then, with the help of xslt-transform,
turns into a xUnit report that can understand and visualize Bamboo.
We turn each type of warning into one unit test, with a list of all the files in which this warning is found.
After the first launch, all warnings are sent to quarantine (that is, builds will be considered successful in case of failed tests).

At that moment, when the developer fixes a particular type of warning, he releases the appropriate quarantine test.
Thus, if suddenly the warning, which we believe corrected in the whole project, suddenly appears again, we get a fallen build
and take action.

It looks like a freshly detected warning:

Standard xUnit contains a three-level hierarchy: TestSuite as a union of TestCases. The TestCase contains from 0 (if the test passed) to the Failures set. We broadcast the test results as follows.

• Testsuite are analyzers in PVS studios, for example, “Problems related to code analyzer”. This level is not used in practice for any purpose; one could dump all TestCase into one TestSuite. But we assume that for some projects some checks will not be considered, and this level of separation will come in handy.

  
  

• Testcase are specific warnings, for example, “Implicit type conversion argument of function 'foo' to 32-bit type”. Actually, this is one test from the point of view of Bamboo. TestCase for any warning is always there, regardless of whether this warning occurs in our code or not. When the warning in our code does not occur - the test turns green.

  
  
• Failure is the file and line number where a specific warning is detected. You can see where exactly it occurs and whether it is worth undertaking the correction of this type on Saturday evening. :-)

Difficulties

When integrating a PVS analyzer into our CI, several difficulties arose.

First, the difficulty with the license. We have purchased a site-license for 30 users. In accordance with the licensing policy, one installation for a specific instance of the OS “eats” one license.
Since we have a lot of assemblies and projects, we also have decent agents - we are approaching the limit of 100 agents (now there are ~ 90). Of course, not all of them are used to build C ++ code, however, we have enough of these machines. We could not put on them at all the PVS studio, we had to make several agents allocated for this. As a result, if at the moment of launching the analysis these selected agents are occupied, the analysis waits for their release and as a result takes them for a long time in the first half of the working day. Because of this, we have health-check builds that are started after each commit, lacking agents. In addition, it makes it difficult to automatically deploy agents (you have to recalculate used licenses all the time, which is not easy in the absence of a license server).

Secondly, difficulties in obtaining a directory of warnings. The only place from which you can get them is the website of the PVS-studio developers, the section of technical documentation. However, we have not yet succeeded in making an automatic parser for this page - the page layout is too inconvenient for proper parsing and changes a little each time. Yes, and there is a certain complexity of automatic centralized update of the PVS-studio versions on all agents and the directory (this can of course be solved, but has not yet reached out). So update while hands.

Result

Here are some examples of errors found, that is, when the process worked in the new code.

• V544. It is odd that the value of 'X' of HRESULT type is compared with 'Y'

  
  

  • Minor issue: style issue at this point

    
    

    static HRESULT findSomething(const X& x); // later in the code if (findSomething(x) == -1) 

    
    
  
  

• V501: There are identical sub-expressions of the foo 'operator

  
  

  • Minor issue: invalid assert - no production impact

    
    

     assert(leftPoints.size() == leftPoints.size()); 

    
    

  • Major issue: bug

    
    

     if (upper && upper) return something; 

    
    

  • Major issue: bug

    
    

     template<class T> void operator () ( const char* name, const T& t1, const T& t2 ) { if( t1 != t1 ) { diff = true; } } 

    
    
  
  

• V674: The expression contains a suspicious mix of integer and real types.

  
  
  • Major issue: bug
    

     double precision = getSettingInt("Model", "Precision", 1e-6); 

    
    

    · V655: The strings were concatenated but are not utilized. Consider inspecting the expression.
    

     switch (someEnum) { case someCondition: err + " is null "; break; 

    
    
  
  

  V596: The object was not used. The 'throw' keyword could be missing.

  
  
  • Minor issue: a bug that manifests itself with a small probability
    

      if (open(…)) { std::runtime_error("Can not open database"); } 

  
  
  

  V557: ​​Array overrun is possible.

  
  
  • Major issue: bug
    

     float Jx[3]; … for (int i=0; i<4; i++){ for (int j=i; j<4; j++){ value += Jx[i]*Jx[j]; } }