[analyzer] NFC: MallocChecker: Avoid redundant transitions.

[NoQ]

Summary

In its checkDeadSymbols callback, when no pointers are tracked and the state is not updated, MallocChecker would update the state from "the map of symbols is unset" to "the map of symbols is set to an empty map", which generates a redundant exploded node. Now that zombie symbols are fixed in D18860, we would call checkDeadSymbols() with no dead symbols more often. Avoid the unnecessary transition here.

[Szelethus]

Please reupload with full context.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2369

We are acquiring an object of type ImmutableMap, modify it, and put it back into state? Can't we just modify it through ProgramState's interface directly?

[george.karpenkov]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2369

state is immutable, I don't think we can put anything into it.
We also can't modify ImmutableMap because, well, it's immutable.

[Szelethus]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2369

Poor choice of words, I admit. What I actually meant is that maybe it would be more straighforward if we didnt create a local varable here. But I'm fine with being in the wrong :)

[NoQ]

Re-upload with context. Whoops.

[NoQ]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2369

Manipulating maps directly is slightly cheaper because for every operation you only create a new map but not a new state. I have no indication that this optimization is worth the effort, but i also have no indication that pessimizing it back is worth the effort.

[Szelethus]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2369

Okay, thanks :)

[Szelethus]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2363–2364

Hmmm, I guess we return here because if RegionState is unchanged, so should be ReallocPairs and FreeReturnValue, correct?

[NoQ]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2363–2364

Yeah, kinda. Deserves a comment, i guess.

[NoQ]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2363–2364

Or even an assertion.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

void MallocChecker::reportLeak(SymbolRef Sym, ExplodedNode *N,
   R->addVisitor(llvm::make_unique<MallocBugVisitor>(Sym, true));
   C.emitReport(std::move(R));
 }
 
 void MallocChecker::checkDeadSymbols(SymbolReaper &SymReaper,
                                      CheckerContext &C) const
 {
   ProgramStateRef state = C.getState();
-  RegionStateTy RS = state->get<RegionState>();
+  RegionStateTy OldRS = state->get<RegionState>();
   RegionStateTy::Factory &F = state->get_context<RegionState>();
 
+  RegionStateTy RS = OldRS;
   SmallVector<SymbolRef, 2> Errors;
   for (RegionStateTy::iterator I = RS.begin(), E = RS.end(); I != E; ++I) {
     if (SymReaper.isDead(I->first)) {
       if (I->second.isAllocated() || I->second.isAllocatedOfSizeZero())
         Errors.push_back(I->first);
       // Remove the dead symbol from the map.
       RS = F.remove(RS, I->first);
-
     }
   }
 
+  if (RS == OldRS) {
+    // We shouldn't have touched other maps yet.
+    assert(state->get<ReallocPairs>() ==
+           C.getState()->get<ReallocPairs>());
+    assert(state->get<FreeReturnValue>() ==
+           C.getState()->get<FreeReturnValue>());
+    return;
+  }
+
   // Cleanup the Realloc Pairs Map.
   ReallocPairsTy RP = state->get<ReallocPairs>();
   for (ReallocPairsTy::iterator I = RP.begin(), E = RP.end(); I != E; ++I) {
     if (SymReaper.isDead(I->first) ||
         SymReaper.isDead(I->second.ReallocatedSym)) {
       state = state->remove<ReallocPairs>(I->first);
     }
   }