[scan-build] fix warnings emiited on LLVM Analysis code base

[apelete]

Summary

Fix "Logic error" warnings of the type "Called C++ object pointer is
null" reported by Clang Static Analyzer on the following files:

• lib/Analysis/ScalarEvolution.cpp,
• lib/Analysis/LoopInfo.cpp.

Signed-off-by: Apelete Seketeli <[e-mail]>

[sbaranga]

lib/Analysis/LoopInfo.cpp

502

Do we really need three of these?

lib/Analysis/ScalarEvolution.cpp

5570

Maybe a more meaningful string here would be "ExitNotTakenExtras is NULL while having more than one exit".

[apelete]

[scan-build] fix warnings emiited on LLVM Analysis code base

Changes since last revision:

• lib/Analysis/LoopInfo.cpp: remove redundant asserts,

• lib/Analysis/ScalarEvolution.cpp: reword assert message.

[sbaranga]

lib/Analysis/LoopInfo.cpp

535

Would there be any reason to have this assert here and not at the top?

[apelete]

lib/Analysis/LoopInfo.cpp

535

Code path that leads to "Called C++ object pointer is null" is the one inside the for() loop on line 537.
Not sure asserting outside that path would help, I'm going to verify that.

[apelete]

lib/Analysis/LoopInfo.cpp

535

Tested moving the assert at the top of function, and I can confirm that it has no effect on the codepath inside the for() loop at line 537.

Would you prefer to have it at the beginning of the for() loop ?

[sbaranga]

lib/Analysis/LoopInfo.cpp

535

Sorry for taking so long to reply. I don't fully understand how clang gets this warning, but having it at the top of the function would surely guarantee that Unloop is not null. So maybe the static analyzer is too eager?

I think it would be overall a lot better to change Unloop to be a reference (which removes this issue entirely).

[apelete]

[scan-build] fix warnings emiited on LLVM Analysis code base

Changes since last revision:

• lib/Analysis/LoopInfo.cpp: change 'Unloop' member of UnloopUpdater class to be a reference to avoid null pointer warning from Clang Static Analyzer.

[sbaranga]

Thanks, LGTM!

-Silviu

[apelete]

In D19964#428285, @sbaranga wrote:

Thanks, LGTM!

-Silviu

I don't have commit access so please go ahead and commit this for me when you're ready.

Thanks for your review.

[sbaranga]

Committed in r269424.

