Checking PVS-Studio plugin with PVS-Studio analyzer

• More details about testing PVS-Studio
• PVS-Studio Plugin
• Conclusion

One of the questions that people ask us all the time since the creation of PVS-Studio is - "Do you check PVS-Studio with PVS-Studio? Where is the article about the analysis results?" So the answer is "yes" - we do that regularly; that's why we weren't able to write about the bugs we found in our own analyzer. Usually we fix the bugs at the stage of writing the code, so we just don't think about noting them down. But this time it's a different story. Because of a slight oversight on our side, the C# code for the Visual Studio plugin wasn't added to the daily overnight checks. Consequently, the bugs in it haven't been noticed since the beginning of the C# PVS-Studio development. But every cloud has a silver lining, so now we have such an article.

More details about testing PVS-Studio

Perhaps, some readers might be interested to know about the process of testing PVS-Studio. We have already written an article on this topic. But it's been a long time, so a lot of things have changed. That's why we have here, the story of our current state of things.

We use seven main methods of testing in the PVS-Studio development.

• Static code analysis on our developers' machines. Every developer has PVS-Studio installed. New code fragments, and the edits made in the existing code, are instantly checked by means of incremental analysis. We check C++ and C# code.
• Static code analysis during the nightly builds. If the warning wasn't catered for, it will show up during the overnight build on the server. PVS-Studio scans C# and C++ code. Besides that, we also use Clang to check C++ code.
• Unit-tests at the class, method and function levels. This system isn't very well-developed, as some situations are hard to test because of the necessity to prepare a large amount of input data for the test. We mostly rely on high-level tests.
• Functional tests for specially prepared and marked up files containing errors.
• Functional tests to prove that we are parsing the main system header files correctly.
• Regression tests of individual third-party projects and solutions. This is the most important and useful way of testing for us. To do it, we regularly check 105 open projects written in C++ and 49 in C#. Comparing old and new analysis results we check that we haven't broken anything; it also provides an opportunity to polish new diagnostic messages.
• Functional tests of the user interface - the add-in, integrated in the Visual Studio environment.

So how did we happen to overlook the plugin check? We don't know that ourselves. No clue, really. Nobody had a thought to add the check of the plugin code on the server. The check of the new C# analyzer was added, but the plugin was left out in the cold. As a result, the C# analyzer was finding bugs in itself while it was developed. But the plugin, written in C#, was slightly abandoned. There were practically no changes in it lately; that's why the incremental analysis was of no help, as we didn't work on the plugin code, and there were no overnight checks.

PVS-Studio Plugin

To be honest with our clients, and to avoid thoughts like "Hey, you guys are always pointing out everyone else's errors, why not your own?", we'll describe all our errors, even the most ridiculous ones.

For all the bugs found, we have to give credit to PVS-Studio analyzer, v6.02, which now has C# support.

Let's start with real errors that had already been fixed by the time this article was written.

public void ProcessFiles(....)
{
  ....
  int RowsCount = 
    DynamicErrorListControl.Instance.Plog.NumberOfRows;
  if (RowsCount > 20000)
    DatatableUpdateInterval = 30000; //30s
  else if (RowsCount > 100000)
    DatatableUpdateInterval = 60000; //1min
  else if (RowsCount > 200000)
    DatatableUpdateInterval = 120000; //2min
  ....
}

The analyzer issued two warnings:

V3022 Expression 'RowsCount > 100000' is always false. ProcessingEngine.cs 559

V3022 Expression 'RowsCount > 200000' is always false. ProcessingEngine.cs 561

The human brain usually thinks sequentially - simple things first, then complex ones; or, as in this case - from smallest to largest, checking all the values. In this case, this logic led to incorrect program behavior. The error is in the check of the number of rows in the table. Having checked the first condition, that there are more than 20000 strings, the program assigns DatatableUpdateInterval with a value of 30 seconds; of course, it will not check other conditions. Even if RowsCount is 1,000,000.

This code was written to optimize the error message display in the Visual Studio IDE window of PVS-Studio. Initially the PVS-Studio plugin issued the analysis results as soon as they were ready, i.e. right at that moment when they were obtained from the cmd process (that invokes the analyzer kernel). But doing the analysis on massive projects, we noticed considerable lagging of the interface. There were, in particular, many issues with the projects which had large number of *.c files. The *.c files are checked very fast, and the UI thread was busy updating the result tables. We decided to add a delay between the updates that would increase with the number of messages. The lag was 15 seconds if the number of messages was less than 20000.

We are lucky in this case as we'll get only a slight slowdown in the program (especially since we rarely get more than a hundred thousand messages after a check), but this analyzer message is meant to reveal more serious cases. For example, it happened in one project from the Infragistics Company:

public static double GenerateTemperature(GeoLocation location){
  ....
  else if (location.Latitude > 10 || location.Latitude < 25) 
  ....
  else if (location.Latitude > -40 || location.Latitude < 10)
  ....
}

The condition will always be true, which leads to erroneous calculations.

The next error was more pivotal for our project:

