Checking the source code of the PVS-Studio plugin with PVS-Studio

One of the eternal questions we face is this: “Have you checked PVS-Studio with PVS-Studio? Where is the article about the test results? ” Yes, we regularly do this, so we could not write an article about errors that we found in ourselves. Errors are corrected by developers at the stage of writing code, and we constantly forget at this moment to write them out. But readers are lucky this time. Due to an oversight, the C # plug-in code for Visual Studio was not added to the daily nightly checks we carry out. And, accordingly, unlike the analyzer core, errors in it were not noticed throughout the development of C # PVS-Studio. As they say, every cloud has a silver lining, and because of this you are reading this article.

A little more about testing PVS-Studio

It may be interesting for readers to find out how the PVS-Studio testing process is built in general. We have already written an article on this topic. But a lot of time has passed since then, and something has changed. Therefore, I will briefly describe how things are at the moment.

When developing PVS-Studio, we use seven testing methods:

1. Static code analysis on developer machines. All developers have PVS-Studio installed. New or changed code is immediately checked using the incremental analysis mechanism. C ++ and C # code is checked.
2. Static code analysis for nightly builds. If the warning was not noticed, it will be revealed at the stage of the night assembly on the server. PVS-Studio checks C # and C ++ code. In addition, we additionally use Clang to check C ++ code.
3. Unit tests at the level of classes, methods, functions. The system is not very developed, since many issues are difficult to test because of the need to prepare a large amount of input data for the test. We rely more on high-level tests.
4. Functional level tests of specially prepared and tagged files with errors.
5. Functional tests confirming that we correctly parse the main system header files.
6. Regression tests of the level of individual third-party projects and solutions (projects and solutions). The most important and useful type of testing for us. To implement it, we regularly check 105 open projects in C ++ and 49 in C #. Comparing the old and new results of the analysis, we control that something has not been broken and we are honing new diagnostic messages.
7. Functional tests of user interface extensions - add-in, integrated into the environment of Visual Studio.

How did it happen that we missed checking the plugin? We do not know. Just no one remembered to add the plugin code verification on the server. A new C # analyzer check was added, but not a plug-in. As a result, the C # analyzer developed and found errors in itself. And the plugin, also written in C #, stayed aside. It has not changed much lately. So it turned out that incremental analysis did not help, since the plug-in code almost did not work. And there were no night checks.

PVS-Studio Plugin

By virtue of your honesty and openness in front of customers and in order to avoid talking a la - "You see a speck in another's eye, and you don’t notice it in your log," we will write down all our typos and errors, even curious ones. We owe all the errors found to the PVS-Studio v6.02 analyzer, which supports C #.
')
Let's begin, perhaps, with real errors that have already been corrected by the time these lines are 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, accordingly, 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 is accustomed to think consistently - from simple to complex or, as in this case, from less to more, checking all values. In this case, such human logic led to the wrong behavior of the program. An error occurred while checking the number of rows in the table. The program, checking the first condition that there are more than 20,000 rows, will immediately assign the value to DatatableUpdateInterval in 30 seconds. And, of course, will not check the other conditions. Even if RowsCount was 1,000,000.

This code was written in order to optimize the display of error messages in the IDE PVS-Studio window in Visual Studio. Initially, the PVS-Studio plugin issued analysis results immediately upon receipt. That is, at the moment when these results came from the cmd process (which starts the analyzer core). However, on very large projects, we began to notice serious interface “brakes”. Especially problems appeared when there was a large number of * .c files in the projects. This happened because the * .c files were checked very quickly, and the UI stream was constantly busy redrawing the results table. It was decided to add a delay between updates, which would increase with an increase in the number of messages. Until reaching 20,000 messages in the list, the delay was set to 15 seconds.

In this case, we are lucky, and we get only a slight slowdown in the program (especially since we rarely receive more than hundreds of thousands of messages after verification), but this message is intended to detect more serious cases. For example, as it was in the project from the company Infragistics.

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

Here the condition will also always be true, which led to erroneous calculations.

The following error is more serious 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. Search for the substrings '/ arch: SSE' and '/ arch: SSE2'. StandaloneProjectItem.cs 229

Although the error has a different number, the essence of it is the same. The logic of man - to follow from simple to complex, pumped up here. By checking the value of “info.PreprocessorCommandLine” on the entry of the substring "/ arch: SSE" into it, we will simultaneously satisfy the condition in the case when the info.PreprocessorCommandLine "contains the following substring" / arch: SSE2 ". As you can see, the text, which is naturally read, does not correspond to the logic that we wanted to set in the program. Even if we know that the PreprocessorCommandLine contains "/ arch: SSE2", then passing the eyes through the code we will speculatively add to the ClangCommandLine variable the value "/ D \" _ M_IX86_FP = 2 \ "", and not "/ D \" _ M_IX86_FP = one\"";

