[analyzer] Add a checker option to detect nested dead stores

Enables the users to specify an optional flag which would warn for more dead
stores.
Previously it ignored if the dead store happened e.g. in an if condition.

if ((X = generate())) { // dead store to X
}

This patch introduces the `WarnForDeadNestedAssignments` option to the checker,
which is `false` by default - so this change would not affect any previous
users.
I have updated the code, tests and the docs as well. If I missed something, tell
me.

I also ran the analysis on Clang which generated 14 more reports compared to the
unmodified version. All of them seemed reasonable for me.

Related previous patches:
rGf224820b45c6847b91071da8d7ade59f373b96f3

Reviewers: NoQ, krememek, Szelethus, baloghadamsoftware

Reviewed By: Szelethus

Patch by Balázs Benics!

Differential Revision: https://reviews.llvm.org/D66733

llvm-svn: 370767

Author: Kristof Umann

Diff for: clang/docs/analyzer/checkers.rst

@@ -319,6 +319,15 @@ Check for values stored to variables that are never read afterwards.
    x = 1; // warn
  }
 
+The ``WarnForDeadNestedAssignments`` option enables the checker to emit
+warnings for nested dead assignments. You can disable with the
+``-analyzer-config deadcode.DeadStores:WarnForDeadNestedAssignments=false``.
+*Defaults to true*.
+
+Would warn for this e.g.:
+if ((y = make_int())) {
+}
+
 .. _nullability-checkers:
 
 nullability

Diff for: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

@@ -648,6 +648,14 @@ let ParentPackage = DeadCode in {
 def DeadStoresChecker : Checker<"DeadStores">,
   HelpText<"Check for values stored to variables that are never read "
            "afterwards">,
+  CheckerOptions<[
+    CmdLineOption<Boolean,
+                  "WarnForDeadNestedAssignments",
+                  "Warns for deadstores in nested assignments."
+                  "E.g.: if ((P = f())) where P is unused.",
+                  "true",
+                  Released>
+  ]>,
   Documentation<HasDocumentation>;
 
 } // end DeadCode

Diff for: clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp

@@ -130,16 +130,19 @@ class DeadStoreObs : public LiveVariables::Observer {
   std::unique_ptr<ReachableCode> reachableCode;
   const CFGBlock *currentBlock;
   std::unique_ptr<llvm::DenseSet<const VarDecl *>> InEH;
+  const bool WarnForDeadNestedAssignments;
 
   enum DeadStoreKind { Standard, Enclosing, DeadIncrement, DeadInit };
 
 public:
   DeadStoreObs(const CFG &cfg, ASTContext &ctx, BugReporter &br,
                const CheckerBase *checker, AnalysisDeclContext *ac,
                ParentMap &parents,
-               llvm::SmallPtrSet<const VarDecl *, 20> &escaped)
+               llvm::SmallPtrSet<const VarDecl *, 20> &escaped,
+               bool warnForDeadNestedAssignments)
       : cfg(cfg), Ctx(ctx), BR(br), Checker(checker), AC(ac), Parents(parents),
-        Escaped(escaped), currentBlock(nullptr) {}
+        Escaped(escaped), currentBlock(nullptr),
+        WarnForDeadNestedAssignments(warnForDeadNestedAssignments) {}
 
   ~DeadStoreObs() override {}
 
@@ -217,11 +220,16 @@ class DeadStoreObs : public LiveVariables::Observer {
         os << "Value stored to '" << *V << "' is never read";
         break;
 
+      // eg.: f((x = foo()))
       case Enclosing:
-        // Don't report issues in this case, e.g.: "if (x = foo())",
-        // where 'x' is unused later.  We have yet to see a case where
-        // this is a real bug.
-        return;
+        if (!WarnForDeadNestedAssignments)
+          return;
+        BugType = "Dead nested assignment";
+        os << "Although the value stored to '" << *V
+           << "' is used in the enclosing expression, the value is never "
+              "actually read from '"
+           << *V << "'";
+        break;
     }
 
     BR.EmitBasicReport(AC->getDecl(), Checker, BugType, "Dead store", os.str(),
@@ -474,6 +482,8 @@ class FindEscaped {
 namespace {
 class DeadStoresChecker : public Checker<check::ASTCodeBody> {
 public:
+  bool WarnForDeadNestedAssignments = true;
+
   void checkASTCodeBody(const Decl *D, AnalysisManager& mgr,
                         BugReporter &BR) const {
 
@@ -491,15 +501,20 @@ class DeadStoresChecker : public Checker<check::ASTCodeBody> {
       ParentMap &pmap = mgr.getParentMap(D);
       FindEscaped FS;
       cfg.VisitBlockStmts(FS);
-      DeadStoreObs A(cfg, BR.getContext(), BR, this, AC, pmap, FS.Escaped);
+      DeadStoreObs A(cfg, BR.getContext(), BR, this, AC, pmap, FS.Escaped,
+                     WarnForDeadNestedAssignments);
       L->runOnAllBlocks(A);
     }
   }
 };
 }
 
-void ento::registerDeadStoresChecker(CheckerManager &mgr) {
-  mgr.registerChecker<DeadStoresChecker>();
+void ento::registerDeadStoresChecker(CheckerManager &Mgr) {
+  auto Chk = Mgr.registerChecker<DeadStoresChecker>();
+
+  const AnalyzerOptions &AnOpts = Mgr.getAnalyzerOptions();
+  Chk->WarnForDeadNestedAssignments =
+      AnOpts.getCheckerBooleanOption(Chk, "WarnForDeadNestedAssignments");
 }
 
 bool ento::shouldRegisterDeadStoresChecker(const LangOptions &LO) {

Diff for: clang/test/Analysis/analyzer-config.c

@@ -30,6 +30,7 @@
 // CHECK-NEXT: ctu-dir = ""
 // CHECK-NEXT: ctu-import-threshold = 100
 // CHECK-NEXT: ctu-index-name = externalDefMap.txt
+// CHECK-NEXT: deadcode.DeadStores:WarnForDeadNestedAssignments = true
 // CHECK-NEXT: debug.AnalysisOrder:* = false
 // CHECK-NEXT: debug.AnalysisOrder:Bind = false
 // CHECK-NEXT: debug.AnalysisOrder:EndFunction = false
@@ -93,4 +94,4 @@
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 90
+// CHECK-NEXT: num-entries = 91