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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

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 :^)


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added a comment.

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


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added a comment.

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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added a comment.

@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!


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: cfe/trunk/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp:133
   std::unique_ptr> InEH;
+  const bool WarnForDeadNestedAssignments;
 

Szelethus wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > 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.
> > What about subcheckers? In any case, feel free to remove it for now.
> 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.
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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: cfe/trunk/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp:133
   std::unique_ptr> InEH;
+  const bool WarnForDeadNestedAssignments;
 

Szelethus wrote:
> NoQ wrote:
> > 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.
> What about subcheckers? In any case, feel free to remove it for now.
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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: cfe/trunk/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp:133
   std::unique_ptr> InEH;
+  const bool WarnForDeadNestedAssignments;
 

NoQ wrote:
> 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.
What about subcheckers? In any case, feel free to remove it for now.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: cfe/trunk/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp:133
   std::unique_ptr> InEH;
+  const bool WarnForDeadNestedAssignments;
 

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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kristóf Umann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL370767: [analyzer] Add a checker option to detect nested 
dead stores (authored by Szelethus, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66733?vs=217665=218461#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733

Files:
  cfe/trunk/docs/analyzer/checkers.rst
  cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
  cfe/trunk/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
  cfe/trunk/test/Analysis/analyzer-config.c
  cfe/trunk/test/Analysis/dead-stores.c
  cfe/trunk/test/Analysis/dead-stores.cpp
  cfe/trunk/test/Analysis/dead-stores.m

Index: cfe/trunk/test/Analysis/dead-stores.cpp
===
--- cfe/trunk/test/Analysis/dead-stores.cpp
+++ cfe/trunk/test/Analysis/dead-stores.cpp
@@ -1,15 +1,26 @@
-// 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:
@@ -17,6 +28,11 @@
 (void)x;
 break;
   }
+
+  int y;
+  (void)y;
+  if ((y = make_int())) // nested-warning {{Although the value stored}}
+return;
 }
 
 //===--===//
@@ -25,6 +41,7 @@
 
 class Test2 {
   int 
+
 public:
   Test2(int ) : x(y) {}
   ~Test2() { ++x; }
@@ -66,17 +83,17 @@
 //===--===//
 
 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 + 1; // no-warninge
+  x = x + 1; // no-warning
 }
 
 void test3_c(int x) {
   int  = 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
 }
 
@@ -94,7 +111,7 @@
 //===--===//
 
 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}}
 }
 
 //===--===//
@@ -102,11 +119,11 @@
 //===--===//
 
 namespace foo {
-  int test_4(int x) {
-x = 2; // expected-warning{{Value stored to 'x' is never read}}
-x = 2;
-return x;
-  }
+int test_4(int x) {
+  x = 2; // non-nested-warning {{Value stored to 'x' is never read}}
+  x = 2;
+  return x;
+}
 }
 
 //===--===//
@@ -119,42 +136,39 @@
   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 

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal updated this revision to Diff 217665.
steakhal added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/dead-stores.c
  clang/test/Analysis/dead-stores.cpp
  clang/test/Analysis/dead-stores.m

Index: clang/test/Analysis/dead-stores.m
===
--- clang/test/Analysis/dead-stores.m
+++ clang/test/Analysis/dead-stores.m
@@ -1,5 +1,4 @@
 // 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;
@@ -72,7 +71,8 @@
 
 @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
Index: clang/test/Analysis/dead-stores.cpp
===
--- clang/test/Analysis/dead-stores.cpp
+++ clang/test/Analysis/dead-stores.cpp
@@ -1,15 +1,26 @@
-// 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:
@@ -17,6 +28,11 @@
 (void)x;
 break;
   }
+
+  int y;
+  (void)y;
+  if ((y = make_int())) // nested-warning {{Although the value stored}}
+return;
 }
 
 //===--===//
@@ -25,6 +41,7 @@
 
 class Test2 {
   int 
+
 public:
   Test2(int ) : x(y) {}
   ~Test2() { ++x; }
@@ -66,17 +83,17 @@
 //===--===//
 
 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 + 1; // no-warninge
+  x = x + 1; // no-warning
 }
 
 void test3_c(int x) {
   int  = 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
 }
 
@@ -94,7 +111,7 @@
 //===--===//
 
 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}}
 }
 
 //===--===//
@@ -102,11 +119,11 @@
 //===--===//
 
 namespace foo {
-  int test_4(int x) {
-x = 2; // expected-warning{{Value stored to 'x' is never read}}
-x = 2;
-return x;
-  }
+int test_4(int x) {
+  x = 2; // non-nested-warning {{Value stored to 'x' is never read}}
+  x = 2;
+  return x;
+}
 }
 
 //===--===//
@@ -119,42 +136,39 @@
   try {
 x = 2; // no-warning
 test_5_Aux();
-  }
-  catch (int z) {
+  } catch (int z) {
 return x 

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal marked 2 inline comments as done.
steakhal added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

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 :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

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.




Comment at: clang/docs/analyzer/checkers.rst:299
+
+This checker has several options which can be set from command line (e.g.
+``-analyzer-config deadcode.DeadStores:WarnForDeadNestedAssignments=true``):

several?



Comment at: clang/test/Analysis/dead-stores.c:81
   extern int *baz();
+#ifdef WARN_FOR_DEAD_NESTED
+  if ((p = baz())) // expected-warning{{Although the value stored}}

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

@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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

@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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

@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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal updated this revision to Diff 217164.
steakhal added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/dead-stores.c
  clang/test/Analysis/dead-stores.cpp

Index: clang/test/Analysis/dead-stores.cpp
===
--- clang/test/Analysis/dead-stores.cpp
+++ clang/test/Analysis/dead-stores.cpp
@@ -1,11 +1,13 @@
 // 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;
 
@@ -17,6 +19,15 @@
 (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;
 }
 
 //===--===//
Index: clang/test/Analysis/dead-stores.c
===
--- clang/test/Analysis/dead-stores.c
+++ clang/test/Analysis/dead-stores.c
@@ -1,5 +1,6 @@
 // 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'}}
@@ -77,7 +78,11 @@
 // 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;
 }
@@ -152,8 +157,11 @@
 // 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;
 }
 
@@ -182,7 +190,11 @@
 
 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() {
@@ -535,7 +547,11 @@
 
 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;
@@ -557,8 +573,14 @@
   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'}}
   

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal updated this revision to Diff 217162.
steakhal added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/dead-stores.c
  clang/test/Analysis/dead-stores.cpp

Index: clang/test/Analysis/dead-stores.cpp
===
--- clang/test/Analysis/dead-stores.cpp
+++ clang/test/Analysis/dead-stores.cpp
@@ -1,11 +1,13 @@
 // 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;
 
@@ -17,6 +19,15 @@
 (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;
 }
 
 //===--===//
Index: clang/test/Analysis/dead-stores.c
===
--- clang/test/Analysis/dead-stores.c
+++ clang/test/Analysis/dead-stores.c
@@ -1,5 +1,6 @@
 // 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'}}
@@ -77,7 +78,11 @@
 // 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;
 }
@@ -152,8 +157,11 @@
 // 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;
 }
 
@@ -182,7 +190,11 @@
 
 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() {
@@ -535,7 +547,11 @@
 
 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;
@@ -557,8 +573,14 @@
   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'}} // 

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal marked 3 inline comments as done.
steakhal added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

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.




Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:644
+  "E.g.: if ((P = f())) where P is unused."
+  "It will likely report false positives.",
+  "false",

Mmm, how about we soften this warning? :)

`"Enabling this may result in some false positive finds.`



Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:646
+  "false",
+  InAlpha>
+  ]>,

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!



Comment at: clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp:223
 
-  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;
+  case Enclosing: // eg.:f((x = foo()))
+if (!WarnForDeadNestedAssignments)

Could you please move this comment to the previous line?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal created this revision.
steakhal added reviewers: NoQ, krememek, Szelethus, baloghadamsoftware.
Herald added subscribers: cfe-commits, Charusso, donat.nagy, dexonsmith, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, xazax.hun, whisperity.
Herald added a project: clang.

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 



Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66733

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/dead-stores.c
  clang/test/Analysis/dead-stores.cpp

Index: clang/test/Analysis/dead-stores.cpp
===
--- clang/test/Analysis/dead-stores.cpp
+++ clang/test/Analysis/dead-stores.cpp
@@ -1,11 +1,14 @@
 // 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;
 
@@ -17,6 +20,15 @@
 (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;
 }
 
 //===--===//
Index: clang/test/Analysis/dead-stores.c
===
--- clang/test/Analysis/dead-stores.c
+++ clang/test/Analysis/dead-stores.c
@@ -1,5 +1,6 @@
 // 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'}}
@@ -77,7 +78,11 @@
 // 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;
 }
@@ -152,8 +157,11 @@
 // 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;
 }
 
@@ -182,7 +190,11 @@
 
 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() {
@@ -535,7 +547,11 @@
 
 int rdar11185138_bar() {
   int