public bool GeneratePreprocessedFile(....)
{
  ....
  if (info.PreprocessorCommandLine.Contains(" /arch:SSE"))
    ClangCommandLine += " /D \"_M_IX86_FP=1\"";
  else if (info.PreprocessorCommandLine.Contains(" /arch:SSE2"))
    ClangCommandLine += " /D \"_M_IX86_FP=2\"";
  else if (info.PreprocessorCommandLine.Contains(" /arch:IA32"))
    ClangCommandLine += " /U \"_M_IX86_FP\"";
  else if (info.PreprocessorCommandLine.Contains(" /arch:AVX"))
    ClangCommandLine += " /D \"_M_IX86_FP=2\"";
  ....
}

V3053 An excessive check. Examine the conditions containing search for the substrings ' /arch:SSE' and ' /arch:SSE2'. StandaloneProjectItem.cs 229

Although the error has a different number, in essence, it is still the same. The human logic - moving from simple to complex things - failed again. Having checked the "info.PreprocessorCommandLine" value for the substring " /arch:SSE", we will fulfil condition in the case when the "info.PreprocessorCommandLine" will contain the substring " /arch:SSE2". As you see, the test that reads quite naturally, doesn't comply with the logic we want to set in the program. Even if we know that there is " /arch:SSE2" in the PreprocessorCommandLine, reviewing the code, we will notionally add " /D \"_M_IX86_FP=2\"" instead of " /D \"_M_IX86_FP=1\""; to the ClangCommandLine variable.

From the point of view of the analyzer, the error was in the incorrect definition of the _M_IX86_FP macro when passing the /arch:SSE2 flag to the compiler. The thing is, that before the analysis starts, PVS-Studio utilizes Clang instead of cl (a standard preprocessor in Visual C++) for preprocessing - Clang is much faster. Unfortunately, the support of C++ dialect from Microsoft in Clang is still far from being perfect - that's why if Clang doesn't manage to preprocess something, PVS-Studio addresses cl. Thus, we transform cl compiler flags to Clang defines. Details.

This error most probably wasn't causing any errors for Clang preprocessor or incorrect analysis results, that's why it was sitting in our code for a pretty long time.

One more real bug is the call of ProcessAnalyzerOutput function.

private void PVSFinishKey(ref Hashtable PathExcludes)
{
  ....
  ProcessAnalyzerOutput(fileName,
                        projectName, 
                        task.strStandardOutput, 
                        task.strStandardError, 
                        false, 
                        ref ParsedOutput, 
                        ref PathExcludes);
}

It's not easy to spot a bug, even looking at the way the function is declared:

private void ProcessAnalyzerOutput(
                        String fileName, 
                        String projectName, 
                        String strStandardError, 
                        String strStandardOutput, 
                        bool LargeFileMode, 
                        ref List<ErrorInfo> ParsedOutputLines, 
                        ref Hashtable PathExcludes)
{
  ....
}

The problem is in the mismatch of the function parameters, and names of the arguments that are passed there:

V3066 Possible incorrect order of arguments passed to 'ProcessAnalyzerOutput' method: 'strStandardError' and 'strStandardOutput'. ProcessingEngine.cs 1995

In such a long list of function parameters, it's rather difficult to notice the discrepancy. Even IntelliSense isn't always a way out in such cases. Moreover, in large projects, it has a tendency to lag, and it's not always clear which element you are now on.

Very unpleasant situations can occur because of this. Although this diagnostic is of the third level, along with all heuristic ones, it is still very useful and shouldn't be ignored.

The fragment where the error was spotted is a "stub" - the stderr and stdout parameters never got anything besides empty strings. This error would reveal itself rather fast, once this stub is used with real data.

There was one more bug detected by a diagnostic V3072 (which is still in the developmental stage):

sealed class ProcessingEngine
{
  ....
  private readonly PVSMessageBase _retiredMessageBase; 
  ....
}
public sealed class PVSMessageBase : 
       ContextBoundObject, IDisposable
{
  ....
}

This diagnostic is designed to find fields having a type implementing IDisposable in the class, which does not itself implement IDisposable. Code like this shows that a programmer probably forgot to clean up some resources after using an object of this class.

In this case, we see that in the ProcessingEngine class (that does not implement IDisposable interface), there is a field of PVSMessageBase class, whose type does implement IDisposable.

This can be attributed as a false positive, which is caused by architecture which is not very "neat". The ProcessingEngine class is used in the program as singleton. That's why there is only one instance of it, as well as the PVSMessageBase in the program during the whole lifetime of the program. The resources will be released after execution of the program completes.

Fortunately, there weren't any other serious bugs found in the code. The rest of the analyzer warnings are more a "just in case" format.

For example, in such an expression:

private int GetSetRemainingClicks(....)
{
  ....
  if ((CurrentRemClicks != 0) || 
      ((CurrentRemClicks == 0) && DecrementCurrent))
  {
    ....
  }
  ....
}

V3031 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions. DynamicErrorList.cs 383

This code can be safely cut to:

if (CurrentRemClicks != 0 || DecrementCurrent)
{

There were also found several more "double-checks":

private void comboBoxFind_KeyDown(object sender, KeyEventArgs e)
{
  ....
  if (e.KeyCode == Keys.Escape)
  {
    if (e.KeyCode == Keys.Escape)
    {
      ProcessingEngine.PluginInstance.HidePVSSearchWindow();
    }
  }
}

Here we see a check of a very evident thing:

public IList<KeyValuePair<String, DataRow>> 
  GetSortedNonRetiredRows()
  {
    if (ei.ProjectNames.Count == 1)
    {
      ....
    }
    else if (ei.ProjectNames.Count > 1)
    {
      ....
    }
    else if (ei.ProjectNames.Count == 0)
    {
      ....
    }
  }

V3022 Expression 'ei.ProjectNames.Count == 0' is always true. PlogController.cs 277

Since we've started doing double-checks, let's stick till the end, and check everything. Like in this fragment, for instance:

void ProcessVCProjAnalysisIntegration (String Path, bool Remove)
{
  if (Remove)
  {
    ....
  }
  else if (!Remove)
  {
    ....
  }
}

V3022 Expression '!Remove' is always true. VCProjectEngine.cs 733

Sometimes we have quite weird casts, but we promised to be honest, so here we go:

private bool PostWebRequest(String post_data)
{
  ....
  String Sts = ex.Status.ToString() as string;
  ....
  string sts = wex.Status.ToString() as string;
  ....
}

V3051 An excessive type cast. The object is already of the 'String' type. TrialExtensionRequest.cs 181

V3051 An excessive type cast. The object is already of the 'String' type. TrialExtensionRequest.cs 241

private String Get_StructMemberAlignment()
{
  ....
  if (CompileAsManaged.Equals("false") ||
      String.IsNullOrEmpty(CompileAsManaged))
    Params += " /GR-";
  ....
}

V3027 The variable 'CompileAsManaged' was utilized in the logical expression before it was verified against null in the same logical expression. MSVCParamsGenerator.cs 801

And once more:

private String Get_DisableLanguageExtensions()
{
  ....
  else if (DisableLanguageExtensions.Equals("false") ||
           String.IsNullOrEmpty(DisableLanguageExtensions))
  {
  ....
}

V3027 The variable 'DisableLanguageExtensions' was utilized in the logical expression before it was verified against null in the same logical expression. MSVCParamsGenerator.cs 1118

It is an error to verify the variable against null after the call of Equals function. In fact there is no real bug here, because according to API, the variable "CompileAsManaged" and "DisableLanguageExtensions" cannot contain null. That's why the checks can be simplified to:

CompileAsManaged == string.Empty
DisableLanguageExtensions == string.Empty

Let's see which code fragments got the attention of our analyzer.

private static DialogResult ShowModalDialog(....)
{
  ....
  if (buttons == MessageBoxButtons.YesNo || 
     buttons == MessageBoxButtons.YesNoCancel)
       return DialogResult.Yes;
  else if (buttons == MessageBoxButtons.OKCancel)
       return DialogResult.OK;
  else
       return DialogResult.OK;
}

V3004 The 'then' statement is equivalent to the 'else' statement. Utilities.cs 496

The verification of buttons variable against MessageBoxButtons.OKCancel makes no sense, as in any case DialogResult.OK will be returned. As a result, the code shrinks to:

return (buttons == MessageBoxButtons.YesNo || 
       buttons == MessageBoxButtons.YesNoCancel) ?
       DialogResult.Yes : DialogResult.OK;

And the last one. Perhaps, the refactoring is to blame here:

public bool ReadPlog(....)
{
  ....
  XmlReadMode readMode = XmlReadMode.Auto;
  ....
  readMode = dataset.ReadXml(filename);
  ....
}

V3008 The 'readMode' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 512, 507. PlogController.cs 512

Conclusion

Checking your own code provokes various feelings. At times, you try to fix your own bug as soon as possible, or find an excuse for it. If it is somebody's mistake, there are totally different feelings. So here's the biggest problem - to realize that we are all just human beings, and we all make mistakes. Some people are able to admit their own imperfection, but some continue persisting.

- ... These are not real bugs...

- ... It's easy to fix...

- ... It has worked for 5 years and nobody had any complaints.

Indeed, some errors are easy to fix, which gives hope. But it's not very easy to notice a typo or an error. Quite often a bug is detected not by a programmer, but a tester - or worse still - by a user.

The main aim of our analyzer is to help you find these errors and misprints. You would probably agree that one chooses Microsoft Word rather than Notepad, if there is a need to write a big portion of text. And the code of some programs is much bigger than that of some much-talked-of bestsellers.

PVS-Studio is, in fact, similar to the spell checker system from Microsoft Word for your code. It will give a hint to you, pointing to a place where you were in a hurry and made a typo, or formulated your thought in a way you hadn't intended. Correct expression of the thought in the text of a book is really important for a reader, and the logic formulation is important for the program user. By using PVS-Studio you will be able to express your ideas more accurately.

We wish you inspiration and sharp thinking! Please feel free to contact us.