[PATCH] D53812: [Analyzer] Iterator Checker - Forbid increments past the begin() and decrements past the end() of containers

2018-12-01 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware updated this revision to Diff 176256.
baloghadamsoftware added a comment.

Thank you for the review!

You are absolutely right, these error messages were not accurate and even 
misleading. Now I updated them. To achieve this I also had to separate the 
function `isOutOfRange()` into three different functions.


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

https://reviews.llvm.org/D53812

Files:
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  test/Analysis/iterator-range.cpp

Index: test/Analysis/iterator-range.cpp
===
--- test/Analysis/iterator-range.cpp
+++ test/Analysis/iterator-range.cpp
@@ -23,34 +23,13 @@
 
 void simple_bad_end(const std::vector ) {
   auto i = v.end();
-  *i; // expected-warning{{Iterator accessed outside of its range}}
-}
-
-void simple_good_begin(const std::vector ) {
-  auto i = v.begin();
-  if (i != v.begin()) {
-clang_analyzer_warnIfReached();
-*--i; // no-warning
-  }
-}
-
-void simple_good_begin_negated(const std::vector ) {
-  auto i = v.begin();
-  if (!(i == v.begin())) {
-clang_analyzer_warnIfReached();
-*--i; // no-warning
-  }
-}
-
-void simple_bad_begin(const std::vector ) {
-  auto i = v.begin();
-  *--i; // expected-warning{{Iterator accessed outside of its range}}
+  *i; // expected-warning{{The past-the-end iterator dereferenced}}
 }
 
 void copy(const std::vector ) {
   auto i1 = v.end();
   auto i2 = i1;
-  *i2; // expected-warning{{Iterator accessed outside of its range}}
+  *i2; // expected-warning{{The past-the-end iterator dereferenced}}
 }
 
 void decrease(const std::vector ) {
@@ -70,7 +49,7 @@
   auto i1 = v.end();
   auto i2 = i1;
   --i1;
-  *i2; // expected-warning{{Iterator accessed outside of its range}}
+  *i2; // expected-warning{{The past-the-end iterator dereferenced}}
 }
 
 void copy_and_increase1(const std::vector ) {
@@ -86,7 +65,7 @@
   auto i2 = i1;
   ++i1;
   if (i2 == v.end())
-*i2; // expected-warning{{Iterator accessed outside of its range}}
+*i2; // expected-warning{{The past-the-end iterator dereferenced}}
 }
 
 void copy_and_increase3(const std::vector ) {
@@ -94,7 +73,7 @@
   auto i2 = i1;
   ++i1;
   if (v.end() == i2)
-*i2; // expected-warning{{Iterator accessed outside of its range}}
+*i2; // expected-warning{{The past-the-end iterator dereferenced}}
 }
 
 template 
