This is another story about programs having a hard time trying to interact with the external world. At first glance, a static analyzer should face no problems at all. It just gets files and some additional information at the input and generates a log-file out of it. But the Devil is, as usual, in the detail.
I find PVS-Studio a very high-quality product. We can build and upload a fresh distribution on almost any day. We use a huge variety of automated tests of different levels and types. Here's a description of some of them: "How we test the code analyzer". Now we've got even more tests. For instance, for the purpose of static analysis, we have recently started using Clang in addition to our own analyzer. If a fixed version has passed all the tests, then we know we can feel free and sure to share it with users.
Unfortunately, all the beauty and security of internal code sometimes get spoiled and fall apart because of the influences of hostile environment. It results in the users' impression of the product to get spoiled too. Although it's not actually us who are to blame but it is our product that doesn't work, after all. I can name numbers of examples. This is what comes to my mind first of all:
Now I will tell you another such story about how easy it is sometimes to spoil an impression of our product even when we are innocent.
One of our potential users sent us a question about PVS-Studio's strange behavior:
We are currently testing the trial version and think of buying the full one. But, you know, we've come across one thing when running the analysis which makes us doubt that the analyzer's output is correct.
Below is a screenshot with the error.
filePath and cachePath are marked as non-used (warning V808) though you can see that they are really used right in the next line after the declaration.
Could you please explain this behavior of the analyzer?
In the screenshot, there is code looking similar to the following one (I've changed the original code):
std::string Foo()
{
std::string filePath(MAX_PATH + 1, 0);
std::string cachePath = "d:\\tmp";
if (!GetTempFileName(cachePath.c_str(), "tmp", 0,
&filePath.front()))
throw MakeSystemError("...", GetLastError(), __SOURCE__);
return std::move(filePath);
}What can I say about that? Shame on the analyzer. It really outputs some nonsense. The filePath and cachePath variables are definitely being used. There's just no reason for the warning at all. I'd understand it if the function were 1000 lines long, but it is really awfully simple.
That's it. The first impression is spoiled. Now I'll tell you about the results of the investigation we've done.
The PVS-Studio analyzer uses either the Visual C++ (CL.exe) or Clang compiler to preprocess files. To learn more about how we use Clang, see the post: "A few words about interaction between PVS-Studio and Clang".
The Visual C++ compiler's preprocessor works well yet too slow. On the contrary, Clang works fast but doesn't support many features or works incorrectly. Clang's developers claim that their tool is highly compatible with Visual C++, but that's not true. There are a lot of subtle features they don't support or do it differently than Visual C++ does. These subtleties may be fatal for the analyzer - and that was exactly the case this time.
The PVS-Studio analyzer by default tries to preprocess a file with Clang at first. However, it is aware that Clang is far not always capable of preprocessing what Visual C++ can. So if a preprocessing error occurs, CL.exe is launched. This way, we have to waste some time on a useless launch of Clang but in general this practice helps save much time when generating *.i files.
It didn't work in this case. Clang had "successfully" preprocessed the file although its output contained some rubbish.
What had been the source of its incorrect behavior was the __SOURCE__ macro declared in the following way:
#define __SLINE_0__(_line) #_line
#define __SLINE__(_line) __SLINE_0__(_line)
#define __SOURCE__ __FILE__":"__SLINE__(__LINE__)When preprocessing the line:
throw MakeSystemError(_T("GetTempFileName"), GetLastError(),
__SOURCE__);It should be turned into:
MakeSystemError("GetTempFileName", GetLastError(),
"..path.."":""37");And this is the way the Visual C++ compiler would do and all would be fine - the analyzer would correctly process this code.
If you explicitly set PVS-Studio to always use CL.exe, false messages will disappear. But if Clang is launched, the analyzer will be dealing with incorrect code.
Clang can't manage macros the right way, and so what we've got at the output is the following:
throw MakeSystemError("GetTempFileName", GetLastError(),
"..path.."":"__SLINE__(37));The __SLINE__ macro has been left unexpanded.
Thus, we've got an incorrect construct invalid from the viewpoint of the C++ language. On stumbling across it, PVS-Studio tries to pass by an incorrect code and continue with the analysis. It's just that you'd better skip one or two things than fail to process an entire file. Such skips don't usually affect the analysis results in any way.
But this time, the analyzer didn't manage to pass by the incorrect fragment safe. It resulted in the whole text block having been thrown away:
if (!GetTempFileName(cachePath.c_str(), "tmp", 0, &filePath.front()))
throw MakeSystemError("....", GetLastError(), __SOURCE__);
return std::move(filePath);It just happened that way... The analyzer did it best and can't be blamed.
Since this fragment doesn't exist from the analyzer's viewpoint, it assumes that the variables are not initialized and used in any way, too. This is the reason why the tool generates the false positive.
One of the ways to solve the problem is to always use Visual C++'s preprocessor. But then you will have to deal with its only drawback - slow analysis.
That's why we took another path this time. Since the company that has contacted us is about to purchase PVS-Studio, we have examined this private case and implemented another workaround in the code. It doesn't look nice yet it is practical. We already have a lot of different special fragments in the code designed to circumvent certain subtleties that can be found in our users' projects. This is kind of technical support.
So, this time we were failed by Clang's preprocessor. I wonder, what will cause me to write another article about external errors next time?
That's it. Thanks for reading.
Welcome to try our static code analyzer on your projects and if you have any problems, write to us. We are good at turning one's bad mood into good one.