lib/Analysis/LoopInfo.cpp

 //===----------------------------------------------------------------------===//
 // UnloopUpdater implementation
 //
 
 namespace {
 /// Find the new parent loop for all blocks within the "unloop" whose last
 /// backedges has just been removed.
 class UnloopUpdater {
-  Loop *Unloop;
+  Loop &Unloop;
   LoopInfo *LI;
 
   LoopBlocksDFS DFS;
 
   // Map unloop's immediate subloops to their nearest reachable parents. Nested
   // loops within these subloops will not change parents. However, an immediate
   // subloop's new parent will be the nearest loop reachable from either its own
   // exits *or* any of its nested loop's exits.
   DenseMap<Loop*, Loop*> SubloopParents;
 
   // Flag the presence of an irreducible backedge whose destination is a block
   // directly contained by the original unloop.
   bool FoundIB;
 
 public:
   UnloopUpdater(Loop *UL, LoopInfo *LInfo) :
-    Unloop(UL), LI(LInfo), DFS(UL), FoundIB(false) {}
+    Unloop(*UL), LI(LInfo), DFS(UL), FoundIB(false) {}
 
   void updateBlockParents();
 
   void removeBlocksFromAncestors();
 
   void updateSubloopParents();
 
 protected:
   Loop *getNearestLoop(BasicBlock *BB, Loop *BBLoop);
 };
 } // end anonymous namespace
 
 /// Update the parent loop for all blocks that are directly contained within the
 /// original "unloop".
 void UnloopUpdater::updateBlockParents() {
-  if (Unloop->getNumBlocks()) {
+  if (Unloop.getNumBlocks()) {
     // Perform a post order CFG traversal of all blocks within this loop,
     // propagating the nearest loop from sucessors to predecessors.
     LoopBlocksTraversal Traversal(DFS, LI);
     for (LoopBlocksTraversal::POTIterator POI = Traversal.begin(),
            POE = Traversal.end(); POI != POE; ++POI) {
 
       Loop *L = LI->getLoopFor(*POI);
       Loop *NL = getNearestLoop(*POI, L);
 
       if (NL != L) {
         // For reducible loops, NL is now an ancestor of Unloop.
-        assert((NL != Unloop && (!NL || NL->contains(Unloop))) &&
+        assert((NL != &Unloop && (!NL || NL->contains(&Unloop))) &&
                "uninitialized successor");
         LI->changeLoopFor(*POI, NL);
       }
       else {
         // Or the current block is part of a subloop, in which case its parent
         // is unchanged.
-        assert((FoundIB || Unloop->contains(L)) && "uninitialized successor");
+        assert((FoundIB || Unloop.contains(L)) && "uninitialized successor");
       }
     }
   }
   // Each irreducible loop within the unloop induces a round of iteration using
   // the DFS result cached by Traversal.
   bool Changed = FoundIB;
   for (unsigned NIters = 0; Changed; ++NIters) {
-    assert(NIters < Unloop->getNumBlocks() && "runaway iterative algorithm");
+    assert(NIters < Unloop.getNumBlocks() && "runaway iterative algorithm");
 
     // Iterate over the postorder list of blocks, propagating the nearest loop
     // from successors to predecessors as before.
     Changed = false;
     for (LoopBlocksDFS::POIterator POI = DFS.beginPostorder(),
            POE = DFS.endPostorder(); POI != POE; ++POI) {
 
       Loop *L = LI->getLoopFor(*POI);
       Loop *NL = getNearestLoop(*POI, L);
       if (NL != L) {
-        assert(NL != Unloop && (!NL || NL->contains(Unloop)) &&
+        assert(NL != &Unloop && (!NL || NL->contains(&Unloop)) &&
                "uninitialized successor");
         LI->changeLoopFor(*POI, NL);
         Changed = true;
       }
     }
   }
 }
 
 /// Remove unloop's blocks from all ancestors below their new parents.
 void UnloopUpdater::removeBlocksFromAncestors() {
   // Remove all unloop's blocks (including those in nested subloops) from
   // ancestors below the new parent loop.
-  for (Loop::block_iterator BI = Unloop->block_begin(),
-         BE = Unloop->block_end(); BI != BE; ++BI) {
+  for (Loop::block_iterator BI = Unloop.block_begin(),
+         BE = Unloop.block_end(); BI != BE; ++BI) {
     Loop *OuterParent = LI->getLoopFor(*BI);
-    if (Unloop->contains(OuterParent)) {
-      while (OuterParent->getParentLoop() != Unloop)
+    if (Unloop.contains(OuterParent)) {
+      while (OuterParent->getParentLoop() != &Unloop)
         OuterParent = OuterParent->getParentLoop();
       OuterParent = SubloopParents[OuterParent];
     }
     // Remove blocks from former Ancestors except Unloop itself which will be
     // deleted.
-    for (Loop *OldParent = Unloop->getParentLoop(); OldParent != OuterParent;
+    for (Loop *OldParent = Unloop.getParentLoop(); OldParent != OuterParent;
          OldParent = OldParent->getParentLoop()) {
       assert(OldParent && "new loop is not an ancestor of the original");
       OldParent->removeBlockFromLoop(*BI);
     }
   }
 }
 
 /// Update the parent loop for all subloops directly nested within unloop.
 void UnloopUpdater::updateSubloopParents() {
-  while (!Unloop->empty()) {
-    Loop *Subloop = *std::prev(Unloop->end());
-    Unloop->removeChildLoop(std::prev(Unloop->end()));
+  while (!Unloop.empty()) {
+    Loop *Subloop = *std::prev(Unloop.end());
+    Unloop.removeChildLoop(std::prev(Unloop.end()));
 
     assert(SubloopParents.count(Subloop) && "DFS failed to visit subloop");
     if (Loop *Parent = SubloopParents[Subloop])
       Parent->addChildLoop(Subloop);
     else
       LI->addTopLevelLoop(Subloop);
   }
 }
 
 /// Return the nearest parent loop among this block's successors. If a successor
 /// is a subloop header, consider its parent to be the nearest parent of the
 /// subloop's exits.
 ///
 /// For subloop blocks, simply update SubloopParents and return NULL.
 Loop *UnloopUpdater::getNearestLoop(BasicBlock *BB, Loop *BBLoop) {
 
   // Initially for blocks directly contained by Unloop, NearLoop == Unloop and
   // is considered uninitialized.
   Loop *NearLoop = BBLoop;
 
   Loop *Subloop = nullptr;
-  if (NearLoop != Unloop && Unloop->contains(NearLoop)) {
+  if (NearLoop != &Unloop && Unloop.contains(NearLoop)) {
     Subloop = NearLoop;
     // Find the subloop ancestor that is directly contained within Unloop.
-    while (Subloop->getParentLoop() != Unloop) {
+    while (Subloop->getParentLoop() != &Unloop) {
       Subloop = Subloop->getParentLoop();
       assert(Subloop && "subloop is not an ancestor of the original loop");
     }
     // Get the current nearest parent of the Subloop exits, initially Unloop.
     NearLoop =
-      SubloopParents.insert(std::make_pair(Subloop, Unloop)).first->second;
+      SubloopParents.insert(std::make_pair(Subloop, &Unloop)).first->second;
   }
 
   succ_iterator I = succ_begin(BB), E = succ_end(BB);
   if (I == E) {
     assert(!Subloop && "subloop blocks must have a successor");
     NearLoop = nullptr; // unloop blocks may now exit the function.
   }
   for (; I != E; ++I) {
     if (*I == BB)
       continue; // self loops are uninteresting
 
     Loop *L = LI->getLoopFor(*I);
-    if (L == Unloop) {
+    if (L == &Unloop) {
       // This successor has not been processed. This path must lead to an
       // irreducible backedge.
       assert((FoundIB || !DFS.hasPostorder(*I)) && "should have seen IB");
       FoundIB = true;
     }
-    if (L != Unloop && Unloop->contains(L)) {
+    if (L != &Unloop && Unloop.contains(L)) {
       // Successor is in a subloop.
       if (Subloop)
         continue; // Branching within subloops. Ignore it.
 
       // BB branches from the original into a subloop header.
-      assert(L->getParentLoop() == Unloop && "cannot skip into nested loops");
+      assert(L->getParentLoop() == &Unloop && "cannot skip into nested loops");
 
       // Get the current nearest parent of the Subloop's exits.
       L = SubloopParents[L];
       // L could be Unloop if the only exit was an irreducible backedge.
     }
-    if (L == Unloop) {
+    if (L == &Unloop) {
       continue;
     }
     // Handle critical edges from Unloop into a sibling loop.
-    if (L && !L->contains(Unloop)) {
+    if (L && !L->contains(&Unloop)) {
       L = L->getParentLoop();
     }
     // Remember the nearest parent loop among successors or subloop exits.
-    if (NearLoop == Unloop || !NearLoop || NearLoop->contains(L))
+    if (NearLoop == &Unloop || !NearLoop || NearLoop->contains(L))
       NearLoop = L;
   }
   if (Subloop) {
     SubloopParents[Subloop] = NearLoop;
     return BBLoop;
   }
   return NearLoop;
 }

lib/Analysis/ScalarEvolution.cpp

if (ExtraInfoSize > 0) {
     ENT = new ExitNotTakenExtras[ExtraInfoSize];
     ExitNotTaken.ExtraInfo = &ENT[0];
     *ExitNotTaken.getPred() = std::move(ExitCounts[0].Pred);
   }
 
   if (NumExits == 1)
     return;
 
+  assert(ENT && "ExitNotTakenExtras is NULL while having more than one exit");
+
   auto &Exits = ExitNotTaken.ExtraInfo->Exits;
 
   // Handle the rare case of multiple computable exits.
   for (unsigned i = 1, PredPos = 1; i < NumExits; ++i) {
     ExitNotTakenExtras *Ptr = nullptr;
     if (!ExitCounts[i].Pred.isAlwaysTrue()) {
       Ptr = &ENT[PredPos++];
       Ptr->Pred = std::move(ExitCounts[i].Pred);