@@ -116,14 +95,14 @@
 
 void bad_non_std_find(std::vector , int e) {
   auto first = nonStdFind(V.begin(), V.end(), e);
-  *first; // expected-warning{{Iterator accessed outside of its range}}
+  *first; // expected-warning{{The past-the-end iterator dereferenced}}
 }
 
 void tricky(std::vector , int e) {
   const auto first = V.begin();
   const auto comp1 = (first != V.end()), comp2 = (first == V.end());
   if (comp1)
-*first;
+*first; // no-warning
 }
 
 void loop(std::vector , int e) {
@@ -147,7 +126,7 @@
   auto i0 = --L.cend();
   L.push_back(n);
   ++i0;
-  *++i0; // expected-warning{{Iterator accessed outside of its range}}
+  *++i0; // expected-warning{{The past-the-end iterator dereferenced}}
 }
 
 void good_pop_back(std::list , int n) {
@@ -159,7 +138,7 @@
 void bad_pop_back(std::list , int n) {
   auto i0 = --L.cend(); --i0;
   L.pop_back();
-  *++i0; // expected-warning{{Iterator accessed outside of its range}}
+  *++i0; // expected-warning{{The past-the-end iterator dereferenced}}
 }
 
 void good_push_front(std::list , int n) {
@@ -172,7 +151,7 @@
   auto i0 = L.cbegin();
   L.push_front(n);
   --i0;
-  *--i0; // expected-warning{{Iterator accessed outside of its range}}
+  --i0; // expected-warning{{Iterator decremented ahead of its valid range}}
 }
 
 void good_pop_front(std::list , int n) {
@@ -184,13 +163,13 @@
 void bad_pop_front(std::list , int n) {
   auto i0 = ++L.cbegin();
   L.pop_front();
-  *--i0; // expected-warning{{Iterator accessed outside of its range}}
+  --i0; // expected-warning{{Iterator decremented ahead of its valid range}}
 }
 
 void bad_move(std::list , std::list ) {
   auto i0 = --L2.cend();
   L1 = std::move(L2);
-  *++i0; // expected-warning{{Iterator accessed outside of its range}}
+  *++i0; // expected-warning{{The past-the-end iterator dereferenced}}
 }
 
 void bad_move_push_back(std::list , std::list , int n) {
@@ -198,5 +177,25 @@
   L2.push_back(n);
   L1 = std::move(L2);
   ++i0;
-  *++i0; // expected-warning{{Iterator accessed outside of its range}}
+  *++i0; // expected-warning{{The past-the-end iterator dereferenced}}
+}
+
+void good_incr_begin(const std::list ) {
+  auto i0 = L.begin();
+  ++i0; // no-warning
+}
+
+void bad_decr_begin(const std::list ) {
+  auto i0 = L.begin();
+  --i0;  // expected-warning{{Iterator decremented ahead of its valid range}}
+}
+
+void good_decr_end(const std::list ) {
+  auto i0 = L.end();
+  --i0; // no-warning
+}
+
+void bad_incr_end(const std::list ) {
+  auto i0 = L.end();
+  ++i0;  // expected-warning{{Iterator incremented behind the

Re: [PATCH] D53812: [Analyzer] Iterator Checker - Forbid increments past the begin() and decrements past the end() of containers

2018-11-30 Thread Artem Dergachev via cfe-commits 
I suspect that the point is that previously we believed that 
decrementing below begin and than *incrementing* it back to begin() 
would yield begin(), but now we disallow this "incrementing back" 
behavior and warn immediately on decrement because in fact it's already UB.


Though, yeah, the title is wonky.

On 11/30/18 2:19 PM, Roman Lebedev wrote:

""Re: [PATCH] D53812: [Analyzer] Iterator Checker - Forbid increments
past the begin() and decrements past the end() of containers"
Is it be or that reads backwards?
Why can't you increment past the begin()? Can you decrement past the
begin() instead?
And the opposite for the end().

On Sat, Dec 1, 2018 at 1:17 AM Artem Dergachev via Phabricator via
cfe-commits  wrote:

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Makes perfect sense to me!




Comment at: test/Analysis/iterator-range.cpp:190
+  auto i0 = L.begin();
+  --i0;  // expected-warning{{Iterator accessed outside of its range}}
+}

I guess we'll have to come up with a separate warning text for this case, as 
there's no access happening through the iterator.

Regardless of how the final message would look, i suggest adding logic for 
having a different message now, so that we didn't forget about this case later.


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

https://reviews.llvm.org/D53812



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D53812: [Analyzer] Iterator Checker - Forbid increments past the begin() and decrements past the end() of containers

2018-11-30 Thread Roman Lebedev via cfe-commits 
""Re: [PATCH] D53812: [Analyzer] Iterator Checker - Forbid increments
past the begin() and decrements past the end() of containers"
Is it be or that reads backwards?
Why can't you increment past the begin()? Can you decrement past the
begin() instead?
And the opposite for the end().

On Sat, Dec 1, 2018 at 1:17 AM Artem Dergachev via Phabricator via
cfe-commits  wrote:
>
> NoQ accepted this revision.
> NoQ added a comment.
> This revision is now accepted and ready to land.
>
> Makes perfect sense to me!
>
>
>
> 
> Comment at: test/Analysis/iterator-range.cpp:190
> +  auto i0 = L.begin();
> +  --i0;  // expected-warning{{Iterator accessed outside of its range}}
> +}
> 
> I guess we'll have to come up with a separate warning text for this case, as 
> there's no access happening through the iterator.
>
> Regardless of how the final message would look, i suggest adding logic for 
> having a different message now, so that we didn't forget about this case 
> later.
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D53812/new/
>
> https://reviews.llvm.org/D53812
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53812: [Analyzer] Iterator Checker - Forbid increments past the begin() and decrements past the end() of containers

2018-11-30 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Makes perfect sense to me!




Comment at: test/Analysis/iterator-range.cpp:190
+  auto i0 = L.begin();
+  --i0;  // expected-warning{{Iterator accessed outside of its range}}
+}

