[analyzer] Minor fixes and improvements to debug.ExprInspection

[NoQ]

Summary

• Fix the bug with transition handling in checkDeadSymbols, that was noticed back in D18860, which never landed.
• Test this bug by adding a new function clang_analyzer_numTimesReached() to catch number of passes through the code, which should be handy for testing against unintended state splits.
• Add two more functions should help debugging issues quickly without running the debugger or dumping exploded graphs - clang_analyzer_dump() which dump()s an SVal argument to a warning message, and clang_analyzer_printState(), which dump()s the current program state to stderr.

[xazax.hun]

LGTM!

cfe/trunk/docs/analyzer/DebugChecks.rst

Example usage::
 
     if (true) {
       clang_analyzer_warnIfReached();  // expected-warning{{REACHABLE}}
     }
     else {
       clang_analyzer_warnIfReached();  // no-warning
     }
 
+- void clang_analyzer_numTimesReached();
+
+  Same as above, but include the number of times this call expression
+  gets reached by the analyzer during the current analysis.
+
+  Example usage::
+
+    for (int x = 0; x < 3; ++x) {
+      clang_analyzer_numTimesReached(); // expected-warning{{3}}
+    }
+
 - void clang_analyzer_warnOnDeadSymbol(int);
 
   Subscribe for a delayed warning when the symbol that represents the value of
   the argument is garbage-collected by the analyzer.
 
   When calling 'clang_analyzer_warnOnDeadSymbol(x)', if value of 'x' is a
   symbol, then this symbol is marked by the ExprInspection checker. Then,
   during each garbage collection run, the checker sees if the marked symbol is
Example usage::
     void clang_analyzer_explain(void *);
 
     void foo(int param, void *ptr) {
       clang_analyzer_explain(param); // expected-warning{{argument 'param'}}
       if (!ptr)
         clang_analyzer_explain(ptr); // expected-warning{{memory address '0'}}
     }
 
+- void clang_analyzer_dump(a single argument of any type);
+
+  Similar to clang_analyzer_explain, but produces a raw dump of the value,
+  same as SVal::dump().
+
+  Example usage::
+
+    void clang_analyzer_dump(int);
+    void foo(int x) {
+      clang_analyzer_dump(x); // expected-warning{{reg_$0<x>}}
+    }
+
 - size_t clang_analyzer_getExtent(void *);
 
   This function returns the value that represents the extent of a memory region
   pointed to by the argument. This value is often difficult to obtain otherwise,
   because no valid code that produces this value. However, it may be useful
   for testing purposes, to see how well does the analyzer model region extents.
 
   Example usage::
 
     void foo() {
       int x, *y;
       size_t xs = clang_analyzer_getExtent(&x);
       clang_analyzer_explain(xs); // expected-warning{{'4'}}
       size_t ys = clang_analyzer_getExtent(&y);
       clang_analyzer_explain(ys); // expected-warning{{'8'}}
     }
 
+- void clang_analyzer_printState();
+
+  Dumps the current ProgramState to the stderr. Quickly lookup the program state
+  at any execution point without ViewExplodedGraph or re-compiling the program.
+  This is not very useful for writing tests (apart from testing how ProgramState
+  gets printed), but useful for debugging tests. Also, this method doesn't
+  produce a warning, so it gets printed on the console before all other
+  ExprInspection warnings.
+
+  Example usage::
+
+    void foo() {
+      int x = 1;
+      clang_analyzer_printState(); // Read the stderr!
+    }
+
 Statistics
 ==========
 
 The debug.Stats checker collects various information about the analysis of each
 function, such as how many blocks were reached and if the analyzer timed out.
 
 There is also an additional -analyzer-stats flag, which enables various
 statistics within the analyzer engine. Note the Stats checker (which produces at
 least one bug report per function) may actually change the values reported by
 -analyzer-stats.

cfe/trunk/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp

 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Checkers/SValExplainer.h"
 #include "llvm/ADT/StringSwitch.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
