[PATCH] D32592: [Analyzer] Iterator Checker - Part 1: Minimal Checker for a Simple Test Case

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Sorry, got carried away by GSoC and critical stuff...

In https://reviews.llvm.org/D32592#749613, @baloghadamsoftware wrote:

> In https://reviews.llvm.org/D32592#747132, @NoQ wrote:
>
> > Then, in methods that deal with iterator `SVal`s directly, i wish we had hints explaining what's going on in these ~7 cases. In my opinion, that'd greatly help people understand the code later, and it'd help us understand how to avoid this variety and provide checker authors with a better API as soon as we get to this, so it's the biggest concern for me about this checker.
>
>
> I am not sure which method do you mean? I think here the crucial functions are the setIteratorPosition() and the getIteratorPosition() functions which provide a common way to handle all these SVals.


Hmm, i think i mostly mean `evalAssume()` and the comparisons mechanism. You're having multiple cases, like:
(1) if the assumed value is a BinarySymExpr for which there's a stored comparison (good for type-II iterators),
(2) if it's a symbol that was conjured by a conservatively evaluated comparison operator (here you lookup the opcode from the referenced expression) for which there's a stored comparison (good for type-III iterators),
(3) if it's a binary symbolic expression of form ($x == 0) or ($x != 0) where $x is one of (1) or (2).

I believe that the whole idea behind storing comparison results should have high-level comments explaining how it works (being tricky but solid - essentially it extends the constraint manager, taking over when handling iterator constraints, which sounds like the right thing to do).

I think we should land after that; i only have comment-related comments now :)



================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:131
+
+// Strcutre fo recording iterator comparisons. We needed to retrieve the
+// original comparison expression in assumptions.
----------------
Strcutre -> Structure :)


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:294-297
+    // Assumption: if return value is an iterator which is not yet bound to a
+    //             container, then look for the first iterator argument, and
+    //             bind the return value to the same container. This approach
+    //             works for STL algorithms.
----------------
baloghadamsoftware wrote:
> NoQ wrote:
> > baloghadamsoftware wrote:
> > > NoQ wrote:
> > > > I guess this deserves a test case (we could split this out as a separate feature as well).
> > > > 
> > > > I'm also afraid that we can encounter false positives on functions that are not STL algorithms. I suggest doing this by default only for STL functions (or maybe for other specific classes of functions for which we know it works this way) and do this for other functions under a checker option (i.e. something like `-analyzer-config IteratorChecker:AggressiveAssumptions=true`, similar to `MallocChecker`'s "Optimistic" option).
> > > I will check whether this piece of code could be moved in a later part of the checker. However, I suggest to first wait for the first false positives before we introduce such an option. This far the false positives in my initial tests had different reasons, not this one.
> > Unfortunately, we've had a poor experience with this approach in other checkers. You never know, and it seems that it's always better to have a safe fallback mode available under a flag, because if a few classes of false positives are found, and we are forced to reduce the checker to a safer behavior, it'd be hard to remember all the places where unsafe heuristics were used.
> I think this heuristic is well marked by the comment, easy to find if it causes false positives. When I started working on Clang (Tidy first) reviewers discouraged me to add options before experiencing false positives.
Well, it's a bit sad to say, but that's one of the things that we didn't agree upon with clang-tidy, i guess; historically, they started as a lint-like tool that produces nice-to-fix-anyway kinds of warnings (having nice bug-finding checks nowadays), and we always kept positioning ourselves as a "quiet" tool that reports only real critical bugs for users that never would be annoyed immensely by every single false positive (doesn't make this true though), and it makes us nervous about every single potential false positive class.

I guess we could leave that as a task for later, with a "FIXME: Add a more conservative mode".


================
Comment at: test/Analysis/iterator-range.cpp:1-2
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-eagerly-assume -analyzer-config c++-container-inlining=false %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-eagerly-assume -analyzer-config c++-container-inlining=true -DINLINE=1 %s -verify
+
----------------
baloghadamsoftware wrote:
> NoQ wrote:
> > Could we add run-lines without `-analyzer-eagerly-assume`? Currently all variants are passing, but if new tests will fail, we could `#ifndef` them out.
> In my first checker Anna suggested to always use this option. She also wrote that she plans to remove possibility to execute the Analyzer without eagerly assume.
Ouch, right. Suffering from short-term memory loss :)


https://reviews.llvm.org/D32592