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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
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);
----------------
baloghadamsoftware wrote:
> 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.
Oh. Hmm. Ok. Right.

To be sure: in what cases do you need to create a new symbol when the iterator is already a symbol? How broken do we become if we try to say that the symbolic iterator and its own offset are the same thing?


https://reviews.llvm.org/D32592