[PATCH] D85424: [Analyzer] Crash fix for alpha.cplusplus.IteratorRange

Mon Sep 21 23:16:29 PDT 2020

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

Uh-oh, so you're saying that you're actively modeling every single operator `+` and `-` on every single integer in the program even when they don't represent any iterators at all? How can a raw integer be an iterator to begin with, given that you can't dereference an integer? Is this caused by D82185 <https://reviews.llvm.org/D82185>? Can you defend against such situations by checking that the operand is a pointer before doing any operations?

As of now it sounds like the function quits pretty quickly because it doesn't find an old iterator position for that integer but i'm pretty worried about us doing weird things because we're not checking our types regularly enough.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp:227
   auto Value = RHS;
   if (auto ValAsLoc = RHS.getAs<Loc>()) {
     Value = State->getRawSVal(*ValAsLoc);
----------------
This code, as per my usual concern, is pretty questionable. A `Loc` may be both the iterator lvalue or the iterator rvalue and you can't figure this out by looking at the value. The information on which the decision to perform this dereference is based should be communicated through a different channel.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp:232
+  if (Value.isUnknownOrUndef())
+    //  if (Value.isUnknownOrUndef())
     return;
----------------
This comment is probably unnecessary.

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

https://reviews.llvm.org/D85424