[clang] 16cb3be - [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments

[Balazs Benics via cfe-commits]

Author: Fred Tingaud
Date: 2022-08-23T18:33:26+02:00
New Revision: 16cb3be62600621361644ebd15d071c711d6aa86

URL: https://github.com/llvm/llvm-project/commit/16cb3be62600621361644ebd15d071c711d6aa86
DIFF: https://github.com/llvm/llvm-project/commit/16cb3be62600621361644ebd15d071c711d6aa86.diff

LOG: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments

Dead store detection automatically checks that an expression is a
CXXConstructor and skips it because of potential side effects. In C++17,
with guaranteed copy elision, this check can fail because we actually
receive the implicit cast of a CXXConstructor.
Most checks in the dead store analysis were already stripping all casts
and parenthesis and those that weren't were either forgotten (like the
constructor) or would not suffer from it, so this patch proposes to
factorize the stripping.
It has an impact on where the dead store warning is reported in the case
of an explicit cast, from

  auto a = static_cast<B>(A());
           ^~~~~~~~~~~~~~~~~~~

to

  auto a = static_cast<B>(A());
                          ^~~

which we think is an improvement.

Patch By: frederic-tingaud-sonarsource

Reviewed By: steakhal

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

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
index 3d9aca91da7d4..e8eb2cb778f36 100644
--- a/clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
@@ -103,8 +103,8 @@ void ReachableCode::computeReachableBlocks() {
 static const Expr *
 LookThroughTransitiveAssignmentsAndCommaOperators(const Expr *Ex) {
   while (Ex) {
-    const BinaryOperator *BO =
-      dyn_cast<BinaryOperator>(Ex->IgnoreParenCasts());
+    Ex = Ex->IgnoreParenCasts();
+    const BinaryOperator *BO = dyn_cast<BinaryOperator>(Ex);
     if (!BO)
       break;
     BinaryOperatorKind Op = BO->getOpcode();
@@ -331,8 +331,7 @@ class DeadStoreObs : public LiveVariables::Observer {
           // Special case: check for assigning null to a pointer.
           //  This is a common form of defensive programming.
           const Expr *RHS =
-            LookThroughTransitiveAssignmentsAndCommaOperators(B->getRHS());
-          RHS = RHS->IgnoreParenCasts();
+              LookThroughTransitiveAssignmentsAndCommaOperators(B->getRHS());
 
           QualType T = VD->getType();
           if (T.isVolatileQualified())
@@ -415,8 +414,7 @@ class DeadStoreObs : public LiveVariables::Observer {
               if (isConstant(E))
                 return;
 
-              if (const DeclRefExpr *DRE =
-                      dyn_cast<DeclRefExpr>(E->IgnoreParenCasts()))
+              if (const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E))
                 if (const VarDecl *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
                   // Special case: check for initialization from constant
                   //  variables.
@@ -438,7 +436,7 @@ class DeadStoreObs : public LiveVariables::Observer {
 
               PathDiagnosticLocation Loc =
                 PathDiagnosticLocation::create(V, BR.getSourceManager());
-              Report(V, DeadInit, Loc, E->getSourceRange());
+              Report(V, DeadInit, Loc, V->getInit()->getSourceRange());
             }
           }
         }
@@ -450,8 +448,9 @@ class DeadStoreObs : public LiveVariables::Observer {
   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 llvm::all_of(Candidate->inits(), [this](const Expr *Init) {
+      return isConstant(Init->IgnoreParenCasts());
+    });
   }
 
   /// Return true if the given expression can be interpreted as constant
@@ -461,7 +460,7 @@ class DeadStoreObs : public LiveVariables::Observer {
       return true;
 
     // We should also allow defensive initialization of structs, i.e. { 0 }
-    if (const auto *ILE = dyn_cast<InitListExpr>(E->IgnoreParenCasts())) {
+    if (const auto *ILE = dyn_cast<InitListExpr>(E)) {
       return isConstant(ILE);
     }
 

diff  --git a/clang/test/Analysis/dead-stores.cpp b/clang/test/Analysis/dead-stores.cpp
index a5b7a1e5e0851..652d70b8ce916 100644
--- a/clang/test/Analysis/dead-stores.cpp
+++ b/clang/test/Analysis/dead-stores.cpp
@@ -11,6 +11,10 @@
 // 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
+//
+// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++17    \
+// RUN:  -analyzer-checker=deadcode.DeadStores -Wno-unreachable-code            \
+// RUN:  -verify=non-nested,nested %s
 
 //===----------------------------------------------------------------------===//
 // Basic dead store checking (but in C++ mode).
@@ -33,6 +37,8 @@ void test1() {
   (void)y;
   if ((y = make_int())) // nested-warning {{Although the value stored}}
     return;
+
+  auto z = "text"; // non-nested-warning {{never read}}
 }
 
 //===----------------------------------------------------------------------===//
@@ -52,6 +58,18 @@ int test2(int x) {
   return x;
 }
 
+class TestConstructor {
+public:
+  TestConstructor(int &y);
+};
+void copy(int x) {
+  // All these calls might have side effects in the opaque constructor
+  TestConstructor tc1 = x;                    // no-warning potential side effects
+  TestConstructor tc2 = TestConstructor(x);   // no-warning potential side effects
+  TestConstructor tc3 = (TestConstructor(x)); // no-warning potential side effects
+  TestConstructor tc4 = (TestConstructor)(x); // no-warning potential side effects
+}
+
 //===----------------------------------------------------------------------===//
 // Dead store checking involving CXXTemporaryExprs
 //===----------------------------------------------------------------------===//