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

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:211-214
+std::pair<SymbolRef, llvm::APSInt>
+createDifference(SymbolManager &SymMgr, SymbolRef Sym1, SymbolRef Sym2);
+const llvm::APSInt *getDifference(ProgramStateRef State, SymbolRef Sym1,
+                                  SymbolRef Sym2);
----------------
NoQ wrote:
> These functions are currently unused; my compiler warns about that, and buildbots would probably yell at us when we commit it, so i guess it's better to add them when they are actually used; they'd also be tested as well.
Sorry, I forgot to delete it. I will do it.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:219
+  OutOfRangeBugType.reset(
+      new BugType(this, "Iterator of out Range", "Misuse of STL APIs"));
+  OutOfRangeBugType->setSuppressOnSink(true);
----------------
NoQ wrote:
> Before we forget: Range -> range. As we've noticed recently in D32702 :), we don't capitalize every word in bug types and categories.
Agree.


================
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.
----------------
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.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:478-479
+    auto &SymMgr = C.getSymbolManager();
+    EndSym = SymMgr.conjureSymbol(CE, C.getLocationContext(),
+                                  C.getASTContext().LongTy, C.blockCount());
+    State = createContainerEnd(State, ContReg, EndSym);
----------------
NoQ wrote:
> I see what you did here! And i suggest considering `SymbolMetadata` here instead of `SymbolConjured`, because it was designed for this purpose: the checker for some reason knows there's a special property of an object he wants to track, the checker doesn't have a ready-made symbolic value to represent this property (because the engine didn't know this property even exists, so it didn't denote its value), so the checker comes up with its own notation. `SymbolMetadata` is used, for example, in `CStringChecker` to denote an unknown string length for a given null-terminated string region.
> 
> This symbol carries the connection to the region (you may use it to reverse-lookup the region if necessary), and in particular it is considered to be live as long as the base region is live (so you don't have to deal with liveness manually; see also comments around `SymbolReaper::MarkInUse`). It's also easier to debug, because we can track the symbol back to the checker. Generally, symbol class hierarchy is cool because we know a lot about the symbol by just looking at it, and `SymbolConjured` is a sad fallback we use when we didn't care or manage to come up with a better symbol.
> 
> I'm not sure if `LongTy` is superior to `IntTy` here, since we don't know what to expect from the container anyway.
> 
> Also, please de-duplicate the symbol creation code. Birth of a symbol is something so rare it deserves a drum roll :)
> 
> I'd take a pause to figure out if the same logic should be applied to the map from containers to end-iterators.
SymbolMetaData is bound to a MemRegion. Iterators are sometimes symbols and sometimes memory regions, this was one of the first lessons I learned from my first iterator checker.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:513
+
+bool isLess(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2);
+bool isGreaterOrEqual(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2);
----------------
NoQ wrote:
> One more unused function.
OK.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:515-516
+bool isGreaterOrEqual(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2);
+bool compareToZero(ProgramStateRef State, const NonLoc &Val,
+                   BinaryOperator::Opcode Opc);
+bool compare(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2,
----------------
NoQ wrote:
> One more unused function.
I thought I removed it.


https://reviews.llvm.org/D32592