[StaticAnalyzer] LoopUnrolling fixes

[szepet]

Summary

1. The LoopUnrolling feature needs the LoopExit included in the CFG so added this dependency via the config options
2. The LoopExit element can be encountered even if we haven't encountered the block of the corresponding LoopStmt. So the asserts were not right.
3. If we are caching out the Node then we get a nullptr from generateNode which case was not handled.

[NoQ]

Yeah, looks good.

Some tests would be great, at least for 2 (in what case do we not have the loop on the stack?)

[szepet]

Test added.

[zaks.anna]

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp

190

I would rather keep this method as is and add the extra logic elsewhere (ex: the place where includeLoopExitInCFG is used). To me, AnalyzerOptions::includeLoopExitInCFG() returns the value of the corresponding parameter and I would not expect it to use anything else.

test/Analysis/loop-unrolling.cpp

276

nit: "loop" "exit" and "loop" "stack" are separate words, so consider using "_" to separate them.

[szepet]

Update based on comments.

[NoQ]

LGTM!

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp

 }
 
 bool AnalyzerOptions::includeLifetimeInCFG() {
   return getBooleanOption(IncludeLifetimeInCFG, "cfg-lifetime",
                           /* Default = */ false);
 }
 
 bool AnalyzerOptions::includeLoopExitInCFG() {
-  return getBooleanOption(IncludeLoopExitInCFG, "cfg-loopexit",
+  bool explicitlyIncludeLoopExit = getBooleanOption(IncludeLoopExitInCFG,
+                                                  "cfg-loopexit",
                                                   /* Default = */ false);
+  return shouldUnrollLoops() || explicitlyIncludeLoopExit;
 }
 
 bool AnalyzerOptions::mayInlineCXXStandardLibrary() {
   return getBooleanOption(InlineCXXStandardLibrary,
                           "c++-stdlib-inlining",
                           /*Default=*/true);
 }
 

lib/StaticAnalyzer/Core/ExprEngine.cpp

void ExprEngine::processCFGBlockEntrance(const BlockEdge &L,
   PrettyStackTraceLocationContext CrashInfo(Pred->getLocationContext());
   // If we reach a loop which has a known bound (and meets
   // other constraints) then consider completely unrolling it.
   if(AMgr.options.shouldUnrollLoops()) {
     const Stmt *Term = nodeBuilder.getContext().getBlock()->getTerminator();
     if (Term) {
       ProgramStateRef NewState = updateLoopStack(Term, AMgr.getASTContext(),
                                                  Pred);
-      if (NewState != Pred->getState()){
-        Pred = nodeBuilder.generateNode(NewState, Pred);
+      if (NewState != Pred->getState()) {
+        ExplodedNode *UpdatedNode = nodeBuilder.generateNode(NewState, Pred);
+        if (!UpdatedNode)
+          return;
+        Pred = UpdatedNode;
       }
     }
     // Is we are inside an unrolled loop then no need the check the counters.
     if(isUnrolledState(Pred->getState()))
       return;
   }
 
   // If this block is terminated by a loop and it has already been visited the

lib/StaticAnalyzer/Core/LoopUnrolling.cpp

 //===--- LoopUnrolling.cpp - Unroll loops -----------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// This file contains functions which are used to decide if a loop worth to be
 /// unrolled. Moreover, these functions manages the stack of loop which is
 /// tracked by the ProgramState.
 ///
 //===----------------------------------------------------------------------===//
 
-#include "clang/Analysis/CFGStmtMap.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "clang/AST/ParentMap.h"
-#include "clang/AST/StmtVisitor.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/LoopUnrolling.h"
-#include "llvm/ADT/Statistic.h"
 
 using namespace clang;
 using namespace ento;
 using namespace clang::ast_matchers;
 
 struct LoopState {
 private:
   enum Kind { Normal, Unrolled } K;

 namespace ento {
 
 static bool isLoopStmt(const Stmt *S) {
   return S && (isa<ForStmt>(S) || isa<WhileStmt>(S) || isa<DoStmt>(S));
 }
 
 ProgramStateRef processLoopEnd(const Stmt *LoopStmt, ProgramStateRef State) {
   auto LS = State->get<LoopStack>();
-  assert(!LS.isEmpty() && "Loop not added to the stack.");
-  assert(LoopStmt == LS.getHead().getLoopStmt() &&
-         "Loop is not on top of the stack.");
-
+  if (!LS.isEmpty() && LS.getHead().getLoopStmt() == LoopStmt)
     State = State->set<LoopStack>(LS.getTail());
   return State;
 }
 
 static internal::Matcher<Stmt> simpleCondition(StringRef BindName) {
   return binaryOperator(
       anyOf(hasOperatorName("<"), hasOperatorName(">"), hasOperatorName("<="),
             hasOperatorName(">="), hasOperatorName("!=")),
       hasEitherOperand(ignoringParenImpCasts(

test/Analysis/loop-unrolling.cpp

if(i == 0 && b) {
       recursion_unroll4(false);
       continue;
     }
     clang_analyzer_numTimesReached(); // expected-warning {{14}}
   }
   int a = 22 / k;
   return 0;
 }
+
+int loopexit_while_empty_loopstack() {
+  if (getNum())
+    for (int i = 1; i < 8; i++)
+      ;
+  return 0;
+}