Introduction
At RedHat we use cppcheck and other static and dynamic analysis software to scan releases of the Linux distributions Fedora and RHEL, and other projects. The project that does this is called OpenScanHub. In a recent report for Fedora 44, we saw these stats for LLVM 21.1:
| count | warning_type | severity |
|---|---|---|
| 52 | CLANG_WARNING | warning |
| 4 | COMPILER_WARNING | error[error] |
| 12 | COMPILER_WARNING | error[fatal error] |
| 2 | COMPILER_WARNING | warning[-Wdeprecated-declarations] |
| 1 | CPPCHECK_WARNING | error[autovarInvalidDeallocation] |
| 4 | CPPCHECK_WARNING | error[ctunullpointer] |
| 2 | CPPCHECK_WARNING | error[ctuuninitvar] |
| 1 | CPPCHECK_WARNING | error[invalidContainer] |
| 4 | CPPCHECK_WARNING | error[legacyUninitvar] |
| 1 | CPPCHECK_WARNING | error[memleakOnRealloc] |
| 3 | CPPCHECK_WARNING | error[missingReturn] |
| 1 | CPPCHECK_WARNING | error[nullPointerArithmetic] |
| 1 | CPPCHECK_WARNING | error[returnDanglingLifetime] |
| 5 | CPPCHECK_WARNING | warning[nullPointerOutOfMemory] |
| 5 | CPPCHECK_WARNING | warning[nullPointer] |
| 4 | CPPCHECK_WARNING | warning[uninitvar] |
| 4 | UNICONTROL_WARNING | warning |
Proposal
I propose to address these findings upstream so that anybody who runs cppcheck does benefit from them. Not to mention the hardening effect for LLVM itself. Cppcheck is quite old but is still actively developed.
An exemplary upstream PR for 5 false-positives (4x error[legacyUninitvar] and 1x warning[uninitvar]) can be found here. This PR uses inline comments to suppress (filter out) certain errors (false-positives in this case).
I’m not saying that everything from the stats table above is a false-positive or an error to be dealt with upstream in LLVM. Some findings come from CMake’s temporary compilations and don’t have to be dealt with upstream for example.
What I’m explicitly asking for is to allow source code comments (aka inline suppressions) that mark false-positives identified by the cppcheck tool.
Why?
Generally, I think that dealing with reports from static analysis tools is a good idea. The best thing that can happen is that the number of false-positive reports is reduced and actual errors show up more prominently.
Potential issues
Rotten comments
Oliver Stöneberg [aka @firewave on github] is an active contributor to cppcheck and he mentioned that inline comments might rot over time. He also mentioned that you can run cppcheck with a flag that shows you when there’s an inline supression in code that is not needed so to me this somewhat invalidates the fear of having rotten comments.
I argue that this is essentially the same with every comment in C/C++ source code for as long as it is not processed by some tool and validated.
I first thought that worst thing that can happen is that a similar error is introduced when the line below the comment is changed and then hidden by the comment that is still kept. But luckily cppcheck allows you to specify to which a particular symbol a suppression is meant.
You can always look at the state of the file when the comment was introduced so it should be doable to analyze what the comment was initially meant for.
Openening the gates for other tools
By introducing comments meant to be consumed by cppcheck we potentially open the gates for other tools that do static analysis and need special source code annotations. If this is the case, we should actually be glad that people check LLVM. I want to defer the discussion of changing to one of the many other suppression mechanism that cppcheck offers (plain text, XML, etc.).
To be fair I don’t know the exact difference between cppcheck and the clang static analyzer.