This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Turn suppress-c++-stdlib on by default
ClosedPublic

Authored by zaks.anna on Mar 9 2017, 4:39 PM.

Details

Summary

We have several reports of false positives coming from libc++. For example, there are reports of false positives in std::regex, std::wcout, and also a bunch of issues are reported in https://reviews.llvm.org/D30593. In many cases, the analyzer trips over the complex libc++ code invariants. Let's turn off the reports coming from these headers until we can re-evalate the support.

We can turn this back on once we individually suppress all known false positives and perform deeper evaluation on large codebases that use libc++. We'd also need to commit to doing these evaluations regularly as libc++ headers change.

Diff Detail

Repository
rL LLVM

Event Timeline

zaks.anna created this revision.Mar 9 2017, 4:39 PM
dcoughlin accepted this revision.Mar 9 2017, 4:44 PM

LGTM,

This revision is now accepted and ready to land.Mar 9 2017, 4:44 PM
This revision was automatically updated to reflect the committed changes.

I've committed the change, but would very much appreciate community feedback here if if there is any!

I've committed the change, but would very much appreciate community feedback here if if there is any!

I agree with the change. Users are usually not interested in the results from the standard library, and since the standard library supposed to be well tested, it is very unlikely that the analyzer could find a true positive there. One side effect would be the suppression of results that materialize due to the misuse of those APIs.

I have no any objection on this change.

NoQ added a subscriber: NoQ.Mar 10 2017, 6:35 AM

I'd like to clarify that in case of path-sensitive analysis there are actually three warning classes to consider.

  1. Warnings that reside completely in system headers and indicate bugs in system headers, most likely falsely.
  2. Warnings that originate from the main file and indicate misuse of system header APIs, likely correctly.
  3. Warnings that originate from the main file, but still indicate bugs in system headers, because the part of the path that stays in the main file is irrelevant to the warning.

This change suppresses 3, and as a side effect we suppress 1, because discriminating between 1 and 3 is hard* . We never needed to suppress 2, because they're never created to begin with: AnalysisConsumer doesn't consider header-only functions for top-level analysis.
__
// * I think that actually such discrimination is curious and probably worth investigating. We could try to see which symbols (originating from where) participate in the equation that expresses the invariant violation of which causes the warning. We could potentially improve our suppressions greatly (maybe even make them non-hackish) and probably avoid the common problems with inlining this way. Just brainstorming.

Correct, this will suppress some valid warnings that occur due to user errors and valid warnings coming from the standard library.

However, I believe this is the right choice right now since we know that the analyzer is not currently effective in understanding invariants coming from libc++ headers and users have no ability to help the analyzer (ex: by inserting assertions). This results in a wave of false positives every time the headers change. Furthermore, we do not perform sufficient evaluation and testing for every new version of the headers.