[clang] 9f0d8ba - [analyzer] Fix dead store checker false positive

[Valeriy Savchenko via cfe-commits]

Author: Valeriy Savchenko
Date: 2021-04-08T16:12:42+03:00
New Revision: 9f0d8bac144c8eb1ca4aff823b2e2d5a0f990072

URL: https://github.com/llvm/llvm-project/commit/9f0d8bac144c8eb1ca4aff823b2e2d5a0f990072
DIFF: https://github.com/llvm/llvm-project/commit/9f0d8bac144c8eb1ca4aff823b2e2d5a0f990072.diff

LOG: [analyzer] Fix dead store checker false positive

It is common to zero-initialize not only scalar variables,
but also structs.  This is also defensive programming and
we shouldn't complain about that.

rdar://34122265

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

Added: 
    

Modified: 
    clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
    clang/test/Analysis/dead-stores.c

Removed: 
    


################################################################################
diff  --git a/clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
index 8c86e83608b1e..8070d869f6785 100644
--- a/clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
@@ -11,17 +11,18 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "clang/Lex/Lexer.h"
-#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/ParentMap.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Analysis/Analyses/LiveVariables.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "llvm/ADT/BitVector.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/SaveAndRestore.h"
 
@@ -408,15 +409,17 @@ class DeadStoreObs : public LiveVariables::Observer {
               // Special case: check for initializations with constants.
               //
               //  e.g. : int x = 0;
+              //         struct A = {0, 1};
+              //         struct B = {{0}, {1, 2}};
               //
               // If x is EVER assigned a new value later, don't issue
               // a warning.  This is because such initialization can be
               // due to defensive programming.
-              if (E->isEvaluatable(Ctx))
+              if (isConstant(E))
                 return;
 
               if (const DeclRefExpr *DRE =
-                  dyn_cast<DeclRefExpr>(E->IgnoreParenCasts()))
+                      dyn_cast<DeclRefExpr>(E->IgnoreParenCasts()))
                 if (const VarDecl *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
                   // Special case: check for initialization from constant
                   //  variables.
@@ -444,6 +447,29 @@ class DeadStoreObs : public LiveVariables::Observer {
         }
       }
   }
+
+private:
+  /// Return true if the given init list can be interpreted as constant
+  bool isConstant(const InitListExpr *Candidate) const {
+    // We consider init list to be constant if each member of the list can be
+    // interpreted as constant.
+    return llvm::all_of(Candidate->inits(),
+                        [this](const Expr *Init) { return isConstant(Init); });
+  }
+
+  /// Return true if the given expression can be interpreted as constant
+  bool isConstant(const Expr *E) const {
+    // It looks like E itself is a constant
+    if (E->isEvaluatable(Ctx))
+      return true;
+
+    // We should also allow defensive initialization of structs, i.e. { 0 }
+    if (const auto *ILE = dyn_cast<InitListExpr>(E->IgnoreParenCasts())) {
+      return isConstant(ILE);
+    }
+
+    return false;
+  }
 };
 
 } // end anonymous namespace

diff  --git a/clang/test/Analysis/dead-stores.c b/clang/test/Analysis/dead-stores.c
index a17e1692496da..145b81bd03327 100644
--- a/clang/test/Analysis/dead-stores.c
+++ b/clang/test/Analysis/dead-stores.c
@@ -635,3 +635,44 @@ void testVolatile() {
   volatile int v;
   v = 0; // no warning
 }
+
+struct Foo {
+  int x;
+  int y;
+};
+
+struct Foo rdar34122265_getFoo(void);
+
+int rdar34122265_test(int input) {
+  // This is allowed for defensive programming.
+  struct Foo foo = {0, 0};
+  if (input > 0) {
+    foo = rdar34122265_getFoo();
+  } else {
+    return 0;
+  }
+  return foo.x + foo.y;
+}
+
+void rdar34122265_test_cast() {
+  // This is allowed for defensive programming.
+  struct Foo foo = {0, 0};
+  (void)foo;
+}
+
+struct Bar {
+  struct Foo x, y;
+};
+
+struct Bar rdar34122265_getBar(void);
+
+int rdar34122265_test_nested(int input) {
+  // This is allowed for defensive programming.
+  struct Bar bar = {{0, 0}, {0, 0}};
+  if (input > 0) {
+    bar = rdar34122265_getBar();
+  } else {
+    return 0;
+  }
+  return bar.x.x + bar.y.y;
+}