To get a trial key
fill out the form below
Team License (standard version)
Enterprise License (extended version)
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
I am interested to try it on the platforms:
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
The Code Analyzer is wrong. Long live t…

The Code Analyzer is wrong. Long live the Analyzer!

Dec 01 2020
Author:

Combining many actions in a single C++ expression is a bad practice, as such code is hard to understand, maintain, and it is easy to make mistakes in it. For example, one can instill a bug by reconciling different actions when evaluating function arguments. We agree with the classic recommendation that code should be simple and clear. Now let's look at an interesting case where the PVS-Studio analyzer is technically wrong, but from a practical point of view, the code should still be changed.

0779_The_code_analyzer_is_wrong/image1.png

Order of arguments evaluation

What I'm going to tell you about is a continuation of the old story about the order of argument evaluation, which we wrote about in the article "How Deep the Rabbit Hole Goes, or C++ Job Interviews at PVS-Studio".

The brief gist is as follows. The order in which function arguments are evaluated is unspecified behavior. The standard does not specify the order in which developers of compilers are required to calculate arguments. For example, from left to right (Clang) or from right to left (GCC, MSVC). Before the C++17 standard, if side effects occurred when evaluating arguments, this could lead to undefined behavior.

With the advent of the C++17 standard, the situation has changed for the better. Now the evaluation of an argument and its side effects will only be performed after all the evaluations and side effects of the previous argument are performed. However, this does not mean that there is no room for errors now.

Let's look at a simple test program:

#include <cstdio>
int main()
{
  int i = 1;
  printf("%d, %d\n", i, i++);
  return 0;
}

What will this code print? The answer still depends on the compiler, its version, and its mood. Depending on the compiler, either "1, 1" or "2, 1"can be printed. Indeed, using the Compiler Explorer I will get the following results:

  • a program compiled using Clang 11.0.0 outputs "1, 1".
  • a program compiled with GCC 10.2 outputs "2, 1".

There is no undefined behavior in this program, but there is unspecified behavior (the order in which arguments are evaluated).

Code from the CSV Parser project

Let's go back to the code fragment from the CSV Parser project that I mentioned in the article "Checking a Header-Only C++ Library Collection (awesome-hpp)".

The analyzer and I, we know that arguments can be evaluated in different order. Therefore, the analyzer, and so did I considered this code to be erroneous:

std::unique_ptr<char[]> buffer(new char[BUFFER_UPPER_LIMIT]);
....
this->feed_state->feed_buffer.push_back(
    std::make_pair<>(std::move(buffer), line_buffer - buffer.get()));

PVS-Studio warning: V769 The 'buffer.get()' pointer in the 'line_buffer - buffer.get()' expression equals nullptr. The resulting value is senseless and it should not be used. csv.hpp 4957

In fact, we are both wrong, and there is no error. I'll tell about nuances further, let's start with a simple question.

Let's find out why it's dangerous to write code like this:

Foo(std::move(buffer), line_buffer - buffer.get());

I think you can guess the answer. The result depends on the order in which the arguments are evaluated. Let's look at this in the following synthetic code:

#include <iostream>
#include <memory>   

void Print(std::unique_ptr<char[]> p, ptrdiff_t diff)
{
    std::cout << diff << std::endl;
} 

void Print2(ptrdiff_t diff, std::unique_ptr<char[]> p)
{
    std::cout << diff << std::endl;
} 

int main()
{
    {
        std::unique_ptr<char[]> buffer(new char[100]);
        char *ptr = buffer.get() + 22;
        Print(std::move(buffer), ptr - buffer.get());
    }
    {
        std::unique_ptr<char[]> buffer(new char[100]);
        char *ptr = buffer.get() + 22;
        Print2(ptr - buffer.get(), std::move(buffer));
    }
    return 0;
}

Let's use the Compiler Explorer again and see the result of this program compiled by different compilers.

Clang 11.0.0 Compiler. Result:

23387846
22

GCC 10.2 compiler. Result:

22
26640070

The result is as expected. But one just can't write like this. This is what the PVS-Studio analyzer warns us about.

I wish I could put an end to this, but everything is a little more complicated. The fact is that we are talking about passing arguments by value, whereas when instantiating the template of the std::make_pair function, everything will be different. So, we will continue to dive into the subtleties and find out why PVS-Studio is wrong in this case.

std::make_pair

Let's refer to the cppreference site and see how the template for the std::make_pair function was changing.

Until C++11:

template< class T1, class T2 >

std::pair<T1,T2> make_pair( T1 t, T2 u );

Since C++11, until C++14:

template< class T1, class T2 >

std::pair<V1,V2> make_pair( T1&& t, T2&& u );

Since C++14:

template< class T1, class T2 >

constexpr std::pair<V1,V2> make_pair( T1&& t, T2&& u );

As you can see, once upon a time std::make_pair was taking arguments by value. If std::unique_ptr had existed at that time, then the code above would have been indeed incorrect. Whether this code would work or not would be a matter of luck. In practice, of course, this situation would never have occurred, since std::unique_ptr appeared in C++11 as a replacement for std::auto_ptr.

Let's go back to our time. Starting with C++11, the constructor started to use move semantics.

