[PATCH] D62525: [Analyzer] Add new visitor to the iterator checkers

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

I don't see obvious red flags here, @NoQ?

In D62525#1523026 <https://reviews.llvm.org/D62525#1523026>, @baloghadamsoftware wrote:

> In D62525#1519868 <https://reviews.llvm.org/D62525#1519868>, @NoQ wrote:
>
> > In D62525#1519475 <https://reviews.llvm.org/D62525#1519475>, @baloghadamsoftware wrote:
> >
> > > Before someone asks: `NoteTag`s are not applicable here since invalidation and reaching one end of the range happens in many different places. This is also true for container emptiness.
> >
> >
> > Mm, what's wrong with many different places? If you're worried about code duplication, just put tag construction into a function (?)
>
>
> It is not about code duplication. Of course, that can be handled by a function.
>
> The problem is that the transition is added in the top level function but iterator position adjustments happen in the lower level ones. Data flow is one-way, top-down. For example, an insertion happens into a vector that invalidates all the iterators at and after the position where it is inserted. In this case `handleInsert()` calls a function that iterates all the active iterator positions and calls other one that invalidates the ones affected by the insertion. To propagate back the list of iterators invalidated would be too complex. Increments and decrements are even worse. Thus here the correct way seems to find the relevant transitions in a visitor instead of trying to figure out what note tags to add at each transition.


Mhm, I personally find this reasoning sufficient.



================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:281-282
+
+  // `FoundChange` becomes true when we find the statement the results in the
+  // current state of the iterator.
+  // `FoundEmptyness` becomes true when we find the block edge assuming
----------------
baloghadamsoftware wrote:
> NoQ wrote:
> > I don't think we should stop here. I think it's worth it to highlight *all* increments and decrements of the interesting iterator, so that it was clear how come that it has the given value.
> > 
> > Additionally, because iterators are copied around, it makes sense to "recursively" apply the visitor to the original object when the iterator is obtained as a copy (similarly to how `trackExpressionValue` works).
> This can be done but an iterator can reach the past-the-end or the first position by changing the container's size as well, not only by incrementing or decrementing the iterator itself.
Maybe in a followup patch, but I'd love to see a `trackIterator` function!


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1277-1292
+            State = setContainerData(State, ContReg, CData->newEnd(OldEndSym));
           } else {
             State = setContainerData(State, ContReg,
-                                     ContainerData::fromEnd(NewEndSym));
+                                     ContainerData::fromEnd(OldEndSym));
           }
+          // Then generate and assign a new "end" symbol for the old container.
+          auto NewEndSym =
----------------
baloghadamsoftware wrote:
> Szelethus wrote:
> > I'm confused, are these changes related to this patch? It doesn't seem to be.
> Yes, they are. Since we want to track the iterator positions upwards along the bugpath as far a possible I had to reverse one of my decisions. When I first decided it was really like a coin thrown up, but now it turned out I took the wrong choice considering the visitor. The change is that upon move assignment of a container the iterators are transferred to the new container, except the past-the-end iterator.  However we also have iterator positions relative to the past-the-end iterator (thus using the same symbol) which must be transferred. So I had to separate them from the past-the-end positions by generating a new end symbol. I originally used this new symbol for the positions to be transferred and kept the old symbol for the past-the-end positions. However this makes tracking of the non past-the-end positions very difficult so I reversed it and now I transfer the old symbol and use the new for the past-the-end positions.
Sometimes I just need a reality check to realize how absurdly difficult the problem is that you're solving... Wow. I mentioned this elsewhere, but do we not need a debug checker to validate that we argue about all of these correctly?


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1495-1496
 
   // For deque-like containers invalidate all iterator positions. For
   // vector-like containers invalidate iterator positions after the insertion.
   const auto *Cont = Pos->getContainer();
----------------
>>! In D62525#1523026, @baloghadamsoftware wrote:
> For example, an insertion happens into a vector that invalidates all the iterators at and after the position where it is inserted.

Is this actually correct?

https://en.cppreference.com/w/cpp/container/deque
> std::deque (double-ended queue) is an indexed sequence container that allows fast insertion and deletion at both its beginning and its end. In addition, insertion and deletion at either end of a deque never invalidates pointers or references to the rest of the elements.

https://en.cppreference.com/w/cpp/container/vector

> `insert`, `emplace`, `resize`: If the vector changed capacity, all of the iterators are invalidated. If not, only those after the insertion point.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62525/new/

https://reviews.llvm.org/D62525