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
<http://cc.elte.hu:15200/Default/#run=before%20deadstore%20option&newcheck=after%20deadstore%20option&review-status=Unreviewed&review-status=Confirmed&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=before%20deadstore%20option_diff_after%20deadstore%20option>.
Note that this link will no longer work after the patch has been accepted.
Related previous patches:
rGf224820b45c6847b91071da8d7ade59f373b96f3
<https://reviews.llvm.org/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 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'}}
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'}}
Index: clang/test/Analysis/analyzer-config.c
===================================================================
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -30,6 +30,7 @@
// CHECK-NEXT: ctu-dir = ""
// CHECK-NEXT: ctu-import-threshold = 100
// CHECK-NEXT: ctu-index-name = externalDefMap.txt
+// CHECK-NEXT: deadcode.DeadStores:WarnForDeadNestedAssignments = false
// CHECK-NEXT: debug.AnalysisOrder:* = false
// CHECK-NEXT: debug.AnalysisOrder:Bind = false
// CHECK-NEXT: debug.AnalysisOrder:EndFunction = false
@@ -92,4 +93,4 @@
// CHECK-NEXT: unroll-loops = false
// CHECK-NEXT: widen-loops = false
// CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 89
+// CHECK-NEXT: num-entries = 90
Index: clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
@@ -130,6 +130,7 @@
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 };
@@ -137,9 +138,11 @@
DeadStoreObs(const CFG &cfg, ASTContext &ctx, BugReporter &br,
const CheckerBase *checker, AnalysisDeclContext *ac,
ParentMap &parents,
- llvm::SmallPtrSet<const VarDecl *, 20> &escaped)
+ llvm::SmallPtrSet<const VarDecl *, 20> &escaped,
+ bool warnForDeadNestedAssignments)
: cfg(cfg), Ctx(ctx), BR(br), Checker(checker), AC(ac), Parents(parents),
- Escaped(escaped), currentBlock(nullptr) {}
+ Escaped(escaped), currentBlock(nullptr),
+ WarnForDeadNestedAssignments(warnForDeadNestedAssignments) {}
~DeadStoreObs() override {}
@@ -217,11 +220,15 @@
os << "Value stored to '" << *V << "' is never read";
break;
- 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)
+ return;
+ BugType = "Dead nested assignment";
+ os << "Although the value stored to '" << *V
+ << "' is used in the enclosing expression, the value is never actually"
+ " read from '"
+ << *V << "'";
+ break;
}
BR.EmitBasicReport(AC->getDecl(), Checker, BugType, "Dead store", os.str(),
@@ -474,6 +481,8 @@
namespace {
class DeadStoresChecker : public Checker<check::ASTCodeBody> {
public:
+ bool WarnForDeadNestedAssignments = false;
+
void checkASTCodeBody(const Decl *D, AnalysisManager& mgr,
BugReporter &BR) const {
@@ -491,15 +500,19 @@
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) {
Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
===================================================================
--- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -636,6 +636,15 @@
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."
+ "It will likely report false positives.",
+ "false",
+ InAlpha>
+ ]>,
Documentation<HasDocumentation>;
} // end DeadCode
Index: clang/docs/analyzer/checkers.rst
===================================================================
--- clang/docs/analyzer/checkers.rst
+++ clang/docs/analyzer/checkers.rst
@@ -294,6 +294,18 @@
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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
