[Analyzer] Hotfix for iterator checkers: Mark left-hand side of `SymIntExpr` objects as live in the program state maps.

[baloghadamsoftware]

Summary

Marking a symbolic expression as live is not recursive. In our checkers we either use conjured symbols or conjured symbols plus/minus concrete integers to represent abstract positions of iterators, so we must mark the left-hand side of these expressions as live to prevent them from getting reaped.

[NoQ]

That's right. You only need to mark "atomic" symbols (SymbolData) as live, and expressions that contain them would automatically become live. So i think you should just iterate through a symbol_iterator and mark all SymbolData symbols you encounter as live.

Is this a hotfix for a test failure? Otherwise it'd be great to have tests.

[baloghadamsoftware]

Updated according to the comments and assertions added to fail the tests without the fix.

[NoQ]

Looks good with minor comments.

lib/StaticAnalyzer/Checkers/IteratorChecker.cpp

493 ↗

(On Diff #153698)

Let's only mark SymbolData-type symbols as live; that should be enough.

1168 ↗

(On Diff #153698)

Typo: "be".

1170–1177 ↗

(On Diff #153698)

I believe this code should be reworked as follows:

1. Subtract the operands using evalBinOp().
2. Assume that the result doesn't overflow.
3. Compare the result to 0.
4. Assume the result of the comparison.

This should hopefully yield the same result without ever needing to introspect the SymExpr.

I suggest a FIXME.

1237 ↗

(On Diff #153698)

Also typo.

[NoQ]

Looks good otherwise, please commit.

cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp

 }
 
 void IteratorChecker::checkLiveSymbols(ProgramStateRef State,
                                        SymbolReaper &SR) const {
   // Keep symbolic expressions of iterator positions, container begins and ends
   // alive
   auto RegionMap = State->get<IteratorRegionMap>();
   for (const auto Reg : RegionMap) {
-    const auto Pos = Reg.second;
-    SR.markLive(Pos.getOffset());
+    const auto Offset = Reg.second.getOffset();
+    for (auto i = Offset->symbol_begin(); i != Offset->symbol_end(); ++i)
+      if (isa<SymbolData>(*i))
+        SR.markLive(*i);
   }
 
   auto SymbolMap = State->get<IteratorSymbolMap>();
   for (const auto Sym : SymbolMap) {
-    const auto Pos = Sym.second;
-    SR.markLive(Pos.getOffset());
+    const auto Offset = Sym.second.getOffset();
+    for (auto i = Offset->symbol_begin(); i != Offset->symbol_end(); ++i)
+      if (isa<SymbolData>(*i))
+        SR.markLive(*i);
   }
 
   auto ContMap = State->get<ContainerMap>();
   for (const auto Cont : ContMap) {
     const auto CData = Cont.second;
     if (CData.getBegin()) {
       SR.markLive(CData.getBegin());
     }
ProgramStateRef adjustIteratorPosition(ProgramStateRef State,
   }
 }
 
 ProgramStateRef relateIteratorPositions(ProgramStateRef State,
                                         const IteratorPosition &Pos1,
                                         const IteratorPosition &Pos2,
                                         bool Equal) {
   auto &SVB = State->getStateManager().getSValBuilder();
+
+  // FIXME: This code should be reworked as follows:
+  // 1. Subtract the operands using evalBinOp().
+  // 2. Assume that the result doesn't overflow.
+  // 3. Compare the result to 0.
+  // 4. Assume the result of the comparison.
   const auto comparison =
       SVB.evalBinOp(State, BO_EQ, nonloc::SymbolVal(Pos1.getOffset()),
-                    nonloc::SymbolVal(Pos2.getOffset()), SVB.getConditionType())
-          .getAs<DefinedSVal>();
+                    nonloc::SymbolVal(Pos2.getOffset()),
+                    SVB.getConditionType());
+
+  assert(comparison.getAs<DefinedSVal>() &&
+    "Symbol comparison must be a `DefinedSVal`");
 
-  if (comparison) {
-    auto NewState = State->assume(*comparison, Equal);
-    if (const auto CompSym = comparison->getAsSymbol()) {
-      return assumeNoOverflow(NewState, cast<SymIntExpr>(CompSym)->getLHS(), 2);
-    }
+  auto NewState = State->assume(comparison.castAs<DefinedSVal>(), Equal);
+  if (const auto CompSym = comparison.getAsSymbol()) {
+    assert(isa<SymIntExpr>(CompSym) &&
+           "Symbol comparison must be a `SymIntExpr`");
+    assert(BinaryOperator::isComparisonOp(
+               cast<SymIntExpr>(CompSym)->getOpcode()) &&
+           "Symbol comparison must be a comparison");
 
-    return NewState;
-  }
 
-  return State;
-}
-
 bool isZero(ProgramStateRef State, const NonLoc &Val) {
   auto &BVF = State->getBasicVals();
   return compare(State, Val,
                  nonloc::ConcreteInt(BVF.getValue(llvm::APSInt::get(0))),
                  BO_EQ);
 }
 
 bool isOutOfRange(ProgramStateRef State, const IteratorPosition &Pos) {
bool compare(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2,
   return compare(State, nonloc::SymbolVal(Sym1), nonloc::SymbolVal(Sym2), Opc);
 }
 
 bool compare(ProgramStateRef State, NonLoc NL1, NonLoc NL2,
              BinaryOperator::Opcode Opc) {
   auto &SVB = State->getStateManager().getSValBuilder();
 
   const auto comparison =
-      SVB.evalBinOp(State, Opc, NL1, NL2, SVB.getConditionType())
-          .getAs<DefinedSVal>();
+    SVB.evalBinOp(State, Opc, NL1, NL2, SVB.getConditionType());
 
-  if (comparison) {
-    return !State->assume(*comparison, false);
-  }
+  assert(comparison.getAs<DefinedSVal>() &&
+    "Symbol comparison must be a `DefinedSVal`");
 
-  return false;
+  return !State->assume(comparison.castAs<DefinedSVal>(), false);
 }
 
 } // namespace
 
 #define REGISTER_CHECKER(name)                                                 \
   void ento::register##name(CheckerManager &Mgr) {                             \
     auto *checker = Mgr.registerChecker<IteratorChecker>();                    \
     checker->ChecksEnabled[IteratorChecker::CK_##name] = true;                 \
     checker->CheckNames[IteratorChecker::CK_##name] =                          \
         Mgr.getCurrentCheckName();                                             \
   }
 
 REGISTER_CHECKER(IteratorRangeChecker)