[llvm] r207743 - [LCG] Fix a bad bug in the new fancy iterator scheme I added to support

[David Blaikie]

On Thu, May 1, 2014 at 3:41 AM, Chandler Carruth <chandlerc at gmail.com> wrote:
> Author: chandlerc
> Date: Thu May  1 05:41:51 2014
> New Revision: 207743
>
> URL: http://llvm.org/viewvc/llvm-project?rev=207743&view=rev
> Log:
> [LCG] Fix a bad bug in the new fancy iterator scheme I added to support
> removal. We can't just blindly increment (or decrement) the adapted
> iterator when the value is null because doing so can walk past the end
> (or beginning) and keep inspecting the value. The fix I've implemented
> is to restrict this further to a forward iterator and add an end
> iterator to the members (replacing a member that had become dead when
> I switched to the adaptor base!) and using that to stop the iteration.
>
> I'm not entirely pleased with this solution. I feel like forward
> iteration is too restrictive. I wasn't even happy about bidirectional
> iteration. It also makes the iterator objects larger and the iteration
> loops more complex. However, I also don't really like the other
> alternative that seems obvious: a sentinel node. I'm still hoping to
> come up with a more elegant solution here, but this at least fixes the
> MSan and Valgrind errors on this code.

I hadn't noticed before, but it looks like you have a filtering
iterator - again, would be nice to have that as a generic abstraction
at some point. I know we have more than a few (specific_decl_iterator
in Clang, to name one off the top of my head) cases of this.

(& FWIW, http://blogs.msdn.com/b/vcblog/archive/2010/08/09/the-filterator.aspx
discusses how to write one and chooses the "store the current and end
iterator in the filtering iterator" strategy - there's no other
general solution, really)

>
> Modified:
>     llvm/trunk/include/llvm/Analysis/LazyCallGraph.h
>
> Modified: llvm/trunk/include/llvm/Analysis/LazyCallGraph.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/LazyCallGraph.h?rev=207743&r1=207742&r2=207743&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Analysis/LazyCallGraph.h (original)
> +++ llvm/trunk/include/llvm/Analysis/LazyCallGraph.h Thu May  1 05:41:51 2014
> @@ -115,17 +115,18 @@ public:
>    /// the graph.
>    class iterator
>        : public iterator_adaptor_base<iterator, NodeVectorImplT::iterator,
> -                                     std::bidirectional_iterator_tag, Node> {
> +                                     std::forward_iterator_tag, Node> {
>      friend class LazyCallGraph;
>      friend class LazyCallGraph::Node;
>
>      LazyCallGraph *G;
> -    NodeVectorImplT::iterator NI;
> +    NodeVectorImplT::iterator E;
>
>      // Build the iterator for a specific position in a node list.
> -    iterator(LazyCallGraph &G, NodeVectorImplT::iterator NI)
> -        : iterator_adaptor_base(NI), G(&G) {
> -      while (I->isNull())
> +    iterator(LazyCallGraph &G, NodeVectorImplT::iterator NI,
> +             NodeVectorImplT::iterator E)
> +        : iterator_adaptor_base(NI), G(&G), E(E) {
> +      while (I != E && I->isNull())
>          ++I;
>      }
>
> @@ -136,15 +137,7 @@ public:
>      iterator &operator++() {
>        do {
>          ++I;
> -      } while (I->isNull());
> -      return *this;
> -    }
> -
> -    using iterator_adaptor_base::operator--;
> -    iterator &operator--() {
> -      do {
> -        --I;
> -      } while (I->isNull());
> +      } while (I != E && I->isNull());
>        return *this;
>      }
>
> @@ -199,8 +192,10 @@ public:
>        return F;
>      };
>
> -    iterator begin() const { return iterator(*G, Callees.begin()); }
> -    iterator end() const { return iterator(*G, Callees.end()); }
> +    iterator begin() const {
> +      return iterator(*G, Callees.begin(), Callees.end());
> +    }
> +    iterator end() const { return iterator(*G, Callees.end(), Callees.end()); }
>
>      /// Equality is defined as address equality.
>      bool operator==(const Node &N) const { return this == &N; }
> @@ -358,8 +353,10 @@ public:
>    LazyCallGraph(LazyCallGraph &&G);
>    LazyCallGraph &operator=(LazyCallGraph &&RHS);
>
> -  iterator begin() { return iterator(*this, EntryNodes.begin()); }
> -  iterator end() { return iterator(*this, EntryNodes.end()); }
> +  iterator begin() {
> +    return iterator(*this, EntryNodes.begin(), EntryNodes.end());
> +  }
> +  iterator end() { return iterator(*this, EntryNodes.end(), EntryNodes.end()); }
>
>    postorder_scc_iterator postorder_scc_begin() {
>      return postorder_scc_iterator(*this);
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits