[analyzer] Treat C++ throw as sink during CFG-based suppress-on-sink.

[NoQ]

Summary

Because throws produce sinks during symbolic execution, we need to treat them as noreturn during our suppress-on-sink analysis as well. This would get a lot less conservative when CFG actually supports exceptions.

[a.sidorin]

Looks good.

[xazax.hun]

One minor nit, otherwise looks good to me.

lib/StaticAnalyzer/Core/BugReporter.cpp

3350

For temporary solutions like this, I prefer to have a FIXME string in the comment. In general, I think it is good to be able to keep track of future tasks by grepping for FIXMEs in the code of the analyzer.

[NoQ]

Rebase. Turn the comment into a FIXME.

[NoQ]

Bring back the test that was accidentally lost.

[dcoughlin]

Do we have a radar for this? It sounds familiar.

cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp

3313 ↗

(On Diff #108029)

Maybe a better name for this would be "treatAsSink"? This way people won't use it as short-hand for whether the block has a no-return element.

lib/StaticAnalyzer/Core/BugReporter.cpp

static const CFGBlock *findBlockForNode(const ExplodedNode *N) {
   // Find the node's current statement in the CFG.
   if (const Stmt *S = PathDiagnosticLocation::getStmt(N))
     return N->getLocationContext()->getAnalysisDeclContext()
                                   ->getCFGStmtMap()->getBlock(S);
 
   return nullptr;
 }
 
+static bool isNoReturnBlock(const CFGBlock *Blk) {
+  if (Blk->hasNoReturnElement())
+    return true;
+
+  // FIXME: Throw-expressions are currently generating sinks during analysis:
+  // they're not supported yet, and also often used for actually terminating
+  // the program. So we should treat them as sinks in this analysis as well,
+  // at least for now, but once we have better support for exceptions,
+  // we'd need to carefully handle the case when the throw is being
+  // immediately caught.
+  if (std::any_of(Blk->begin(), Blk->end(), [](const CFGElement &Elm) {
+        if (Optional<CFGStmt> StmtElm = Elm.getAs<CFGStmt>())
+          if (isa<CXXThrowExpr>(StmtElm->getStmt()))
+            return true;
+        return false;
+      }))
+    return true;
+
+  return false;
+}
+
 static bool isDominatedByNoReturnBlocks(const ExplodedNode *N) {
   const CFG &Cfg = N->getCFG();
 
   const CFGBlock *StartBlk = findBlockForNode(N);
   if (!StartBlk)
     return false;
-  if (StartBlk->hasNoReturnElement())
+  if (isNoReturnBlock(StartBlk))
     return true;
 
   llvm::SmallVector<const CFGBlock *, 32> DFSWorkList;
   llvm::SmallPtrSet<const CFGBlock *, 32> Visited;
 
   DFSWorkList.push_back(StartBlk);
   while (!DFSWorkList.empty()) {
     const CFGBlock *Blk = DFSWorkList.back();
     DFSWorkList.pop_back();
     Visited.insert(Blk);
 
     for (const auto &Succ : Blk->succs()) {
       if (const CFGBlock *SuccBlk = Succ.getReachableBlock()) {
         if (SuccBlk == &Cfg.getExit()) {
           // We seem to be leaving the current CFG.
           // We're no longer sure what happens next.
           return false;
         }
 
-        if (!SuccBlk->hasNoReturnElement() && !Visited.count(SuccBlk)) {
+        if (!isNoReturnBlock(SuccBlk) && !Visited.count(SuccBlk)) {
           // If the block has reachable child blocks that aren't no-return,
           // add them to the worklist.
           DFSWorkList.push_back(SuccBlk);
         }
       }
     }
   }
 

test/Analysis/max-nodes-suppress-on-sink.cpp

+// RUN: %clang_analyze_cc1 -x c++ -fcxx-exceptions -analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-config max-nodes=12 -verify %s
+
+// Here we test how "suppress on sink" feature of certain bugtypes interacts
+// with reaching analysis limits. See comments in max-nodes-suppress-on-sink.c
+// for more discussion.
+
+typedef __typeof(sizeof(int)) size_t;
+void *malloc(size_t);
+
+void clang_analyzer_warnIfReached(void);
+
+// Because we don't have a better approach, we currently treat throw as
+// noreturn.
+void test_throw_treated_as_noreturn() {
+  void *p = malloc(1); // no-warning
+
+  clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+  clang_analyzer_warnIfReached(); // no-warning
+
+  throw 0;
+}
+
+// FIXME: Handled throws shouldn't be suppressing us!
+void test_handled_throw_treated_as_noreturn() {
+  void *p = malloc(1); // no-warning
+
+  clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+  clang_analyzer_warnIfReached(); // no-warning
+
+  try {
+    throw 0;
+  } catch (int i) {
+  }
+}