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

[steakhal]

Summary

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.
Shouldn't we enable this option by default?
You can check those at this CodeChecker instance. Note that this link will no longer work after the patch has been accepted.

Related previous patches:
rGf224820b45c6847b91071da8d7ade59f373b96f3

[Szelethus]

I really-really like this change. The results look great, though I'm not sure what happened here, are we sure that this originates from the analyzer, and is not some storing error?

In any case, please use clang-format-diff.py on this patch, otherwise LGTM.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

644

Mmm, how about we soften this warning? :)

"Enabling this may result in some false positive finds.

646

We mark checker options that are still in development with InAlpha, but I feel like this feature is as good as it gets. Feel free to mark it as Released, so it'll be user facing!

clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp

225–233

Could you please move this comment to the previous line?

[steakhal]

Thank you for your response @Szelethus.
Fixed, updating the patch.

[steakhal]

Reformatted using clang-format-diff.py.
Minor fixes which were requested by @Szelethus.

[steakhal]

Fix copy-paste mistake.
This time upload the correct version.

[steakhal]

@NoQ What do you think, should it be under a flag (as it would be now), or enabled by default?
I think these warnings are valuable and we should consider it enabling by default.
An interesting fact is that previously rGf224820b45c6847b91071da8d7ade59f373b96f3 patch disabled this warning saying that it generates too many false-positives without any real benefit.

But after seeing the reports on the LLVM codebase (CodeChecker instance for the diff) I still question this decision. What is your opinion?

Any comment how could this result in false-positives are welcomed.

[steakhal]

@Szelethus Your catch with the mispositioned report message is kinda strange. I will investigate that, but I think it's probably connected to something deeper, and most likely related to CodeChecker itself.

[steakhal]

@Szelethus The mispositioned report message was my fault. I used a different version of clang for the analysis and to upload the results, which resulted in some mispositioned reports.
I've fixed the linked CodeChecker instance.

[NoQ]

Mmm, i don't know what was that commit meant to fix. Your evaluation looks fairly sufficient for turning it on by default. Let's make it an on-by-default option and flip the flag back into off-by-default if it turns out to be somehow broken.

clang/docs/analyzer/checkers.rst

299

several?

clang/test/Analysis/dead-stores.c

81

Pls consider -verify=... for new tests (cf. D60732).

[Szelethus]

In D66733#1647964, @steakhal wrote:

@Szelethus The mispositioned report message was my fault. I used a different version of clang for the analysis and to upload the results, which resulted in some mispositioned reports.
I've fixed the linked CodeChecker instance.

If you want to see how the analyzer behaves, I advise to keep an LLVM repository purely for analysis that you're not ever refreshing, and a separate one for building your clang :)

[steakhal]

Fixes for @NoQ's comments.
I will update the patch.

[steakhal]

Changes:

• Flag option marked as 'enabled by default'.
• Reformat all the test cases for C, C++ and Obj C.
• Now uses -verify=tags approach.
• Fixes checker documentation.

[Szelethus]

Excellent detective work! Thanks! I'll do the honors.

[NoQ]

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

133 ↗

