[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

496

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

1179

Typo: "be".

1181–1188

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.

1238

Also typo.

[NoQ]

Looks good otherwise, please commit.

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;
+    if (const auto *SIE = dyn_cast<SymIntExpr>(Pos.getOffset()))
+      SR.markLive(SIE->getLHS());
     SR.markLive(Pos.getOffset());
   }
 
   auto SymbolMap = State->get<IteratorSymbolMap>();
   for (const auto Sym : SymbolMap) {
     const auto Pos = Sym.second;
+    if (const auto *SIE = dyn_cast<SymIntExpr>(Pos.getOffset()))
+      SR.markLive(SIE->getLHS());
     SR.markLive(Pos.getOffset());
   }
 
   auto ContMap = State->get<ContainerMap>();
   for (const auto Cont : ContMap) {
     const auto CData = Cont.second;
     if (CData.getBegin()) {
       SR.markLive(CData.getBegin());
if (comparison) {
     if (const auto CompSym = comparison->getAsSymbol()) {
       return assumeNoOverflow(NewState, cast<SymIntExpr>(CompSym)->getLHS(), 2);
     }
 
     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) {
   const auto *Cont = Pos.getContainer();
   const auto *CData = getContainerData(State, Cont);
   if (!CData)
     return false;
 
   // Out of range means less than the begin symbol or greater or equal to the
   // end symbol.
 
bool compare(ProgramStateRef State, NonLoc NL1, NonLoc NL2,
 
   const auto comparison =
       SVB.evalBinOp(State, Opc, NL1, NL2, SVB.getConditionType())
           .getAs<DefinedSVal>();
 
   if (comparison) {
     return !State->assume(*comparison, false);
   }
 
   return 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)