[analyzer] Fix dead store checker false positive

[vsavchenko]

Summary

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

[steakhal]

I see your point.

Would it report this issue?

int test() {
  struct Foo foo = {0, 0}; // I would expect a warning here, that 'foo' was initialized but never read.
  (void)foo;
 return 0;
}

Only nits besides this.

clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp

25

I don't see any new code that would depend on this header.

422–427

Eh, the indentation looks horrible.
It would be probably better to use braces here.

[vsavchenko]

In D99262#2648512, @steakhal wrote:

I see your point.

Would it report this issue?

int test() {
  struct Foo foo = {0, 0}; // I would expect a warning here, that 'foo' was initialized but never read.
  (void)foo;
 return 0;
}

Only nits besides this.

We got one big fat complaint about that, and I can see the point.

clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp

25

llvm::all_of?

422–427

Yeah, I agree. I'm not sure braces will help much. I will try to do smith about it

[steakhal]

In D99262#2648533, @vsavchenko wrote:

In D99262#2648512, @steakhal wrote:

I see your point.

Would it report this issue?

int test() {
  struct Foo foo = {0, 0}; // I would expect a warning here, that 'foo' was initialized but never read.
  (void)foo;
 return 0;
}

Only nits besides this.

We got one big fat complaint about that, and I can see the point.

We should at least document it as testcase.

clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp

25

Oh sure!

[vsavchenko]

Add comments

[vsavchenko]

Add another test

[martong]

clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp

422–423

What about nested InitListExpr's?

std::array<int, 3> a1{ {1, 2, 3} };

VarDecl 0x561b200333a0 </home/egbomrt/tmp/aaa.cc:2:1, col:34> col:20 a1 'std::array<int, 3>':'std::array<int, 3>' listinit
`-InitListExpr 0x561b20036d78 <col:22, col:34> 'std::array<int, 3>':'std::array<int, 3>'
  `-InitListExpr 0x561b20036dc0 <col:24, col:32> 'typename _AT_Type::_Type':'int [3]'
    |-IntegerLiteral 0x561b20033408 <col:25> 'int' 1
    |-IntegerLiteral 0x561b20033428 <col:28> 'int' 2
    `-IntegerLiteral 0x561b20033448 <col:31> 'int' 3

[vsavchenko]

clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp

422–423

I'm not sure that we'll report anything on that

[NoQ]

Looks great and I'm also curious about nested initializers.

clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp

422–423

We probably won't because it's a C++ object, even though it's an aggregate so we should probably warn(?) What about a plain C object like

int x[2][2] = { { 0, 0 }, { 0, 0 } };

?

[vsavchenko]

clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp

422–423

The same here, we still don't warn about stores like this, but I think I can add test with nested structs

[steakhal]

Looks good. Thank you.

clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp

422–423

Should we add a C++ test case as well?

[vsavchenko]

In D99262#2673825, @steakhal wrote:

Looks good. Thank you.

I guess it will do for PODs, but for regular C++ types we have an early exit until we learn about side effects from constructors.

[vsavchenko]

Support nested init lists

[martong]

In D99262#2673842, @vsavchenko wrote:

Support nested init lists

Thanks for addressing the nested lists! (And sorry for the late reply, I was on vacation)

[vsavchenko]

In D99262#2682803, @martong wrote:

In D99262#2673842, @vsavchenko wrote:

Support nested init lists

Thanks for addressing the nested lists! (And sorry for the late reply, I was on vacation)

Hey, thanks for taking your time and reviewing it!

clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp

 //==- DeadStoresChecker.cpp - Check for stores to dead variables -*- C++ -*-==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines a DeadStores, a flow-sensitive checker that looks for
 //  stores to variables that are no longer live.
 //
 //===----------------------------------------------------------------------===//
 
-#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"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
 
else if (const DeclStmt *DS = dyn_cast<DeclStmt>(S))
             // is initialized.  We don't flag warnings for those variables
             // marked 'unused' or 'objc_precise_lifetime'.
             if (!isLive(Live, V) &&
                 !V->hasAttr<UnusedAttr>() &&
                 !V->hasAttr<ObjCPreciseLifetimeAttr>()) {
               // 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()))
                 if (const VarDecl *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
                   // Special case: check for initialization from constant
                   //  variables.
                   //
                   //  e.g. extern const int MyConstant;
                   //       int x = MyConstant;
                   //
                   if (VD->hasGlobalStorage() &&
                       VD->getType().isConstQualified())
                     return;
                   // Special case: check for initialization from scalar
                   //  parameters.  This is often a form of defensive
                   //  programming.  Non-scalars are still an error since
                   //  because it more likely represents an actual algorithmic
                   //  bug.
                   if (isa<ParmVarDecl>(VD) && VD->getType()->isScalarType())
                     return;
                 }
 
               PathDiagnosticLocation Loc =
                 PathDiagnosticLocation::create(V, BR.getSourceManager());
               Report(V, DeadInit, Loc, E->getSourceRange());
             }
           }
         }
       }
   }
+
+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
 
 //===----------------------------------------------------------------------===//
 // Driver function to invoke the Dead-Stores checker on a CFG.
 //===----------------------------------------------------------------------===//
 

clang/test/Analysis/dead-stores.c

void testBOComma() {
   int *p;
   p = (getPtr(), (int *)0); // no warning
 }
 
 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;
+}