[cfe-dev] [analyzer] alpha checkers

[paul via cfe-dev]

Moving discussion to the list:

On Sat, 2018-05-05 at 00:30 +0000, Artem Dergachev via Phabricator wrote:
> NoQ added a comment.
> 
> 
> ~*sorry for hijacking review with this, we should move this conversation to
> the list*~
> 
> In https://reviews.llvm.org/D46159#1088768, @pfultz2 wrote:
> 
> > 
> > I am definitely interested in working on getting some of the checkers
> > stable, and have already started some work on expanding the
> > ConversionChecker, but would like user feedback as I move forward.
> 
> Yay! I would most likely have a chance to help out with final evaluation of
> checkers before they move out of alpha.
> 
> In https://reviews.llvm.org/D46159#1088768, @pfultz2 wrote:
> 
> > 
> > Some like the Conversion checker doesn't seem to have any issues(because
> > its fairly limited at this point).
> 
> Last time i tried it, it was very hard to understand positives produced by
> the checker. It seems that it needs to implement a relatively sophisticated
> "bug visitor" in order to explain why does it think that a certain
> conversion would overflow by highlighting the respective spot somewhere in
> the middle of the path. It is especially important when the value comes from
> an inlined function because the analyzer wouldn't draw paths through all
> inlined functions (it would have been overwhelming) unless something
> interesting happens in there.

Initially, I am thinking of showing the path for a variable(ie DeclRef) in the
expression, which should be better than showing the path for the implicit
cast. For multiple variables, perhaps trying to show the variable with the
longest path, as perhaps that is more interesting. 

Although, there could be cases were the interesting path is from something
else. 

> 
> Then somebody would need to have a look at intentional overflows and if it's
> possible to avoid them.

I am considering not warning when its an integral constant expression. There
are too many places where the user explicitly underflows. Instead a clang-tidy 
check or warning could provide a check for such conversions instead.

> 
> In https://reviews.llvm.org/D46159#1088768, @pfultz2 wrote:
> 
> > 
> > Others like StreamChecker probably just needs some simple fixes.
> 
> Hmm, at a glance, it looks almost empty.
> 
> 1. Again, it needs a bug visitor to highlight where the file was open.
> Otherwise many reports may become impossible to understand.
> 2. It is eagerly splitting states upon file open, which doubles remaining
> analysis time on every file open. This needs to be deferred until an actual
> branch is found in the code.
> 3. It needs a pointer escape callback, in order to work with wrappers around
> open/close functions. This needs to be complemented with a certain knowledge
> about the standard library, to see what functions should not cause escapes.
> SimpleStreamChecker has an example.
> 4. If it's ever to support `open()`/`close()`, it would, apart from other
> things, also need an "integer escape" callback , which will require a bit of
> digging into the analyzer core.
> 
> Pt.4 is mostly about feature work, but Pt.1–3 are must-fix. This isn't an
> overwhelming amount of work, but it will take some time.
> 
> Otherwise, yeah, it's a good simple check to have. It could be expanded to
> support a variety of weird APIs such as `fdopen()` but it's also a checker
> that's hard to get wrong. PthreadLockChecker is also a great candidate with
> similar problems; i have some unfinished work on that on here on the
> phabricator.
> 
> 
> https://reviews.llvm.org/D46159
> 
> 
>