-class ExprInspectionChecker : public Checker<eval::Call, check::DeadSymbols> {
+class ExprInspectionChecker : public Checker<eval::Call, check::DeadSymbols,
+                                             check::EndAnalysis> {
   mutable std::unique_ptr<BugType> BT;
 
+  // These stats are per-analysis, not per-branch, hence they shouldn't
+  // stay inside the program state.
+  struct ReachedStat {
+    ExplodedNode *ExampleNode;
+    unsigned NumTimesReached;
+  };
+  mutable llvm::DenseMap<const CallExpr *, ReachedStat> ReachedStats;
+
   void analyzerEval(const CallExpr *CE, CheckerContext &C) const;
   void analyzerCheckInlined(const CallExpr *CE, CheckerContext &C) const;
   void analyzerWarnIfReached(const CallExpr *CE, CheckerContext &C) const;
+  void analyzerNumTimesReached(const CallExpr *CE, CheckerContext &C) const;
   void analyzerCrash(const CallExpr *CE, CheckerContext &C) const;
   void analyzerWarnOnDeadSymbol(const CallExpr *CE, CheckerContext &C) const;
+  void analyzerDump(const CallExpr *CE, CheckerContext &C) const;
   void analyzerExplain(const CallExpr *CE, CheckerContext &C) const;
+  void analyzerPrintState(const CallExpr *CE, CheckerContext &C) const;
   void analyzerGetExtent(const CallExpr *CE, CheckerContext &C) const;
 
   typedef void (ExprInspectionChecker::*FnCheck)(const CallExpr *,
                                                  CheckerContext &C) const;
 
-  void reportBug(llvm::StringRef Msg, CheckerContext &C) const;
+  ExplodedNode *reportBug(llvm::StringRef Msg, CheckerContext &C) const;
+  ExplodedNode *reportBug(llvm::StringRef Msg, BugReporter &BR,
+                          ExplodedNode *N) const;
 
 public:
   bool evalCall(const CallExpr *CE, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
+  void checkEndAnalysis(ExplodedGraph &G, BugReporter &BR,
+                        ExprEngine &Eng) const;
 };
 }
 
 REGISTER_SET_WITH_PROGRAMSTATE(MarkedSymbols, SymbolRef)
 
 bool ExprInspectionChecker::evalCall(const CallExpr *CE,
                                      CheckerContext &C) const {
   // These checks should have no effect on the surrounding environment
   // (globals should not be invalidated, etc), hence the use of evalCall.
   FnCheck Handler = llvm::StringSwitch<FnCheck>(C.getCalleeName(CE))
     .Case("clang_analyzer_eval", &ExprInspectionChecker::analyzerEval)
     .Case("clang_analyzer_checkInlined",
           &ExprInspectionChecker::analyzerCheckInlined)
     .Case("clang_analyzer_crash", &ExprInspectionChecker::analyzerCrash)
     .Case("clang_analyzer_warnIfReached",
           &ExprInspectionChecker::analyzerWarnIfReached)
     .Case("clang_analyzer_warnOnDeadSymbol",
           &ExprInspectionChecker::analyzerWarnOnDeadSymbol)
     .Case("clang_analyzer_explain", &ExprInspectionChecker::analyzerExplain)
+    .Case("clang_analyzer_dump", &ExprInspectionChecker::analyzerDump)
     .Case("clang_analyzer_getExtent", &ExprInspectionChecker::analyzerGetExtent)
+    .Case("clang_analyzer_printState",
+          &ExprInspectionChecker::analyzerPrintState)
+    .Case("clang_analyzer_numTimesReached",
+          &ExprInspectionChecker::analyzerNumTimesReached)
     .Default(nullptr);
 
   if (!Handler)
     return false;
 
   (this->*Handler)(CE, C);
   return true;
 }
static const char *getArgumentValueString(const CallExpr *CE,
   } else {
     if (StFalse)
       return "FALSE";
     else
       llvm_unreachable("Invalid constraint; neither true or false.");
   }
 }
 