There is a subtle point here that std::move does not actually move anything, but only converts the object to an rvalue reference. This allows std::make_pair to pass a pointer to the new std::unique_ptr, leaving nullptr in the original smart pointer. But passing of this pointer won't happen until we get inside std::make_pair. By then, we will have evaluated line_buffer - buffer.get(), and everything will be fine. In other words, a call to the buffer.get() function can't return nullptr at the moment it is evaluated, regardless of when exactly this happens.

I apologize for the complicated description. The bottom line is that this code is quite correct. And in fact, the PVS-Studio static analyzer gave a false positive in this case. However, our team is not sure if we should rush to make changes to the analyzer's logic for such situations.

The King is dead. Long live the King!

We found out that the warning described in the article was false. Thanks to one of our readers who drew our attention to the implementation of std::make_pair.

However, this is the case when we are not sure that we should improve the behavior of the analyzer. The fact is that this code is too confusing. You have to admit that the code above does not deserve such a detailed investigation resulting in the entire article. If this code requires so much attention, it is janky code.

It is fitting here to recall the article "False positives are our enemies, but may still be your friends". The post is not ours, but we agree with it.

This is probably the case. The warning may be false, but it points to a better place to refactor. The author can just write something like this:

auto delta = line_buffer - buffer.get();
this->feed_state->feed_buffer.push_back(
  std::make_pair(std::move(buffer), delta));

In this case, one can make the code even better by using the emplace_back method:

auto delta = line_buffer - buffer.get();
this->feed_state->feed_buffer.emplace_back(std::move(buffer), delta);

Such code creates the final std::pair object in the container "in place", bypassing creating a temporary object and moving it to the container. By the way, the PVS-Studio analyzer offers to make such a replacement by issuing the V823 warning from the set of rules for code micro-optimizations.

The code will definitely become easier and clearer for any reader and analyzer. There is no merit in putting as many actions as possible in one line of code.

Well, in this case there is no error due to pure chance. Still it is unlikely that the author kept in mind all that we discussed when writing this code. Most likely, it was luck that played its part. And other times one might not be that lucky.

Conclusion

So, we figured out that there is no real error. The analyzer issues a false positive. We may or may not remove the warning for such cases. We'll think over it. After all, this is a fairly rare case. The code where arguments are evaluated with side effects is generally dangerous, and it is better not to allow it. It is worth refactoring at least for preventive purposes.

The code as the following:

Foo(std::move(buffer), line_buffer - buffer.get());

can be easily crashed by changing something else in the program. This code is hard to maintain. It is also nasty because it may give you the false impression that everything is working correctly. In fact, this is just a set of circumstances, and everything can crash when changing a compiler or optimization settings.

Write simple code!

Popular related articles
Appreciate Static Code Analysis!

Date: Oct 16 2017

Author: Andrey Karpov

I am really astonished by the capabilities of static code analysis even though I am one of the developers of PVS-Studio analyzer myself. The tool surprised me the other day as it turned out to be sma…
The way static analyzers fight against false positives, and why they do it

Date: Mar 20 2017

Author: Andrey Karpov

In my previous article I wrote that I don't like the approach of evaluating the efficiency of static analyzers with the help of synthetic tests. In that article, I give the example of a code fragment…
Free PVS-Studio for those who develops open source projects

Date: Dec 22 2018

Author: Andrey Karpov

On the New 2019 year's eve, a PVS-Studio team decided to make a nice gift for all contributors of open-source projects hosted on GitHub, GitLab or Bitbucket. They are given free usage of PVS-Studio s…
PVS-Studio ROI

Date: Jan 30 2019

Author: Andrey Karpov

Occasionally, we're asked a question, what monetary value the company will receive from using PVS-Studio. We decided to draw up a response in the form of an article and provide tables, which will sho…
Technologies used in the PVS-Studio code analyzer for finding bugs and potential vulnerabilities

Date: Nov 21 2018

Author: Andrey Karpov

A brief description of technologies used in the PVS-Studio tool, which let us effectively detect a large number of error patterns and potential vulnerabilities. The article describes the implementati…
The Evil within the Comparison Functions

Date: May 19 2017

Author: Andrey Karpov

Perhaps, readers remember my article titled "Last line effect". It describes a pattern I've once noticed: in most cases programmers make an error in the last line of similar text blocks. Now I want t…
How PVS-Studio Proved to Be More Attentive Than Three and a Half Programmers

Date: Oct 22 2018

Author: Andrey Karpov

Just like other static analyzers, PVS-Studio often produces false positives. What you are about to read is a short story where I'll tell you how PVS-Studio proved, just one more time, to be more atte…
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: Jul 31 2017

Author: Andrey Karpov

After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many erro…
Static analysis as part of the development process in Unreal Engine

Date: Jun 27 2017

Author: Andrey Karpov

Unreal Engine continues to develop as new code is added and previously written code is changed. What is the inevitable consequence of ongoing development in a project? The emergence of new bugs in th…
PVS-Studio for Java

Date: Jan 17 2019

Author: Andrey Karpov

In the seventh version of the PVS-Studio static analyzer, we added support of the Java language. It's time for a brief story of how we've started making support of the Java language, how far we've co…

Comments (0)

Next comments

This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept