[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 ↗

(On Diff #217089)

Mmm, how about we soften this warning? :)

"Enabling this may result in some false positive finds.

646 ↗

(On Diff #217089)

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

223 ↗

(On Diff #217089)

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 ↗

(On Diff #217164)

several?

clang/test/Analysis/dead-stores.c

81 ↗

(On Diff #217164)

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

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

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

[Szelethus]

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

133

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

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.

cfe/trunk/docs/analyzer/checkers.rst

 
 .. code-block:: c
 
  void test() {
    int x;
    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
 ^^^^^^^^^^^
 
 Objective C checkers that warn for null pointer passing and dereferencing errors.
 
 .. _nullability-NullPassedToNonnull:

cfe/trunk/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.",
+                  "true",
+                  Released>
+  ]>,
   Documentation<HasDocumentation>;
 
 } // end DeadCode
 
 let ParentPackage = DeadCodeAlpha in {
 
 def UnreachableCodeChecker : Checker<"UnreachableCode">,
   HelpText<"Check unreachable code">,

cfe/trunk/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 = true;
+
   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;
 }

cfe/trunk/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 = true
 // 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 = true
 // 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 = 90
+// CHECK-NEXT: num-entries = 91

cfe/trunk/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 -fblocks -Wno-unreachable-code     \
+// RUN:  -analyzer-checker=core,deadcode.DeadStores                             \
+// RUN:  -analyzer-config deadcode.DeadStores:WarnForDeadNestedAssignments=false\
+// RUN:  -analyzer-opt-analyze-nested-blocks -verify=non-nested %s
+//
+// RUN: %clang_analyze_cc1 -Wunused-variable -fblocks -Wno-unreachable-code     \
+// RUN:  -analyzer-checker=core,deadcode.DeadStores                             \
+// RUN:  -analyzer-config deadcode.DeadStores:WarnForDeadNestedAssignments=false\
+// RUN:  -analyzer-opt-analyze-nested-blocks -verify=non-nested                 \
+// RUN:  -analyzer-store=region %s
+//
+// RUN: %clang_analyze_cc1 -Wunused-variable -fblocks -Wno-unreachable-code     \
+// RUN:  -analyzer-checker=core,deadcode.DeadStores                             \
+// RUN:  -analyzer-opt-analyze-nested-blocks -verify=non-nested,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'}}
+  int k, y; // non-nested-warning {{unused variable 'k'}}
+            // non-nested-warning@-1 {{unused variable 'y'}}
+  int abc = 1;
+  long idx = abc + 3 * 5; // non-nested-warning {{never read}}
+                          // non-nested-warning@-1 {{unused variable 'idx'}}
 }
 
 void f2(void *b) {
- char *c = (char*)b; // no-warning
- char *d = b+1; // expected-warning {{never read}} expected-warning{{unused variable 'd'}}
- printf("%s", c); // expected-warning{{implicitly declaring library function 'printf' with type 'int (const char *, ...)'}} \
- // expected-note{{include the header <stdio.h> or explicitly provide a declaration for 'printf'}}
+  char *c = (char *)b; // no-warning
+  char *d = b + 1;     // non-nested-warning {{never read}}
+                       // non-nested-warning@-1 {{unused variable 'd'}}
+  printf("%s", c);
+  // non-nested-warning@-1 {{implicitly declaring library function 'printf' with type 'int (const char *, ...)'}}
+  // non-nested-note@-2 {{include the header <stdio.h> or explicitly provide a declaration for 'printf'}}
 }
 
 int f();
-
 void f3() {
   int r;
   if ((r = f()) != 0) { // no-warning
-    int y = r; // no-warning
+    int y = r;          // no-warning
     printf("the error is: %d\n", y);
   }
 }
 
 void f4(int k) {
-  
   k = 1;
-  
   if (k)
     f1();
-    
-  k = 2;  // expected-warning {{never read}}
+  k = 2; // non-nested-warning {{never read}}
 }
-  
 void f5() {
-
-  int x = 4; // no-warning
-  int *p = &x; // expected-warning{{never read}} expected-warning{{unused variable 'p'}}
-
+  int x = 4;   // no-warning
+  int *p = &x; // non-nested-warning {{never read}}
+               // non-nested-warning@-1 {{unused variable 'p'}}
 }
 
-//
 int f6() {
-  
   int x = 4;
   ++x; // no-warning
   return 1;
 }
 
-int f7(int *p) {  
+int f7(int *p) {
   // This is allowed for defensive programming.
-  p = 0; // no-warning  
+  p = 0; // no-warning
   return 1;
 }
 
-int f7b(int *p) {  
+int f7b(int *p) {
   // This is allowed for defensive programming.
-  p = (0); // no-warning  
+  p = (0); // no-warning
   return 1;
 }
 
-int f7c(int *p) {  
+int f7c(int *p) {
   // This is allowed for defensive programming.
-  p = (void*) 0; // no-warning  
+  p = (void *)0; // no-warning
   return 1;
 }
 
-int f7d(int *p) {  
+int f7d(int *p) {
   // This is allowed for defensive programming.
-  p = (void*) (0); // no-warning  
+  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.
+// Warn for dead stores in nested expressions.
 int f8(int *p) {
   extern int *baz();
-  if ((p = baz())) // no-warning
+  if ((p = baz())) // nested-warning {{Although the value stored}}
     return 1;
   return 0;
 }
 
 int f9() {
   int x = 4;
-  x = x + 10; // expected-warning{{never read}}
+  x = x + 10; // non-nested-warning {{never read}}
   return 1;
 }
 
 int f10() {
   int x = 4;
-  x = 10 + x; // expected-warning{{never read}}
+  x = 10 + x; // non-nested-warning {{never read}}
   return 1;
 }
 
 int f11() {
   int x = 4;
-  return x++; // expected-warning{{never read}}
+  return x++; // non-nested-warning {{never read}}
 }
 
 int f11b() {
   int x = 4;
   return ((((++x)))); // no-warning
 }
 
 int f12a(int y) {
-  int x = y;  // expected-warning{{unused variable 'x'}}
+  int x = y; // non-nested-warning {{unused variable 'x'}}
   return 1;
 }
+
 int f12b(int y) {
-  int x __attribute__((unused)) = y;  // no-warning
+  int x __attribute__((unused)) = y; // no-warning
   return 1;
 }
+
 int f12c(int y) {
   // Allow initialiation of scalar variables by parameters as a form of
   // defensive programming.
-  int x = y;  // no-warning
+  int x = y; // no-warning
   x = 1;
   return x;
 }
 
 // Filed with PR 2630.  This code should produce no warnings.
-int f13(void)
-{
+int f13(void) {
   int a = 1;
   int b, c = b = a + a;
 
   if (b > 0)
     return (0);
-
   return (a + b + c);
 }
 
 // Filed with PR 2763.
 int f14(int count) {
   int index, nextLineIndex;
-  for (index = 0; index < count; index = nextLineIndex+1) {
-    nextLineIndex = index+1;  // no-warning
+  for (index = 0; index < count; index = nextLineIndex + 1) {
+    nextLineIndex = index + 1; // no-warning
     continue;
   }
   return index;
 }
 
 // Test case for <rdar://problem/6248086>
 void f15(unsigned x, unsigned y) {
-  int count = x * y;   // no-warning
-  int z[count]; // expected-warning{{unused variable 'z'}}
+  int count = x * y; // no-warning
+  int z[count];      // non-nested-warning {{unused variable 'z'}}
 }
 
-// Don't warn for dead stores in nested expressions.  We have yet
-// to see a real bug in this scenario.
+// Warn for dead stores in nested expressions.
 int f16(int x) {
   x = x * 2;
-  x = sizeof(int [x = (x || x + 1) * 2])
-      ? 5 : 8;
+  x = sizeof(int[x = (x || x + 1) * 2]) ? 5 : 8;
+  // nested-warning@-1 {{Although the value stored}}
   return x;
 }
 
 // Self-assignments should not be flagged as dead stores.
 void f17() {
   int x = 1;
   x = x;
 }
 
 // <rdar://problem/6506065>
 // The values of dead stores are only "consumed" in an enclosing expression
 // what that value is actually used.  In other words, don't say "Although the
 // value stored to 'x' is used...".
 int f18() {
-   int x = 0; // no-warning
-   if (1)
-      x = 10;  // expected-warning{{Value stored to 'x' is never read}}
-   while (1)
-      x = 10;  // expected-warning{{Value stored to 'x' is never read}}
-   // unreachable.
-   do
-      x = 10;   // no-warning
-   while (1);
-   return (x = 10); // no-warning
+    x = 10; // non-nested-warning {{Value stored to 'x' is never read}}
+    x = 10; // non-nested-warning {{Value stored to 'x' is never read}}
+    x = 10; // no-warning
 }
 
 int f18_a() {
-   int x = 0; // no-warning
-   return (x = 10); // no-warning
+  int x = 0;       // no-warning
+  return (x = 10); // nested-warning {{Although the value stored}}
 }
 
 void f18_b() {
-   int x = 0; // no-warning
-   if (1)
-      x = 10;  // expected-warning{{Value stored to 'x' is never read}}
+    x = 10; // non-nested-warning {{Value stored to 'x' is never read}}
 }
 
 void f18_c() {
   int x = 0;
   while (1)
-     x = 10;  // expected-warning{{Value stored to 'x' is never read}}
+    x = 10; // non-nested-warning {{Value stored to 'x' is never read}}
 }
 
 void f18_d() {
   int x = 0; // no-warning
   do
-     x = 10;   // expected-warning{{Value stored to 'x' is never read}}
+    x = 10; // non-nested-warning {{Value stored to 'x' is never read}}
   while (1);
 }
 
 // PR 3514: false positive `dead initialization` warning for init to global
 //  http://llvm.org/bugs/show_bug.cgi?id=3514
 extern const int MyConstant;
 int f19(void) {
-  int x = MyConstant;  // no-warning
+  int x = MyConstant; // no-warning
   x = 1;
   return x;
 }
 
 int f19b(void) { // This case is the same as f19.
   const int MyConstant = 0;
   int x = MyConstant; // no-warning
   x = 1;
-  return x;  
+  return x;
 }
 
 void f20(void) {
   int x = 1; // no-warning
 #pragma unused(x)
 }
 
 void halt() __attribute__((noreturn));
 int f21() {
   int x = 4;
-  
-  x = x + 1; // expected-warning{{never read}}
+  x = x + 1; // non-nested-warning {{never read}}
   if (1) {
     halt();
     (void)x;
   }
   return 1;
 }
 
 int j;
void f22() {
   int y14 = 4;
   int y15 = 4;
   int y16 = 4;
   int y17 = 4;
   int y18 = 4;
   int y19 = 4;
   int y20 = 4;
 
-  x = x + 1; // expected-warning{{never read}}
+  x = x + 1; // non-nested-warning {{never read}}
   ++y1;
   ++y2;
   ++y3;
   ++y4;
   ++y5;
   ++y6;
   ++y7;
   ++y8;
case 2:
     break;
   case 3:
     if (1) {
       (void)y3;
       return;
     } else
       (void)x;
     (void)x;
     break;
   case 4:
-    0 ? : ((void)y4, ({ return; }));
+    0 ?: ((void)y4, ({ return; }));
     (void)x;
     break;
   case 5:
-    1 ? : (void)x;
+    1 ?: (void)x;
     0 ? (void)x : ((void)y5, ({ return; }));
     (void)x;
     break;
   case 6:
     1 ? ((void)y6, ({ return; })) : (void)x;
     (void)x;
     break;
   case 7:
     (void)(0 && x);
     (void)y7;
-    (void)(0 || (y8, ({ return; }), 1));  // expected-warning {{expression result unused}}
+    (void)(0 || (y8, ({ return; }), 1));
+    // non-nested-warning@-1 {{expression result unused}}
     (void)x;
     break;
   case 8:
-    (void)(1 && (y9, ({ return; }), 1));  // expected-warning {{expression result unused}}
+    (void)(1 && (y9, ({ return; }), 1));
+    // non-nested-warning@-1 {{expression result unused}}
     (void)x;
     break;
   case 9:
     (void)(1 || x);
     (void)y10;
     break;
   case 10:
     while (0) {
do {
       (void)y15;
     } while (1);
     (void)x;
     break;
   case 14:
     for (;;) {
       (void)y16;
     }
-    (void)x;    
+    (void)x;
     break;
   case 15:
-    for (;1;) {
+    for (; 1;) {
       (void)y17;
     }
     (void)x;
     break;
   case 16:
-    for (;0;) {
+    for (; 0;) {
       (void)x;
     }
     (void)y18;
     break;
   case 17:
     __builtin_choose_expr(0, (void)x, ((void)y19, ({ return; })));
     (void)x;
     break;
   case 19:
     __builtin_choose_expr(1, ((void)y20, ({ return; })), (void)x);
     (void)x;
     break;
   }
 }
 
-void f23_aux(const char* s);
+void f23_aux(const char *s);
 void f23(int argc, char **argv) {
   int shouldLog = (argc > 1); // no-warning
-  ^{ 
-     if (shouldLog) f23_aux("I did too use it!\n");
-     else f23_aux("I shouldn't log.  Wait.. d'oh!\n");
+  ^{
+    if (shouldLog)
+      f23_aux("I did too use it!\n");
+    else
+      f23_aux("I shouldn't log.  Wait.. d'oh!\n");
   }();
 }
 
 void f23_pos(int argc, char **argv) {
-  int shouldLog = (argc > 1); // expected-warning{{Value stored to 'shouldLog' during its initialization is never read}} expected-warning{{unused variable 'shouldLog'}}
-  ^{ 
-     f23_aux("I did too use it!\n");
-  }();  
+  int shouldLog = (argc > 1);
+  // non-nested-warning@-1 {{Value stored to 'shouldLog' during its initialization is never read}}
+  // non-nested-warning@-2 {{unused variable 'shouldLog'}}
+  ^{
+  }();
 }
 
 void f24_A(int y) {
   // FIXME: One day this should be reported as dead since 'z = x + y' is dead.
   int x = (y > 2); // no-warning
-  ^ {
-      int z = x + y; // expected-warning{{Value stored to 'z' during its initialization is never read}} expected-warning{{unused variable 'z'}}
-  }();  
+  ^{
+    int z = x + y;
+    // non-nested-warning@-1 {{Value stored to 'z' during its initialization is never read}}
+    // non-nested-warning@-2 {{unused variable 'z'}}
+  }();
 }
 
 void f24_B(int y) {
   // FIXME: One day this should be reported as dead since 'x' is just overwritten.
   __block int x = (y > 2); // no-warning
   ^{
     // FIXME: This should eventually be a dead store since it is never read either.
     x = 5; // no-warning
   }();
 }
 
 int f24_C(int y) {
   // FIXME: One day this should be reported as dead since 'x' is just overwritten.
   __block int x = (y > 2); // no-warning
-  ^{ 
+  ^{
     x = 5; // no-warning
   }();
   return x;
 }
 
 int f24_D(int y) {
   __block int x = (y > 2); // no-warning
-  ^{ 
+  ^{
     if (y > 4)
       x = 5; // no-warning
   }();
   return x;
 }
 
-// This example shows that writing to a variable captured by a block means that it might
-// not be dead.
+// This example shows that writing to a variable captured by a block means that
+// it might not be dead.
 int f25(int y) {
   __block int x = (y > 2);
   __block int z = 0;
-  void (^foo)() = ^{ z = x + y; };
+  void (^foo)() = ^{
+    z = x + y;
+  };
   x = 4; // no-warning
   foo();
-  return z; 
+  return z;
 }
 
-// This test is mostly the same as 'f25', but shows that the heuristic of pruning out dead
-// stores for variables that are just marked '__block' is overly conservative.
+// This test is mostly the same as 'f25', but shows that the heuristic of
+// pruning out dead stores for variables that are just marked '__block' is
+// overly conservative.
 int f25_b(int y) {
   // FIXME: we should eventually report a dead store here.
   __block int x = (y > 2);
   __block int z = 0;
   x = 4; // no-warning
-  return z; 
+  return z;
 }
 
 int f26_nestedblocks() {
   int z;
   z = 1;
   __block int y = 0;
   ^{
     int k;
-    k = 1; // expected-warning{{Value stored to 'k' is never read}}
+    k = 1; // non-nested-warning {{Value stored to 'k' is never read}}
     ^{
-        y = z + 1;
-     }();
   }();
   return y;
 }
 
 // The FOREACH macro in QT uses 'break' statements within statement expressions
 // placed within the increment code of for loops.
 void rdar8014335() {
   for (int i = 0 ; i != 10 ; ({ break; })) {
-    for ( ; ; ({ ++i; break; })) ; // expected-warning {{'break' is bound to current loop, GCC binds it to the enclosing loop}}
+    for (;; ({ ++i; break; }))
+      ;
+    // non-nested-warning@-2 {{'break' is bound to current loop, GCC binds it to the enclosing loop}}
     // Note that the next value stored to 'i' is never executed
     // because the next statement to be executed is the 'break'
     // in the increment code of the first loop.
-    i = i * 3; // expected-warning{{Value stored to 'i' is never read}}
+    i = i * 3; // non-nested-warning {{Value stored to 'i' is never read}}
   }
 }
 
 // <rdar://problem/8320674> NullStmts followed by do...while() can lead to disconnected CFG
 //
 // This previously caused bogus dead-stores warnings because the body of the first do...while was
 // disconnected from the entry of the function.
 typedef struct { float r; float i; } s_rdar8320674;
void rdar8320674(s_rdar8320674 *z, unsigned y, s2_rdar8320674 *st, int m)
     }while (--m);
 }
 
 // Avoid dead stores resulting from an assignment (and use) being unreachable.
 void rdar8405222_aux(int i);
 void rdar8405222() {
   const int show = 0;
   int i = 0;
-    
   if (show)
-      i = 5; // no-warning
-
   if (show)
     rdar8405222_aux(i);
 }
 
 // Look through chains of assignments, e.g.: int x = y = 0, when employing
 // silencing heuristics.
 int radar11185138_foo() {
   int x, y;
-  x = y = 0; // expected-warning {{never read}}
+  x = y = 0; // non-nested-warning {{never read}}
   return y;
 }
 
 int rdar11185138_bar() {
   int y;
-  int x = y = 0; // no-warning
+  int x = y = 0; // nested-warning {{Although the value stored}}
   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 x0 = (getInt(), 0); // non-nested-warning {{unused variable 'x0'}}
+  int x1 = (getInt(), getInt());
+  // non-nested-warning@-1 {{Value stored to 'x1' during its initialization is never read}}
+  // non-nested-warning@-2 {{unused variable 'x1'}}
+
+  int x2 = (getInt(), getInt(), getInt());
+  // non-nested-warning@-1 {{Value stored to 'x2' during its initialization is never read}}
+  // non-nested-warning@-2 {{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'}}
+  x3 = (getInt(), getInt(), 0);
+  // non-nested-warning@-1 {{Value stored to 'x3' is never read}}
+
+  int x4 = (getInt(), (getInt(), 0));
+  // non-nested-warning@-1 {{unused variable 'x4'}}
+
   int y;
-  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'}}
-  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 x5 = (getInt(), (y = 0));
+  // non-nested-warning@-1 {{unused variable 'x5'}}
+  // nested-warning@-2 {{Although the value stored}}
+
+  int x6 = (getInt(), (y = getInt()));
+  // non-nested-warning@-1 {{Value stored to 'x6' during its initialization is never read}}
+  // non-nested-warning@-2 {{unused variable 'x6'}}
+  // nested-warning@-3 {{Although the value stored}}
+
+  int x7 = 0, x8 = getInt();
+  // non-nested-warning@-1 {{Value stored to 'x8' during its initialization is never read}}
+  // non-nested-warning@-2 {{unused variable 'x8'}}
+  // non-nested-warning@-3 {{unused variable 'x7'}}
+
+  int x9 = getInt(), x10 = 0;
+  // non-nested-warning@-1 {{Value stored to 'x9' during its initialization is never read}}
+  // non-nested-warning@-2 {{unused variable 'x9'}}
+  // non-nested-warning@-3 {{unused variable 'x10'}}
+
+  int m = getInt(), mm, mmm;
+  // non-nested-warning@-1 {{Value stored to 'm' during its initialization is never read}}
+  // non-nested-warning@-2 {{unused variable 'm'}}
+  // non-nested-warning@-3 {{unused variable 'mm'}}
+  // non-nested-warning@-4 {{unused variable 'mmm'}}
+
+  int n, nn = getInt();
+  // non-nested-warning@-1 {{Value stored to 'nn' during its initialization is never read}}
+  // non-nested-warning@-2 {{unused variable 'n'}}
+  // non-nested-warning@-3 {{unused variable 'nn'}}
 
   int *p;
   p = (getPtr(), (int *)0); // no warning
-
 }
 
 void testVolatile() {
-    volatile int v;
-    v = 0; // no warning
 }

cfe/trunk/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    \
+// RUN:  -analyzer-checker=deadcode.DeadStores -Wno-unreachable-code            \
+// RUN:  -analyzer-config deadcode.DeadStores:WarnForDeadNestedAssignments=false\
+// RUN:  -verify=non-nested %s
+//
+// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++11    \
+// RUN:  -analyzer-store=region -analyzer-checker=deadcode.DeadStores           \
+// RUN:  -analyzer-config deadcode.DeadStores:WarnForDeadNestedAssignments=false\
+// RUN:  -Wno-unreachable-code -verify=non-nested %s
+//
+// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++11    \
+// RUN:  -analyzer-checker=deadcode.DeadStores -Wno-unreachable-code            \
+// RUN:  -verify=non-nested,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}}
+  x = x + 1; // non-nested-warning {{never read}}
 
   switch (j) {
   case 1:
     throw 1;
     (void)x;
     break;
   }
+
+  int y;
+  (void)y;
+  if ((y = make_int())) // nested-warning {{Although the value stored}}
+    return;
 }
 
 //===----------------------------------------------------------------------===//
 // Dead store checking involving constructors.
 //===----------------------------------------------------------------------===//
 
 class Test2 {
   int &x;
+
 public:
   Test2(int &y) : x(y) {}
   ~Test2() { ++x; }
 };
 
 int test2(int x) {
   { Test2 a(x); } // no-warning
   return x;
void test2_b() {
   TestTemp::row<const char*> x; // no-warning
 }
 
 //===----------------------------------------------------------------------===//
 // Test references.
 //===----------------------------------------------------------------------===//
 
 void test3_a(int x) {
-   x = x + 1; // expected-warning{{never read}}
+  x = x + 1; // non-nested-warning {{never read}}
 }
 
 void test3_b(int &x) {
-  x = x + 1; // no-warninge
+  x = x + 1; // no-warning
 }
 
 void test3_c(int x) {
   int &y = x;
-  // Shows the limitation of dead stores tracking.  The write is really
-  // dead since the value cannot escape the function.
+  // Shows the limitation of dead stores tracking. The write is really dead
+  // since the value cannot escape the function.
   ++y; // no-warning
 }
 
 void test3_d(int &x) {
   int &y = x;
   ++y; // no-warning
 }
 
 void test3_e(int &x) {
   int &y = x;
 }
 
 //===----------------------------------------------------------------------===//
 // Dead stores involving 'new'
 //===----------------------------------------------------------------------===//
 
 static void test_new(unsigned n) {
-  char **p = new char* [n]; // expected-warning{{never read}}
+  char **p = new char *[n]; // non-nested-warning {{never read}}
 }
 
 //===----------------------------------------------------------------------===//
 // Dead stores in namespaces.
 //===----------------------------------------------------------------------===//
 
 namespace foo {
-  int test_4(int x) {
-    x = 2; // expected-warning{{Value stored to 'x' is never read}}
-    x = 2;
-    return x;
-  }
+  x = 2; // non-nested-warning {{Value stored to 'x' is never read}}
 }
 
 //===----------------------------------------------------------------------===//
 // Dead stores in with EH code.
 //===----------------------------------------------------------------------===//
 
 void test_5_Aux();
 int test_5() {
   int x = 0;
   try {
     x = 2; // no-warning
     test_5_Aux();
-  }
-  catch (int z) {
+  } catch (int z) {
     return x + z;
   }
   return 1;
 }
 
-
 int test_6_aux(unsigned x);
-
 void test_6() {
-  unsigned currDestLen = 0;  // no-warning
+  unsigned currDestLen = 0; // no-warning
   try {
     while (test_6_aux(currDestLen)) {
       currDestLen += 2; // no-warning
-    } 
+    }
+  } catch (void *) {
   }
-  catch (void *) {}
 }
 
 void test_6b() {
-  unsigned currDestLen = 0;  // no-warning
+  unsigned currDestLen = 0; // no-warning
   try {
     while (test_6_aux(currDestLen)) {
-      currDestLen += 2; // expected-warning {{Value stored to 'currDestLen' is never read}}
+      currDestLen += 2;
+      // non-nested-warning@-1 {{Value stored to 'currDestLen' is never read}}
       break;
-    } 
+    }
+  } catch (void *) {
   }
-  catch (void *) {}
 }
 
-
 void testCXX11Using() {
   using Int = int;
   Int value;
-  value = 1; // expected-warning {{never read}}
+  value = 1; // non-nested-warning {{never read}}
 }
 
 //===----------------------------------------------------------------------===//
 // Dead stores in template instantiations (do not warn).
 //===----------------------------------------------------------------------===//
 
 template <bool f> int radar13213575_testit(int i) {
   int x = 5+i; // warning: Value stored to 'x' during its initialization is never read
   int y = 7;
   if (f)
     return x;
   else
     return y;
 }
 
 int radar_13213575() {
   return radar13213575_testit<true>(5) + radar13213575_testit<false>(3);
 }
 
 template <class T>
 void test_block_in_dependent_context(typename T::some_t someArray) {
   ^{
-     int i = someArray[0]; // no-warning
   }();
 }
 
 void test_block_in_non_dependent_context(int *someArray) {
   ^{
-     int i = someArray[0]; // expected-warning {{Value stored to 'i' during its initialization is never read}}
+    int i = someArray[0];
+    // non-nested-warning@-1 {{Value stored to 'i' during its initialization is never read}}
   }();
 }
 
 
 //===----------------------------------------------------------------------===//
 // Dead store checking involving lambdas.
 //===----------------------------------------------------------------------===//
 

cfe/trunk/test/Analysis/dead-stores.m

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core -analyzer-checker=deadcode.DeadStores,osx.cocoa.RetainCount -fblocks -verify -Wno-objc-root-class %s
-// expected-no-diagnostics
 
 typedef signed char BOOL;
 typedef unsigned int NSUInteger;
 typedef struct _NSZone NSZone;
 @class NSInvocation, NSMethodSignature, NSCoder, NSString, NSEnumerator;
 @protocol NSObject  - (BOOL)isEqual:(id)object; @end
 @protocol NSCopying  - (id)copyWithZone:(NSZone *)zone; @end
 @protocol NSCoding  - (void)encodeWithCoder:(NSCoder *)aCoder; @end

 @end
 
 @interface Rdar7947686_B : Rdar7947686
 - (id) init;
 @end
 
 @implementation Rdar7947686_B
 - (id) init {
-  id x = (self = [super init]); // no-warning
+  id x = (self = [super init]);
+  // expected-warning@-1 {{Although the value stored to 'self'}}
   return x;
 }
 @end
 
 // Don't flag dead stores when a variable is captured in a block used
 // by a property access.
 @interface RDar10591355
 @property (assign) int x;