-void ExprInspectionChecker::reportBug(llvm::StringRef Msg,
+ExplodedNode *ExprInspectionChecker::reportBug(llvm::StringRef Msg,
                                                CheckerContext &C) const {
-  if (!BT)
-    BT.reset(new BugType(this, "Checking analyzer assumptions", "debug"));
-
   ExplodedNode *N = C.generateNonFatalErrorNode();
+  reportBug(Msg, C.getBugReporter(), N);
+  return N;
+}
+
+ExplodedNode *ExprInspectionChecker::reportBug(llvm::StringRef Msg,
+                                               BugReporter &BR,
+                                               ExplodedNode *N) const {
   if (!N)
-    return;
+    return nullptr;
+
+  if (!BT)
+    BT.reset(new BugType(this, "Checking analyzer assumptions", "debug"));
 
-  C.emitReport(llvm::make_unique<BugReport>(*BT, Msg, N));
+  BR.emitReport(llvm::make_unique<BugReport>(*BT, Msg, N));
+  return N;
 }
 
 void ExprInspectionChecker::analyzerEval(const CallExpr *CE,
                                          CheckerContext &C) const {
   const LocationContext *LC = C.getPredecessor()->getLocationContext();
 
   // A specific instantiation of an inlined function may have more constrained
   // values than can generally be assumed. Skip the check.
   if (LC->getCurrentStackFrame()->getParent() != nullptr)
     return;
 
   reportBug(getArgumentValueString(CE, C), C);
 }
 
 void ExprInspectionChecker::analyzerWarnIfReached(const CallExpr *CE,
                                                   CheckerContext &C) const {
   reportBug("REACHABLE", C);
 }
 
+void ExprInspectionChecker::analyzerNumTimesReached(const CallExpr *CE,
+                                                    CheckerContext &C) const {
+  ++ReachedStats[CE].NumTimesReached;
+  if (!ReachedStats[CE].ExampleNode) {
+    // Later, in checkEndAnalysis, we'd throw a report against it.
+    ReachedStats[CE].ExampleNode = C.generateNonFatalErrorNode();
+  }
+}
+
 void ExprInspectionChecker::analyzerCheckInlined(const CallExpr *CE,
                                                  CheckerContext &C) const {
   const LocationContext *LC = C.getPredecessor()->getLocationContext();
 
   // An inlined function could conceivably also be analyzed as a top-level
   // function. We ignore this case and only emit a message (TRUE or FALSE)
   // when we are analyzing it as an inlined function. This means that
   // clang_analyzer_checkInlined(true) should always print TRUE, but
   // clang_analyzer_checkInlined(false) should never actually print anything.
   if (LC->getCurrentStackFrame()->getParent() == nullptr)
     return;
 
   reportBug(getArgumentValueString(CE, C), C);
 }
 
 void ExprInspectionChecker::analyzerExplain(const CallExpr *CE,
                                             CheckerContext &C) const {
-  if (CE->getNumArgs() == 0)
+  if (CE->getNumArgs() == 0) {
     reportBug("Missing argument for explaining", C);
+    return;
+  }
 
   SVal V = C.getSVal(CE->getArg(0));
   SValExplainer Ex(C.getASTContext());
   reportBug(Ex.Visit(V), C);
 }
 
+void ExprInspectionChecker::analyzerDump(const CallExpr *CE,
+                                         CheckerContext &C) const {
+  if (CE->getNumArgs() == 0) {
+    reportBug("Missing argument for dumping", C);
+    return;
+  }
+
+  SVal V = C.getSVal(CE->getArg(0));
+
+  llvm::SmallString<32> Str;
+  llvm::raw_svector_ostream OS(Str);
+  V.dumpToStream(OS);
+  reportBug(OS.str(), C);
+}
+
 void ExprInspectionChecker::analyzerGetExtent(const CallExpr *CE,
                                               CheckerContext &C) const {
-  if (CE->getNumArgs() == 0)
+  if (CE->getNumArgs() == 0) {
     reportBug("Missing region for obtaining extent", C);
+    return;
+  }
 
   auto MR = dyn_cast_or_null<SubRegion>(C.getSVal(CE->getArg(0)).getAsRegion());
-  if (!MR)
+  if (!MR) {
     reportBug("Obtaining extent of a non-region", C);
+    return;
+  }
 
   ProgramStateRef State = C.getState();
   State = State->BindExpr(CE, C.getLocationContext(),
                           MR->getExtent(C.getSValBuilder()));
   C.addTransition(State);
 }
 
+void ExprInspectionChecker::analyzerPrintState(const CallExpr *CE,
+                                               CheckerContext &C) const {
+  C.getState()->dump();
+}
+
 void ExprInspectionChecker::analyzerWarnOnDeadSymbol(const CallExpr *CE,
                                                      CheckerContext &C) const {
   if (CE->getNumArgs() == 0)
     return;
   SVal Val = C.getSVal(CE->getArg(0));
   SymbolRef Sym = Val.getAsSymbol();
   if (!Sym)
     return;
 
   ProgramStateRef State = C.getState();
   State = State->add<MarkedSymbols>(Sym);
   C.addTransition(State);
 }
 
 void ExprInspectionChecker::checkDeadSymbols(SymbolReaper &SymReaper,
                                              CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   const MarkedSymbolsTy &Syms = State->get<MarkedSymbols>();
+  ExplodedNode *N = C.getPredecessor();
   for (auto I = Syms.begin(), E = Syms.end(); I != E; ++I) {
     SymbolRef Sym = *I;
     if (!SymReaper.isDead(Sym))
       continue;
 
-    reportBug("SYMBOL DEAD", C);
+    // The non-fatal error node should be the same for all reports.
+    if (ExplodedNode *BugNode = reportBug("SYMBOL DEAD", C))
+      N = BugNode;
     State = State->remove<MarkedSymbols>(Sym);
   }
-  C.addTransition(State);
+  C.addTransition(State, N);
+}
+
+void ExprInspectionChecker::checkEndAnalysis(ExplodedGraph &G, BugReporter &BR,
+                                             ExprEngine &Eng) const {
+  for (auto Item: ReachedStats) {
+    unsigned NumTimesReached = Item.second.NumTimesReached;
+    ExplodedNode *N = Item.second.ExampleNode;
+
+    reportBug(std::to_string(NumTimesReached), BR, N);
+  }
 }
 
 void ExprInspectionChecker::analyzerCrash(const CallExpr *CE,
                                           CheckerContext &C) const {
   LLVM_BUILTIN_TRAP;
 }
 
 void ento::registerExprInspectionChecker(CheckerManager &Mgr) {
   Mgr.registerChecker<ExprInspectionChecker>();
 }
 

cfe/trunk/test/Analysis/expr-inspection.c

+// RUN: %clang_cc1 -analyze -analyzer-checker=debug.ExprInspection -verify %s 2>&1 | FileCheck %s
+
+// Self-tests for the debug.ExprInspection checker.
+
+void clang_analyzer_dump(int x);
+void clang_analyzer_printState();
+void clang_analyzer_numTimesReached();
+
+void foo(int x) {
+  clang_analyzer_dump(x); // expected-warning{{reg_$0<x>}}
+  int y = 1;
+  clang_analyzer_printState();
+  for (; y < 3; ++y)
+    clang_analyzer_numTimesReached(); // expected-warning{{2}}
+}
+
+// CHECK: Store (direct and default bindings)
+// CHECK-NEXT: (y,0,direct) : 1 S32b
+
+// CHECK: Expressions:
+// CHECK-NEXT: clang_analyzer_printState : &code{clang_analyzer_printState}
+// CHECK-NEXT: Ranges are empty.

cfe/trunk/test/Analysis/symbol-reaper.c

 // RUN: %clang_cc1 -analyze -analyzer-checker=debug.ExprInspection -verify %s
 
 void clang_analyzer_eval(int);
 void clang_analyzer_warnOnDeadSymbol(int);
+void clang_analyzer_numTimesReached();
 
 int conjure_index();
 
 void test_that_expr_inspection_works() {
   do {
     int x = conjure_index();
     clang_analyzer_warnOnDeadSymbol(x);
   } while(0); // expected-warning{{SYMBOL DEAD}}
+
+  // Make sure we don't accidentally split state in ExprInspection.
+  clang_analyzer_numTimesReached(); // expected-warning{{1}}
 }
 
 // These tests verify the reaping of symbols that are only referenced as
 // index values in element regions. Most of the time, depending on where
 // the element region, as Loc value, is stored, it is possible to
 // recover the index symbol in checker code, which is also demonstrated
 // in the return_ptr_range.c test file.