I guess we'll have to come up with a separate warning text for this case, as 
there's no access happening through the iterator.

Regardless of how the final message would look, i suggest adding logic for 
having a different message now, so that we didn't forget about this case later.


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

https://reviews.llvm.org/D53812



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53812: [Analyzer] Iterator Checker - Forbid increments past the begin() and decrements past the end() of containers

2018-11-23 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware updated this revision to Diff 175134.
baloghadamsoftware added a comment.

Updated according to the comments.


https://reviews.llvm.org/D53812

Files:
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  test/Analysis/iterator-range.cpp

Index: test/Analysis/iterator-range.cpp
===
--- test/Analysis/iterator-range.cpp
+++ test/Analysis/iterator-range.cpp
@@ -26,27 +26,6 @@
   *i; // expected-warning{{Iterator accessed outside of its range}}
 }
 
-void simple_good_begin(const std::vector ) {
-  auto i = v.begin();
-  if (i != v.begin()) {
-clang_analyzer_warnIfReached();
-*--i; // no-warning
-  }
-}
-
-void simple_good_begin_negated(const std::vector ) {
-  auto i = v.begin();
-  if (!(i == v.begin())) {
-clang_analyzer_warnIfReached();
-*--i; // no-warning
-  }
-}
-
-void simple_bad_begin(const std::vector ) {
-  auto i = v.begin();
-  *--i; // expected-warning{{Iterator accessed outside of its range}}
-}
-
 void copy(const std::vector ) {
   auto i1 = v.end();
   auto i2 = i1;
@@ -172,7 +151,7 @@
   auto i0 = L.cbegin();
   L.push_front(n);
   --i0;
-  *--i0; // expected-warning{{Iterator accessed outside of its range}}
+  --i0; // expected-warning{{Iterator accessed outside of its range}}
 }
 
 void good_pop_front(std::list , int n) {
@@ -184,7 +163,7 @@
 void bad_pop_front(std::list , int n) {
   auto i0 = ++L.cbegin();
   L.pop_front();
-  *--i0; // expected-warning{{Iterator accessed outside of its range}}
+  --i0; // expected-warning{{Iterator accessed outside of its range}}
 }
 
 void bad_move(std::list , std::list ) {
@@ -200,3 +179,23 @@
   ++i0;
   *++i0; // expected-warning{{Iterator accessed outside of its range}}
 }
+
+void good_incr_begin(const std::list ) {
+  auto i0 = L.begin();
+  ++i0; // no-warning
+}
+
+void bad_decr_begin(const std::list ) {
+  auto i0 = L.begin();
+  --i0;  // expected-warning{{Iterator accessed outside of its range}}
+}
+
+void good_decr_end(const std::list ) {
+  auto i0 = L.end();
+  --i0; // no-warning
+}
+
+void bad_incr_end(const std::list ) {
+  auto i0 = L.end();
+  ++i0;  // expected-warning{{Iterator accessed outside of its range}}
+}
Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -238,14 +238,17 @@
   void handleEraseAfter(CheckerContext , const SVal ) const;
   void handleEraseAfter(CheckerContext , const SVal ,
 const SVal ) const;
+  void verifyIncrement(CheckerContext , const SVal ) const;
+  void verifyDecrement(CheckerContext , const SVal ) const;
   void verifyRandomIncrOrDecr(CheckerContext , OverloadedOperatorKind Op,
-  const SVal , const SVal ,
-  const SVal ) const;
+  const SVal , const SVal ) const;
   void verifyMatch(CheckerContext , const SVal ,
const MemRegion *Cont) const;
   void verifyMatch(CheckerContext , const SVal ,
const SVal ) const;
-
+  IteratorPosition advancePosition(CheckerContext , OverloadedOperatorKind Op,
+   const IteratorPosition ,
+   const SVal ) const;
   void reportOutOfRangeBug(const StringRef , const SVal ,
CheckerContext , ExplodedNode *ErrNode) const;
   void reportMismatchedBug(const StringRef , const SVal ,
@@ -388,7 +391,8 @@
 bool hasLiveIterators(ProgramStateRef State, const MemRegion *Cont);
 bool isBoundThroughLazyCompoundVal(const Environment ,
const MemRegion *Reg);
-bool isOutOfRange(ProgramStateRef State, const IteratorPosition );
+bool isOutOfRange(ProgramStateRef State, const IteratorPosition ,
+  bool IncludeEnd = false);
 bool isZero(ProgramStateRef State, const NonLoc );
 } // namespace
 
