[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

515

Do we really need three of these?

lib/Analysis/ScalarEvolution.cpp

5557

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

551

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

[apelete]

lib/Analysis/LoopInfo.cpp

551

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

551

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

551

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

 
 /// 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) {
 
+  assert(Unloop && "Unloop cannot be NULL");
+
   // Initially for blocks directly contained by Unloop, NearLoop == Unloop and
   // is considered uninitialized.
   Loop *NearLoop = BBLoop;
 
   Loop *Subloop = nullptr;
+  assert(Unloop && "does not contain any loop");
   if (NearLoop != Unloop && Unloop->contains(NearLoop)) {
     Subloop = NearLoop;
     // Find the subloop ancestor that is directly contained within 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.
for (; I != E; ++I) {
 
     Loop *L = LI->getLoopFor(*I);
     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;
     }
+    assert(Unloop && "does not contain any loop");
     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");
 

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 array is NULL, cannot further compute exits");
+
   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);