[PATCH] D29183: [Analysis] Fix for call graph to correctly handle nested call expressions

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

baloghadamsoftware added a comment.

Some comments how I found this bug. I got some strange false positives even after the fix for the iterator past end checker (https://reviews.llvm.org/D28771). I could reproduce it with the following code:

  #include <list>
  
  using std::list;
  using std::prev;
  
  namespace {
  
    class Strange {
    public:
      Strange(list<int> &l) : Back(prev(l.end())), Item(*Back) {}
      void reset(list<int> &l) { Back = prev(l.end()); Item = *Back; }
      int get1(list<int> &l) { Back = prev(l.end()); return *Back; }
      int get2(list<int> &l) { list<int>::iterator i; i = prev(l.end()); return *i; }
      
    private:
      list<int>::iterator Back;
      int Item;
    };
  
  }

This example was analyzed without errors, but if I removed get2(), I got iterator past end errors for every other function. Even more strange was that I could not reproduce this with std::vector, just with std::list. I started a long debugging session (days) and it turned out, that if I delete get2(), std::prev() is analyzed as top-level. Since std::prev() calls std::advance(), which calls an __advance() overload for bidirectional operators which increments or decrements the iterator in a loop. Since the number of iterations is undefined, after 4 iterations on both branch (++ and --) the analysis stops and marks the function as non-inlinable. This, of course does not happen for std::vector because it contains no loop but a simple += operation. If __advance() is marked as non-inlinable, it does not change its argument, so prev(l.end()) is the same as l.end(), so dereferencing the iterator is indeed an error.


https://reviews.llvm.org/D29183