From the point of view of the analyzer, the error was the incorrect definition of the _M_IX86_FP macro when the / arch: SSE2 flag was passed to the compiler. The point is that for preprocessing before directly analyzing, PVS-Studio uses Clang instead of cl (standard Visual C ++ preprocessor). Clang runs much faster. Unfortunately, the Clang C ++ language dialect support from Microsoft is still far from ideal - so if Clang "could not" preprocess something, PVS-Studio will call cl. Therefore, we will convert the cl compiler flags to define for Clang. Details

This error most likely never led to errors in the Clang operation or incorrect output of the analysis results, so it existed in our code for a very long time.

Another real error is calling the ProcessAnalyzerOutput function.

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

Noticing the error is not easy, even if you look at the function declaration:

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

The problem is the mismatch between the names of the function parameters and the names of the arguments that we pass there:

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

It is always difficult to find a discrepancy in such a long list of function parameters. Even IntelliSense in such cases does not always save. In large projects, it can be slowed down, and because of this, it is not always clear which element you are on now.



Because of this, a very unpleasant situation can occur. Although this diagnostics belongs to the third level, like all other heuristic ones, but it is very useful and you should not ignore it.

The place where this error was detected is a “stub” - the stderr and stdout parameters never received anything but empty lines. This error would find itself quickly enough when this stub began to be used.

Another error was detected by diagnostics number V3072, which is currently under development:

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

This diagnostic is intended for finding in a class that does not implement the IDisposable interface, fields that have a type that implements the IDisposable interface. Such code tells us that a programmer may forget to clear some resources after using an object of its class.

In this case, the ProcessingEngine class (which does not implement the IDisposable interface) has a field of the PVSMessageBase class, the type of which implements the IDisposable interface.

This operation can be attributed to false positive, caused by not quite “neat” architecture. The ProcessingEngine class is used in the program as singleton. And therefore, its only instance exists during the whole time the program is running, just like the PVSMessageBase field. Resources will be released upon completion of the program.

Well, in spite of ill-wishers and, to our delight, no other serious errors were found in the code. The rest of the "errors" rather refers to the format - "just in case."

For example, in this expression:

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

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

This code can be safely reduced to:

 if (CurrentRemClicks != 0 || DecrementCurrent) { 

Also found several "reinsurance":

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

And here once again rechecked the obvious 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

Well, if he began to reinsure, then we must hold to the end and check everything. Like, for example, here:

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

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

Sometimes the code comes to “very strange ghosts,” but if you promised to write about everything, then we write:

 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

The following checks are also found in our code:

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

V3027 The variable "CompileAsManaged" was used in the same logical expression. MSVCParamsGenerator.cs 801

Once again:

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

V3027 The variable "DisableLanguageExtensions" was used in the same logical expression. MSVCParamsGenerator.cs 1118

It is a mistake to check a variable for null after the Equals function was called. In fact, there is no real error here, because according to the API, the variables “CompileAsManaged” and “DisableLanguageExtensions” cannot be null. Therefore, checks can be simplified to:

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

Let's see where else we wrote the code, which received attention from the 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

Checking the variable buttons for equality MessageBoxButtons.OKCancel does not make sense because in any case, we will return DialogResult.OK. As a result, in our case, the code is reduced to:

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

And the last. Here, most likely, refactoring is to blame:

 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 code, you have different feelings. If the mistake is made with your own hand, then you try to correct it or find an excuse for it. If the other has a mistake, then you experience a completely different spectrum of feelings for it. This is the big problem of realizing that we are all human and we are all mistaken. Some of us admit it, and this is good, some persist:

“... these are not mistakes ...”

“... it's easy to fix ...”

- ... 5 years worked, and no one complained ...

Yes, indeed, many errors are easy to fix, and this is very good. But to notice this typo or error is very difficult. Often the error is noticed not by the programmer himself, but by the tester or, even worse, by the user.

The goal of our analyzer is to help you see these errors and typos. Agree, it is much better to write a large text in Microsoft Word than in notepad. And the program code is often more than some sensational bestsellers.

PVS-Studio, in fact, is the spelling system from Microsoft Word for your code. He will simply tell you that here you were hurried and sealed up, or, perhaps, expressed their thought not in the way they wanted. Agree - the correct formulation of thoughts in the text of the book is very important for the reader, and the correct formation of the program logic is important for the end user. Using PVS-Studio you will be able to express and formulate your idea much better.

We wish you inspiration and clear thought, and we will help you with this.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Vitaliy Alferov. Checking PVS-Studio plugin with PVS-Studio analyzer .