(On Diff #218461)

I suggest we adopt the idiom of passing the Checker object around and asking it about its options instead of passing each option around separately. This is easier and i don't see any downsides. Moreover, we already pass the Checker around (just type-erased for no good reason). If you don't mind, i'll remove this field as part of resolving merge conflicts in D65182.

[Szelethus]

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

133 ↗

(On Diff #218461)

What about subcheckers? In any case, feel free to remove it for now.

[Szelethus]

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

133 ↗

(On Diff #218461)

What type erasure do you talk about specifically? In any case, it might happen because of our library layout, I had a bad time with that when I did the checker registration thingie.

[NoQ]

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

133 ↗

(On Diff #218461)

I meant const DeadStoresChecker * -> const CheckerBase *.

What about subcheckers?

As long as they're represented by the same checker object that's defined within the same translation unit, this isn't a problem. When they're multiple checker objects - dunno, i've honestly never seen that happen (:

In any case, i don't think it's a hard requirement, just an always-easier-thing-to-implement.

[sylvestre.ledru]

@steakhal Thanks for this patch. I tried it on Firefox and it found a bunch of stuff we improved.
Some examples:
https://hg.mozilla.org/integration/autoland/rev/db24db8f5f549ff446d1c3c69799187bcc2409e2
https://hg.mozilla.org/integration/autoland/rev/5de53dab979a401d9ba1405974f691927e53c9bd
(and more to come)

I think it should be added to the release notes!

[sylvestre.ledru]

I added it to the release notes here : https://reviews.llvm.org/rC374593
I am wondering if the option( WarnForDeadNestedAssignments ) to disable it is really necessary?
I haven't seen any false positive while deadstore has some.

[sylvestre.ledru]

It now shows for the llvm toolchain: http://llvm.org/reports/scan-build/

[Szelethus]

In D66733#1706521, @sylvestre.ledru wrote:

I added it to the release notes here : https://reviews.llvm.org/rC374593
I am wondering if the option( WarnForDeadNestedAssignments ) to disable it is really necessary?
I haven't seen any false positive while deadstore has some.

Thank you!! Well, the option was in place in case some false positives do arise from this, but we did suspect that wouldn't happen. I'm not against removing it completely, or hiding it (so that it would only be listed under -analyzer-checker-option-help-developer).

Some of folks working on the analyzer are preparing for the conference, hence the slower response time, sorry :^)

[steakhal]

Thank you guys the responses. I cannot agree more. The sole reason why this option exists is that if you scroll back in the git blame of that line, you would find a commit, which removed this warning for this exact scenario.
Possibly due to some seemingly false positives.
I've added the author of that patch to the reviewers of this change, but did not respond.

If you interested to dig deeper, you could start here:
rC123394 same on github: https://github.com/steakhal/llvm-project/commit/f224820b45c6847b91071da8d7ade59f373b96f3

And again, thank you that mentioned it in the release notes and for the kind responses as well.

clang/docs/analyzer/checkers.rst

 
 .. code-block:: c
 
  void test() {
    int x;
    x = 1; // warn
  }
 
+**Options**
+
+This checker has several options which can be set from command line (e.g.
+``-analyzer-config deadcode.DeadStores:WarnForDeadNestedAssignments=true``):
+
+* ``WarnForDeadNestedAssignments`` (boolean). If set to false, the checker
+  won't emit warnings for nested dead assignments. When enabled likely reports
+  false-positives. *Defaults to false*.
+  Would warn for this e.g.:
+  if ((y = make_int())) {
+  }
+
 .. _nullability-checkers:
 
 nullability
 ^^^^^^^^^^^
 
 Objective C checkers that warn for null pointer passing and dereferencing errors.
 
 .. _nullability-NullPassedToNonnull:

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

 // Deadcode checkers.
 //===----------------------------------------------------------------------===//
 
 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."
+                  "Enabling this may result in some false positive finds.",
+                  "false",
+                  Released>
+  ]>,
   Documentation<HasDocumentation>;
 
 } // end DeadCode
 
 let ParentPackage = DeadCodeAlpha in {
 
 def UnreachableCodeChecker : Checker<"UnreachableCode">,
   HelpText<"Check unreachable code">,

clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp

class DeadStoreObs : public LiveVariables::Observer {
   BugReporter& BR;
   const CheckerBase *Checker;
   AnalysisDeclContext* AC;
   ParentMap& Parents;
   llvm::SmallPtrSet<const VarDecl*, 20> Escaped;
   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 {}
 
   bool isLive(const LiveVariables::LivenessValues &Live, const VarDecl *D) {
     if (Live.isLive(D))
       return true;
     // Lazily construct the set that records which VarDecls are in
     // EH code.
switch (dsk) {
       case DeadIncrement:
         BugType = "Dead increment";
         LLVM_FALLTHROUGH;
       case Standard:
         if (!BugType) BugType = "Dead assignment";
         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.
+        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(),
                        L, R);
   }
 
   void CheckVarDecl(const VarDecl *VD, const Expr *Ex, const Expr *Val,
                     DeadStoreKind dsk,

 
 //===----------------------------------------------------------------------===//
 // DeadStoresChecker
 //===----------------------------------------------------------------------===//
 
 namespace {
 class DeadStoresChecker : public Checker<check::ASTCodeBody> {
 public:
+  bool WarnForDeadNestedAssignments = false;
+
   void checkASTCodeBody(const Decl *D, AnalysisManager& mgr,
                         BugReporter &BR) const {
 
     // Don't do anything for template instantiations.
     // Proving that code in a template instantiation is "dead"
     // means proving that it is dead in all instantiations.
     // This same problem exists with -Wunreachable-code.
     if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D))
       if (FD->isTemplateInstantiation())
         return;
 
     if (LiveVariables *L = mgr.getAnalysis<LiveVariables>(D)) {
       CFG &cfg = *mgr.getCFG(D);
       AnalysisDeclContext *AC = mgr.getAnalysisDeclContext(D);
       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) {
   return true;
 }

clang/test/Analysis/analyzer-config.c

 // CHECK-NEXT: cfg-rich-constructors = true
 // CHECK-NEXT: cfg-scopes = false
 // CHECK-NEXT: cfg-temporary-dtors = true
 // CHECK-NEXT: cplusplus.Move:WarnOn = KnownsAndLocals
 // CHECK-NEXT: crosscheck-with-z3 = false
 // CHECK-NEXT: ctu-dir = ""
 // CHECK-NEXT: ctu-import-threshold = 100
 // CHECK-NEXT: ctu-index-name = externalDefMap.txt
+// CHECK-NEXT: deadcode.DeadStores:WarnForDeadNestedAssignments = false
 // CHECK-NEXT: debug.AnalysisOrder:* = false
 // CHECK-NEXT: debug.AnalysisOrder:Bind = false
 // CHECK-NEXT: debug.AnalysisOrder:EndFunction = false
 // CHECK-NEXT: debug.AnalysisOrder:LiveSymbols = false
 // CHECK-NEXT: debug.AnalysisOrder:NewAllocator = false
 // CHECK-NEXT: debug.AnalysisOrder:PostCall = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtArraySubscriptExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtCXXNewExpr = false

 // CHECK-NEXT: suppress-inlined-defensive-checks = true
 // CHECK-NEXT: suppress-null-return-paths = true
 // CHECK-NEXT: track-conditions = false
 // CHECK-NEXT: track-conditions-debug = false
 // CHECK-NEXT: unix.DynamicMemoryModeling:Optimistic = false
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 89
+// CHECK-NEXT: num-entries = 90

clang/test/Analysis/dead-stores.c

 // RUN: %clang_analyze_cc1 -Wunused-variable -analyzer-checker=core,deadcode.DeadStores -fblocks -verify -Wno-unreachable-code -analyzer-opt-analyze-nested-blocks %s
 // RUN: %clang_analyze_cc1 -Wunused-variable -analyzer-checker=core,deadcode.DeadStores -analyzer-store=region -fblocks -verify -Wno-unreachable-code -analyzer-opt-analyze-nested-blocks %s
+// RUN: %clang_analyze_cc1 -Wunused-variable -analyzer-checker=core,deadcode.DeadStores -analyzer-config deadcode.DeadStores:WarnForDeadNestedAssignments=true -fblocks -verify -Wno-unreachable-code -analyzer-opt-analyze-nested-blocks -DWARN_FOR_DEAD_NESTED %s
 
 void f1() {
   int k, y; // expected-warning{{unused variable 'k'}} expected-warning{{unused variable 'y'}}
   int abc=1;
   long idx=abc+3*5; // expected-warning {{never read}} expected-warning{{unused variable 'idx'}}
 }
 
 void f2(void *b) {
int f7d(int *p) {
   p = (void*) (0); // no-warning  
   return 1;
 }
 
 // Don't warn for dead stores in nested expressions.  We have yet
 // to see a real bug in this scenario.
 int f8(int *p) {
   extern int *baz();
+#ifdef WARN_FOR_DEAD_NESTED
+  if ((p = baz())) // expected-warning{{Although the value stored}}
+#else
   if ((p = baz())) // no-warning
+#endif
     return 1;
   return 0;
 }
 
 int f9() {
   int x = 4;
   x = x + 10; // expected-warning{{never read}}
   return 1;
void f15(unsigned x, unsigned y) {
   int count = x * y;   // no-warning
   int z[count]; // expected-warning{{unused variable 'z'}}
 }
 
 // Don't warn for dead stores in nested expressions.  We have yet
 // to see a real bug in this scenario.
 int f16(int x) {
   x = x * 2;
-  x = sizeof(int [x = (x || x + 1) * 2])
-      ? 5 : 8;
+#ifdef WARN_FOR_DEAD_NESTED
+  x = sizeof(int[x = (x || x + 1) * 2]) ? 5 : 8; // expected-warning{{Although the value stored}}
+#else
+  x = sizeof(int[x = (x || x + 1) * 2]) ? 5 : 8; // no-warning
+#endif
   return x;
 }
 
 // Self-assignments should not be flagged as dead stores.
 void f17() {
   int x = 1;
   x = x;
 }
int f18() {
    do
       x = 10;   // no-warning
    while (1);
    return (x = 10); // no-warning
 }
 
 int f18_a() {
    int x = 0; // no-warning
+#ifdef WARN_FOR_DEAD_NESTED
+   return (x = 10); // expected-warning{{Although the value stored}}
+#else
    return (x = 10); // no-warning
+#endif
 }
 
 void f18_b() {
    int x = 0; // no-warning
    if (1)
       x = 10;  // expected-warning{{Value stored to 'x' is never read}}
 }
 

 int radar11185138_foo() {
   int x, y;
   x = y = 0; // expected-warning {{never read}}
   return y;
 }
 
 int rdar11185138_bar() {
   int y;
+#ifdef WARN_FOR_DEAD_NESTED
+  int x = y = 0; // expected-warning{{Although the value stored}}
+#else
   int x = y = 0; // no-warning
+#endif
   x = 2;
   y = 2;
   return x + y;
 }
 
 int *radar11185138_baz() {
   int *x, *y;
   x = y = 0; // no-warning
   return y;
 }
 
 int getInt();
 int *getPtr();
 void testBOComma() {
   int x0 = (getInt(), 0); // expected-warning{{unused variable 'x0'}}
   int x1 = (getInt(), getInt()); // expected-warning {{Value stored to 'x1' during its initialization is never read}} // expected-warning{{unused variable 'x1'}}
   int x2 = (getInt(), getInt(), getInt()); //expected-warning{{Value stored to 'x2' during its initialization is never read}} // expected-warning{{unused variable 'x2'}}
   int x3;
   x3 = (getInt(), getInt(), 0); // expected-warning{{Value stored to 'x3' is never read}}
   int x4 = (getInt(), (getInt(), 0)); // expected-warning{{unused variable 'x4'}}
   int y;
+
+#ifdef WARN_FOR_DEAD_NESTED
+  int x5 = (getInt(), (y = 0));        // expected-warning{{unused variable 'x5'}} // expected-warning{{Although the value stored}}
+  int x6 = (getInt(), (y = getInt())); //expected-warning {{Value stored to 'x6' during its initialization is never read}} // expected-warning{{unused variable 'x6'}} // expected-warning{{Although the value stored}}
+#else
   int x5 = (getInt(), (y = 0)); // expected-warning{{unused variable 'x5'}}
   int x6 = (getInt(), (y = getInt())); //expected-warning {{Value stored to 'x6' during its initialization is never read}} // expected-warning{{unused variable 'x6'}}
+#endif
   int x7 = 0, x8 = getInt(); //expected-warning {{Value stored to 'x8' during its initialization is never read}} // expected-warning{{unused variable 'x8'}} // expected-warning{{unused variable 'x7'}}
   int x9 = getInt(), x10 = 0; //expected-warning {{Value stored to 'x9' during its initialization is never read}} // expected-warning{{unused variable 'x9'}}  // expected-warning{{unused variable 'x10'}}
   int m = getInt(), mm, mmm; //expected-warning {{Value stored to 'm' during its initialization is never read}} // expected-warning{{unused variable 'm'}} // expected-warning{{unused variable 'mm'}} // expected-warning{{unused variable 'mmm'}}
   int n, nn = getInt(); //expected-warning {{Value stored to 'nn' during its initialization is never read}} // expected-warning{{unused variable 'n'}} // expected-warning{{unused variable 'nn'}}
 
   int *p;
   p = (getPtr(), (int *)0); // no warning
 
 }
 
 void testVolatile() {
     volatile int v;
     v = 0; // no warning
 }

clang/test/Analysis/dead-stores.cpp

 // RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++11 -analyzer-checker=deadcode.DeadStores -verify -Wno-unreachable-code %s
 // RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++11 -analyzer-store=region -analyzer-checker=deadcode.DeadStores -verify -Wno-unreachable-code %s
+// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++11 -analyzer-checker=deadcode.DeadStores -analyzer-config deadcode.DeadStores:WarnForDeadNestedAssignments=true -verify -Wno-unreachable-code -DWARN_FOR_DEAD_NESTED %s
 
 //===----------------------------------------------------------------------===//
 // Basic dead store checking (but in C++ mode).
 //===----------------------------------------------------------------------===//
 
 int j;
+int make_int();
 void test1() {
   int x = 4;
 
   x = x + 1; // expected-warning{{never read}}
 
   switch (j) {
   case 1:
     throw 1;
     (void)x;
     break;
   }
+
+  int y;
+  (void)y;
+#ifdef WARN_FOR_DEAD_NESTED
+  if ((y = make_int())) // expected-warning{{Although the value stored}}
+#else
+  if ((y = make_int())) // no-warning
+#endif
+    return;
 }
 
 //===----------------------------------------------------------------------===//
 // Dead store checking involving constructors.
 //===----------------------------------------------------------------------===//
 
 class Test2 {
   int &x;