Page MenuHomePhabricator

[analyzer] Analysis: Prevent bitwise operation false positives
Needs ReviewPublic

Authored by Charusso on Sun, Aug 25, 11:50 AM.

Details

Reviewers
NoQ
Summary

This patch introduces a new analyzer-config configuration:
-analyzer-config check-bitwise=true
which could be used to toggle whether the bitwise (and shift) operations
should be checked. It by default set to true.

Diff Detail

Event Timeline

Charusso created this revision.Sun, Aug 25, 11:50 AM
NoQ added a comment.Sun, Aug 25, 1:25 PM

No-no, you're disabling the checkers here but you should be silencing them. This will be crashing because modeling is disabled.

I also recommend checker options, even if they apply to multiple checkers (@Szelethus mentioned that package options are a thing, you might use these if you don't want to make multiple options).

clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
83–89

Ugh. This part is also wrong for the same reason.

NoQ added inline comments.Sun, Aug 25, 1:30 PM
clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
305

We didn't do enough debugging to demonstrate that the problems are specific to the checker. I suggest we keep this on by default and only disable it on LLVM as an overfitting effort.

Charusso updated this revision to Diff 217062.Sun, Aug 25, 1:43 PM
Charusso marked 4 inline comments as done.
Charusso edited the summary of this revision. (Show Details)
  • Fix.
clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
305

Well, okay.

clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
83–89

If I do not return here then clang/test/Analysis/uninit-vals.m:113: Returning from 'swap' related notes are gone, so I will leave it untouched.

NoQ added a comment.Sun, Aug 25, 1:51 PM

Now you're silencing the *whole* checker. This is equivalent to passing an -analyzer-silence-checker flag.

In D66721#1644531, @NoQ wrote:

Now you're silencing the *whole* checker. This is equivalent to passing an -analyzer-silence-checker flag.

I am silencing the *whole* checker if and only if a bitwise operation or a shift operation occurs. That is the correct behavior which is not equal to -analyzer-silence-checker which disables the *whole* checker during the *whole* analysis.

In D66721#1644514, @NoQ wrote:

No-no, you're disabling the checkers here but you should be silencing them. This will be crashing because modeling is disabled.

I also recommend checker options, even if they apply to multiple checkers (@Szelethus mentioned that package options are a thing, you might use these if you don't want to make multiple options).

Just to be clear, I object to nothing as long as it is an off-by-default developer-only feature.

clang/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp
66

This is one of the reasons why I believe AnalyzerOptions should be immutable once analysis begins. Say that you'd like to list the checkers currently enabled with -analyzer-list-enabled-checkers, and that list is compiled around the time CheckerRegistry is active (which is a very brief period during the initialization of CheckerManager). Wouldn't it be super infuriating to learn that this lislt may change *during* analysis?

I feel somewhat the same way about this, this should either be moved to shouldRegister*() or be handled outside the analyzer.

NoQ added a comment.Sun, Aug 25, 4:47 PM

Until we do enough debugging to understand the nature of these false positives, i recommend silencing the checker when it has false positives. Later we can improve the granularity of our checkers, so that to be able to disable smaller portions of them, or make constraint solver improvements, depending on what we see.

MaskRay added inline comments.
clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
308

typo: unsigned

NoQ added inline comments.Sun, Aug 25, 6:02 PM
clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
308

Yup, see also :)