@@ -422,29 +426,46 @@
 verifyAccess(C, Call.getArgSVal(0));
   }
 }
-if (ChecksEnabled[CK_IteratorRangeChecker] &&
-isRandomIncrOrDecrOperator(Func->getOverloadedOperator())) {
-  if (const auto *InstCall = dyn_cast()) {
-// Check for out-of-range incrementions and decrementions
-if (Call.getNumArgs() >= 1) {
-  verifyRandomIncrOrDecr(C, Func->getOverloadedOperator(),
- Call.getReturnValue(),
- InstCall->getCXXThisVal(), Call.getArgSVal(0));
+if (ChecksEnabled[CK_IteratorRangeChecker]) {
+  if (isIncrementOperator(Func->getOverloadedOperator())) {
+// Check for out-of-range incrementions
+if (const auto *InstCall = dyn_cast()) {
+  verifyIncrement(C, InstCall->getCXXThisVal());
+} else {
+  if (Call.getNumArgs() >= 1) {
+verifyIncrement(C, Call.getArgSVal(0));
+

[PATCH] D53812: [Analyzer] Iterator Checker - Forbid increments past the begin() and decrements past the end() of containers

2018-11-23 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware updated this revision to Diff 175124.
baloghadamsoftware added a comment.

Restored corrected patch after uploading an incorrect one.


https://reviews.llvm.org/D53812

Files:
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  test/Analysis/iterator-range.cpp

Index: test/Analysis/iterator-range.cpp
===
--- test/Analysis/iterator-range.cpp
+++ test/Analysis/iterator-range.cpp
@@ -26,27 +26,6 @@
   *i; // expected-warning{{Iterator accessed outside of its range}}
 }
 
-void simple_good_begin(const std::vector ) {
-  auto i = v.begin();
-  if (i != v.begin()) {
-clang_analyzer_warnIfReached();
-*--i; // no-warning
-  }
-}
-
-void simple_good_begin_negated(const std::vector ) {
-  auto i = v.begin();
-  if (!(i == v.begin())) {
-clang_analyzer_warnIfReached();
-*--i; // no-warning
-  }
-}
-
-void simple_bad_begin(const std::vector ) {
-  auto i = v.begin();
-  *--i; // expected-warning{{Iterator accessed outside of its range}}
-}
-
 void copy(const std::vector ) {
   auto i1 = v.end();
   auto i2 = i1;
@@ -172,7 +151,7 @@
   auto i0 = L.cbegin();
   L.push_front(n);
   --i0;
-  *--i0; // expected-warning{{Iterator accessed outside of its range}}
+  --i0; // expected-warning{{Iterator accessed outside of its range}}
 }
 
 void good_pop_front(std::list , int n) {
@@ -184,7 +163,7 @@
 void bad_pop_front(std::list , int n) {
   auto i0 = ++L.cbegin();
   L.pop_front();
-  *--i0; // expected-warning{{Iterator accessed outside of its range}}
+  --i0; // expected-warning{{Iterator accessed outside of its range}}
 }
 
 void bad_move(std::list , std::list ) {
@@ -200,3 +179,23 @@
   ++i0;
   *++i0; // expected-warning{{Iterator accessed outside of its range}}
 }
+
+void good_incr_begin(const std::list ) {
+  auto i0 = L.begin();
+  ++i0; // no-warning
+}
+
+void bad_decr_begin(const std::list ) {
+  auto i0 = L.begin();
+  --i0;  // expected-warning{{Iterator accessed outside of its range}}
+}
+
+void good_decr_end(const std::list ) {
+  auto i0 = L.end();
+  --i0; // no-warning
+}
+
+void bad_incr_end(const std::list ) {
+  auto i0 = L.end();
+  ++i0;  // expected-warning{{Iterator accessed outside of its range}}
+}
Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -238,14 +238,17 @@
   void handleEraseAfter(CheckerContext , const SVal ) const;
   void handleEraseAfter(CheckerContext , const SVal ,
 const SVal ) const;
+  void verifyIncrement(CheckerContext , const SVal ) const;
+  void verifyDecrement(CheckerContext , const SVal ) const;
   void verifyRandomIncrOrDecr(CheckerContext , OverloadedOperatorKind Op,
-  const SVal , const SVal ,
-  const SVal ) const;
+  const SVal , const SVal ) const;
   void verifyMatch(CheckerContext , const SVal ,
const MemRegion *Cont) const;
   void verifyMatch(CheckerContext , const SVal ,
const SVal ) const;
-
+  IteratorPosition shiftPosition(CheckerContext , OverloadedOperatorKind Op,
+ const IteratorPosition ,
+ const SVal ) const;
   void reportOutOfRangeBug(const StringRef , const SVal ,
CheckerContext , ExplodedNode *ErrNode) const;
   void reportMismatchedBug(const StringRef , const SVal ,
@@ -388,7 +391,8 @@
 bool hasLiveIterators(ProgramStateRef State, const MemRegion *Cont);
 bool isBoundThroughLazyCompoundVal(const Environment ,
const MemRegion *Reg);
-bool isOutOfRange(ProgramStateRef State, const IteratorPosition );
+bool isOutOfRange(ProgramStateRef State, const IteratorPosition ,
+  bool IncludeEnd = false);
 bool isZero(ProgramStateRef State, const NonLoc );
 } // namespace
 
@@ -422,29 +426,46 @@
 verifyAccess(C, Call.getArgSVal(0));
   }
 }
-if (ChecksEnabled[CK_IteratorRangeChecker] &&
-isRandomIncrOrDecrOperator(Func->getOverloadedOperator())) {
-  if (const auto *InstCall = dyn_cast()) {
-// Check for out-of-range incrementions and decrementions
-if (Call.getNumArgs() >= 1) {
-  verifyRandomIncrOrDecr(C, Func->getOverloadedOperator(),
- Call.getReturnValue(),
- InstCall->getCXXThisVal(), Call.getArgSVal(0));
+if (ChecksEnabled[CK_IteratorRangeChecker]) {
+  if (isIncrementOperator(Func->getOverloadedOperator())) {
+// Check for out-of-range incrementions
+if (const auto *InstCall = dyn_cast()) {
+  verifyIncrement(C, InstCall->getCXXThisVal());
+} else {
+  if (Call.getNumArgs() >= 1) {
+verifyIncrement(C,

[PATCH] D53812: [Analyzer] Iterator Checker - Forbid increments past the begin() and decrements past the end() of containers

2018-11-23 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware updated this revision to Diff 175113.
baloghadamsoftware added a comment.
Herald added a subscriber: gamesh411.

More standard-like tests.


https://reviews.llvm.org/D53812

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  lib/StaticAnalyzer/Core/MemRegion.cpp
  test/Analysis/iterator-range.cpp

Index: test/Analysis/iterator-range.cpp
===
--- test/Analysis/iterator-range.cpp
+++ test/Analysis/iterator-range.cpp
@@ -200,3 +200,40 @@
   ++i0;
   *++i0; // expected-warning{{Iterator accessed outside of its range}}
 }
+
+struct simple_iterator_base {
+  simple_iterator_base();
+  simple_iterator_base(const simple_iterator_base& rhs);
+  simple_iterator_base =(const simple_iterator_base& rhs);
+  virtual ~simple_iterator_base();
+  bool friend operator==(const simple_iterator_base ,
+ const simple_iterator_base );
+  bool friend operator!=(const simple_iterator_base ,
+ const simple_iterator_base );
+private:
+  int *ptr;
+};
+
+struct simple_derived_iterator: public simple_iterator_base {
+  int& operator*();
+  int* operator->();
+  simple_iterator_base ++();
+  simple_iterator_base operator++(int);
+  simple_iterator_base ();
+  simple_iterator_base operator--(int);
+};
+
+struct simple_container {
+  typedef simple_derived_iterator iterator;
+
+  iterator begin();
+  iterator end();
+};
+
+void good_derived(simple_container c) {
+  auto i0 = c.end();
+  if (i0 != c.end()) {
+clang_analyzer_warnIfReached();
+*i0; // no-warning
+  }
+}
Index: lib/StaticAnalyzer/Core/MemRegion.cpp
===
--- lib/StaticAnalyzer/Core/MemRegion.cpp
+++ lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -1175,6 +1175,20 @@
   return R;
 }
 
+// getgetMostDerivedObjectRegion gets the region of the root class of a C++
+// class hierarchy.
+const MemRegion *MemRegion::getMostDerivedObjectRegion() const {
+  const MemRegion *R = this;
+  while (true) {
+if (R->getKind() == MemRegion::CXXBaseObjectRegionKind) {
+R = cast(R)->getSuperRegion();
+continue;
+}
+break;
+  }
+  return R;
+}
+
 bool MemRegion::isSubRegionOf(const MemRegion *) const {
   return false;
 }
Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -1089,9 +1089,7 @@
 void IteratorChecker::verifyMatch(CheckerContext , const SVal ,
   const MemRegion *Cont) const {
   // Verify match between a container and the container of an iterator
-  while (const auto *CBOR = Cont->getAs()) {
-Cont = CBOR->getSuperRegion();
-  }
+  Cont = Cont->getMostDerivedObjectRegion();
 
   auto State = C.getState();
   const auto *Pos = getIteratorPosition(State, Iter);
@@ -1125,9 +1123,7 @@
   if (!ContReg)
 return;
 
-  while (const auto *CBOR = ContReg->getAs()) {
-ContReg = CBOR->getSuperRegion();
-  }
+  ContReg = ContReg->getMostDerivedObjectRegion();
 
   // If the container already has a begin symbol then use it. Otherwise first
   // create a new one.
@@ -1151,9 +1147,7 @@
   if (!ContReg)
 return;
 
-  while (const auto *CBOR = ContReg->getAs()) {
-ContReg = CBOR->getSuperRegion();
-  }
+  ContReg = ContReg->getMostDerivedObjectRegion();
 
   // If the container already has an end symbol then use it. Otherwise first
   // create a new one.
@@ -1174,9 +1168,7 @@
 void IteratorChecker::assignToContainer(CheckerContext , const Expr *CE,
 const SVal ,
 const MemRegion *Cont) const {
-  while (const auto *CBOR = Cont->getAs()) {
-Cont = CBOR->getSuperRegion();
-  }
+  Cont = Cont->getMostDerivedObjectRegion();
 
   auto State = C.getState();
   auto  = C.getSymbolManager();
@@ -1194,9 +1186,7 @@
   if (!ContReg)
 return;
 
-  while (const auto *CBOR = ContReg->getAs()) {
-ContReg = CBOR->getSuperRegion();
-  }
+  ContReg = ContReg->getMostDerivedObjectRegion();
 
   // Assignment of a new value to a container always invalidates all its
   // iterators
@@ -1211,9 +1201,7 @@
   if (!OldCont.isUndef()) {
 const auto *OldContReg = OldCont.getAsRegion();
 if (OldContReg) {
-  while (const auto *CBOR = OldContReg->getAs()) {
-OldContReg = CBOR->getSuperRegion();
-  }
+  OldContReg = OldContReg->getMostDerivedObjectRegion();
   const auto OldCData = getContainerData(State, OldContReg);
   if (OldCData) {
 if (const auto OldEndSym = OldCData->getEnd()) {
@@ -1273,9 +1261,7 @@
   if (!ContReg)
 return;
 
-  while (const auto *CBOR = ContReg->getAs()) {
-ContReg = CBOR->getSuperRegion();
-  }
+  ContReg = ContReg->getMostDerivedObjectRegion();

[PATCH] D53812: [Analyzer] Iterator Checker - Forbid increments past the begin() and decrements past the end() of containers

2018-10-30 Thread Whisperity via Phabricator via cfe-commits 
whisperity added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:420-465
 if (ChecksEnabled[CK_InvalidatedIteratorChecker] &&
 isAccessOperator(Func->getOverloadedOperator())) {
   // Check for any kind of access of invalidated iterator positions
   if (const auto *InstCall = dyn_cast()) {
 verifyAccess(C, InstCall->getCXXThisVal());
   } else {
 verifyAccess(C, Call.getArgSVal(0));

This is a bit of an organisational comment (and the compiler of course 
hopefully optimises this out), but could you, for the sake of code readability, 
break the if-elseif-elseif-elseif chain's common check out into an outer if, 
and only check for the inner parts? Thinking of something like this:

```
if (ChecksEnabled[CK_InvalidatedIteratorChecker])
{
   /* yadda */
}

if (ChecksEnabled[CK_IteratorRangeChecker])
{
  X* OOperator = Func->getOverloadedOperator();
  if (isIncrementOperator(OOperator))
  {
/* yadda */
  } else if (isDecrementOperator(OOperator)) {
/* yadda */
  }
}

/* etc. */ 
```



Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1074
 
-  auto  = C.getSymbolManager();
-  auto  = C.getSValBuilder();
-  auto BinOp = (Op == OO_Plus || Op == OO_PlusEqual) ? BO_Add : BO_Sub;
-  const auto OldOffset = Pos->getOffset();
-  const auto intValue = value.getAs();
-  if (!intValue)
+  // Incremention or decremention by 0 is never bug
+  if (isZero(State, Value.castAs()))

is never a, also `.` at the end



Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1568
 
+IteratorPosition IteratorChecker::shiftPosition(CheckerContext ,
+OverloadedOperatorKind Op,

(Minor: I don't like the name of this function, `advancePosition` (akin to 
`std::advance`) sounds cleaner to my ears.)



Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1575
+  auto  = C.getSValBuilder();
+  auto BinOp = (Op == OO_Plus || Op == OO_PlusEqual) ? BO_Add : BO_Sub;
+  if (const auto IntDist = Distance.getAs()) {

For the sake of clarity, I think an assert should be introduced her, lest 
someone ends up shifting the position with `<=`.



Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:2329-2330
 
   // Out of range means less than the begin symbol or greater or equal to the
   // end symbol.
 

How does the introduction of `IncludeEnd` change this behaviour? What does the 
parameter refer to?


Repository:
  rC Clang

https://reviews.llvm.org/D53812



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53812: [Analyzer] Iterator Checker - Forbid increments past the begin() and decrements past the end() of containers

2018-10-29 Thread Balogh , Ádám via Phabricator via cfe-commits 
baloghadamsoftware created this revision.
baloghadamsoftware added a reviewer: NoQ.
baloghadamsoftware added a project: clang.
Herald added subscribers: donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, 
rnkovacs, szepet, whisperity.
Herald added a reviewer: george.karpenkov.

Previously, the iterator range checker only warned upon dereference of 
iterators outside their valid range as well as increments and decrements of 
out-of range iterators where the result remains out-of range. However, the 
`C++` standard is more strict than this: decrementing `begin()` or inrementing 
`end()` results in undefined behavior even if the iterator is not dereferenced 
afterward. Coming back to the range once out-of-range is also undefined.

This patch corrects the behavior of the iterator range checker: warnings are 
given for any operation whose result is ahead of `begin()` or past the `end()` 
(which is the past-end iterator itself, thus now we are speaking of past 
past-end).


Repository:
  rC Clang

https://reviews.llvm.org/D53812

Files:
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  test/Analysis/iterator-range.cpp

Index: test/Analysis/iterator-range.cpp
===
--- test/Analysis/iterator-range.cpp
+++ test/Analysis/iterator-range.cpp
@@ -26,27 +26,6 @@
   *i; // expected-warning{{Iterator accessed outside of its range}}
 }
 
-void simple_good_begin(const std::vector ) {
-  auto i = v.begin();
-  if (i != v.begin()) {
-clang_analyzer_warnIfReached();
-*--i; // no-warning
-  }
-}
-
-void simple_good_begin_negated(const std::vector ) {
-  auto i = v.begin();
-  if (!(i == v.begin())) {
-clang_analyzer_warnIfReached();
-*--i; // no-warning
-  }
-}
-
-void simple_bad_begin(const std::vector ) {
-  auto i = v.begin();
-  *--i; // expected-warning{{Iterator accessed outside of its range}}
-}
-
 void copy(const std::vector ) {
   auto i1 = v.end();
   auto i2 = i1;
@@ -172,7 +151,7 @@
   auto i0 = L.cbegin();
   L.push_front(n);
   --i0;
-  *--i0; // expected-warning{{Iterator accessed outside of its range}}
+  --i0; // expected-warning{{Iterator accessed outside of its range}}
 }
 
 void good_pop_front(std::list , int n) {
@@ -184,7 +163,7 @@
 void bad_pop_front(std::list , int n) {
   auto i0 = ++L.cbegin();
   L.pop_front();
-  *--i0; // expected-warning{{Iterator accessed outside of its range}}
+  --i0; // expected-warning{{Iterator accessed outside of its range}}
 }
 
 void bad_move(std::list , std::list ) {
@@ -200,3 +179,23 @@
   ++i0;
   *++i0; // expected-warning{{Iterator accessed outside of its range}}
 }
+
+void good_incr_begin(const std::list ) {
+  auto i0 = L.begin();
+  ++i0; // no-warning
+}
+
+void bad_decr_begin(const std::list ) {
+  auto i0 = L.begin();
+  --i0;  // expected-warning{{Iterator accessed outside of its range}}
+}
+
+void good_decr_end(const std::list ) {
+  auto i0 = L.end();
+  --i0; // no-warning
+}
+
+void bad_incr_end(const std::list ) {
+  auto i0 = L.end();
+  ++i0;  // expected-warning{{Iterator accessed outside of its range}}
+}
Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -238,14 +238,17 @@
   void handleEraseAfter(CheckerContext , const SVal ) const;
   void handleEraseAfter(CheckerContext , const SVal ,
 const SVal ) const;
+  void verifyIncrement(CheckerContext , const SVal ) const;
+  void verifyDecrement(CheckerContext , const SVal ) const;
   void verifyRandomIncrOrDecr(CheckerContext , OverloadedOperatorKind Op,
-  const SVal , const SVal ,
-  const SVal ) const;
+  const SVal , const SVal ) const;
   void verifyMatch(CheckerContext , const SVal ,
const MemRegion *Cont) const;
   void verifyMatch(CheckerContext , const SVal ,
const SVal ) const;
-
+  IteratorPosition shiftPosition(CheckerContext , OverloadedOperatorKind Op,
+ const IteratorPosition ,
+ const SVal ) const;
   void reportOutOfRangeBug(const StringRef , const SVal ,
CheckerContext , ExplodedNode *ErrNode) const;
   void reportMismatchedBug(const StringRef , const SVal ,
@@ -388,7 +391,8 @@
 bool hasLiveIterators(ProgramStateRef State, const MemRegion *Cont);
 bool isBoundThroughLazyCompoundVal(const Environment ,
const MemRegion *Reg);
-bool isOutOfRange(ProgramStateRef State, const IteratorPosition );
+bool isOutOfRange(ProgramStateRef State, const IteratorPosition ,
+  bool IncludeEnd = false);
 bool isZero(ProgramStateRef State, const NonLoc );
 } // namespace
 
@@ -423,19 +427,37 @@
   }
 }
 if (ChecksEnabled[CK_IteratorRangeChecker] &&
+