[cfe-dev] Clang Static Analyzer: False Positive Suppression Support

[Anna Zaks via cfe-dev]

While I agree with Alexander that it is best if users change their code to teach the analysis about it or make their intent more obvious by adding asserts or casts, unfortunately, not all static analyzer warnings could be suppressed that way. We do see requests for better suppression mechanisms come up from time to time. 

The main in-source suppression mechanism that the clang static analyzer supports is the ‘__clang_analyzer__’ macro. I am not sure if you tried using it or not... It might be more difficult to use than other in-code suppressions, might not be as esthetically appealing, and is definitely not something that could be used by other tools. However, this is what is recommended right now: http://clang-analyzer.llvm.org/faq.html#exclude_code <http://clang-analyzer.llvm.org/faq.html#exclude_code>, so I am curious what are the main limitations of it that you are seeing.
 
The static analyzer already has support for suppression hashes in tree. The CodeChecker tool (https://github.com/Ericsson/codechecker <https://github.com/Ericsson/codechecker>) is using them to provide user workflows such as baselining and issue suppression. The Ericsson team has sent an email to this list about their plans on adding the tool to the llvm codebase, so please, take a look at that thread (http://lists.llvm.org/pipermail/cfe-dev/2016-April/048538.html) and comment if you are interested! I hope they will start that process soon.

With regards to suppression using comments or pragmas, even though they are not an ideal way of suppressing path-sensitive errors coming from the analyzer, there seems to be a high demand for this feature, so I think it would be a valuable addition. However, there are caveats here that I’d like to raise:

1. I really dislike using the checker names in the suppressions or in user code. The are not user visible or discoverable, as someone has already mentioned, but moreover they were not designed to be user-friendly. We did not think about them as API, and unfortunately, they leaked out. (For one, the package names and how the packages are split up is not very good.) Also, there is no reason why we should suppress on checker granularity and not on a bug type granularity. I would suggest exploring the route of giving user-friendly and meaningful names to bug types and possibly using those when we need to specify the type of a bug to suppress. On the other hand, suppressing reports of a certain type is an icing on the cake and I think it should be done as a second step after we agree on how the generic suppression should work.

2. The second problem is related to suppressing path-sensitive checks at the line of the report. Unfortunately, Gabor did not go into details about how the suppressions would actually work, so I’ll try to fill in some gaps here. The simplest solution is to change the diagnostic engine not to show the report if it occurs on the line that is suppressed. That will not work very well because often the cause of the path-sensitive issue is not at the line where the bug is reported. Consider a leak example where the analyzer triggers a report of a leak on different paths when the object becomes dead. If the user wanted to suppress such a leak, they’d potentially have to suppress it in many places - along each path where the object is leaked. A potential solution here is to use a "uniquing location" of the report, which could either be the location of the report or something else, such as the allocation site. Once the suppressed report is generated, we’d store that uniquing location and suppress all other reports with the same uniquing location.

3. I am not convinced that analyzing after a suppressed bug is a desirable feature. It might result in unexpected analysis results or cascading warnings. Having this might be similar to not having sinks at all. We can experiment with relaxing the sinks in some cases, but much more investigation is needed here.

Thanks for pushing this forward!
Anna.

> On Aug 18, 2016, at 1:17 AM, Gábor Horváth <xazax.hun at gmail.com> wrote:
> 
> Hi!
> 
> In the CodeChecker tool (https://github.com/Ericsson/codechecker <https://github.com/Ericsson/codechecker>) we support suppressing false positive bugs as a post processing step. After all the issues were reported, CodeChecker filters out the suppressed ones, and those will not be displayed to the user. 
> 
> In our experience, however, and external tool only suppression suppport is not sufficient for certian reasons:
> * When a checker generates a sink node during analysis, the rest of the path will not be covered by the Static Analyzer. Unfortunately, if the user suppress the bug in an external tool, the coverage will not get better. This way false positive results can hide true positive results regardless of suppression.
> * The compiler could do better job diagnosing ill formed suppressions (invalid source range, typo in checker name etc).
> * Tools that are developed on top of the compiler need not to introduce their own customized solution for suppression. 
> 
> It is beneficial to have a suppression format that is standard across all clang tools. So the same format could be used to:
> * Suppress clang warnings
> * Suppress clang static analyzer warnings
> * Suppress clang tidy warnings
> 
> There are two main approaches to suppress warnings that I can think of right now:
> 
> Suppress using comments (or pragma):
> * This is a good solution for code evolution. The comments are moving together with the code. Edits are less likely to invalidate comments.
> * This is an intrusive solution, the source code needs to be changed to suppress an issue. This is both good, because the suppressions are version controlled and bad, because suppressing in 3rd party libraries might be problematic.
> * For path sensitive checks, it is not always self evident at which point of the path should a suppression comment be added. 
> 
> 
> Suppress using hashes:
> * It is hard to come up with a reliable hash to identify bugs. There always might be corner cases when the hashes of different bugs collide or the hash of a bug changes due to an edit that in fact should not affect the bug.
> * It is non intrusive.
> * The user do not need to think about at which point should the suppression comment be inserted.
> 
> 
> I would suggest the suppress comment road.
> The syntax could be something like:
> // clang suppress warning-or-checker-name [optional line offset][optional column range] [comment]
> 
> The warning-or-checker-name could be a regex, so multiple checks can be suppressed at the same time.
> Column ranges are useful when multiple errors are reported for the same line.
> Line offsets are useful when one do not want to break the flow of a multi line code snippet with a comment, so this way warnings could be suppressed without too much negative effects on the readability.
> Comment is for documentation purposes, the user can document why does she think that this is a false positive.
> 
> What do you think?
> 
> Regards,
> Gábor
> 
> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20160824/e4df66a1/attachment.html>