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

[frederic-tingaud-sonarsource]

Summary

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.

[frederic-tingaud-sonarsource]

Fix formatting

[steakhal]

I haven't checked the implementation; the Deadstores checker was always a nemesis for me.

Off the top of my head, I think in the analyzer we have the elide-constructors=true/false analyzer config option, which quotes:

Whether elidable C++ copy-constructors and move-constructors should be actually elided during analysis. Both behaviors are allowed by the C++ standard, and the analyzer, like CodeGen, defaults to eliding. Starting with C++17 some elisions become mandatory, and in these cases the option will be ignored.

You also mentioned this copy elision, but I don't see anywhere checks for the standard version in code. I would expect that your change should apply only to C++17 and above.

clang/test/Analysis/dead-stores.cpp

65–70

So we don't warn for these since the opaque ctor call might introduce some side-effects.
Please note this after the no-warning tag.

[frederic-tingaud-sonarsource]

Document tests better

[frederic-tingaud-sonarsource]

Thanks for looking at the PR!

In D126534#3542402, @steakhal wrote:

You also mentioned this copy elision, but I don't see anywhere checks for the standard version in code. I would expect that your change should apply only to C++17 and above.

The false-positive due to copy elision in C++17 is the main pain point that this patch fixes, which is why I mention it in the title, but it does in fact also solve other cases for all C++ versions if there are parentheses or casts involved.

[NoQ]

Oh nice, I think I've heard about such problems, thanks a lot.

clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp

106–107

It looks like this introduces a change in behavior in unrelated situations, eg. the ones in objc-arc.m.plist where the diagnostic is moved to the right:

id obj1 = (__bridge_transfer id)CFCreateSomething(); // expected-warning{{never read}}
          ^before               ^after

I think this change is undesirable because it distracts from the assignment. In this case you can easily avoid this problem by using IgnoreParenImpCasts() instead, but it also generally makes sense to have patches that target specific situations to match on these situations more precisely. Eg., could you see if you can add a check that a constructor is involved, and maybe check for the exact cast kind that we want to unwrap?

[frederic-tingaud-sonarsource]

Thanks @NoQ!

The change in behavior is indeed what I was mentioning in the summary. After discussing it with a colleague, it seemed to us that pointing to a more precise range where the object is allocated was an improvement. I understand your point about pointing at something that is nearer to the assignment, but in that case, wouldn't it make sense to point at the whole assignment?

id obj1 = (__bridge_transfer id)CFCreateSomething(); // expected-warning{{never read}}
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

We already detect the presence of a constructor, but to skip the report altogether as constructors could have undetected side effects.

[frederic-tingaud-sonarsource]

It might also be interesting to think about cases that went through transitive assignment stripping when talking about the diagnostic position:
Before my patch, a dead store on variable a was displayed as follows:

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

(I'm switching back to C++ because I'm not sure exactly how legal such a thing would be in objective-C).
With the current fix, that doesn't change.
It might be clearer for the user to either put the diagnostic on the whole assignment or at least the whole initialization.

[frederic-tingaud-sonarsource]

Update diagnostic location to highlight the whole content of the initialization.

[frederic-tingaud-sonarsource]

Adding an additional false negative fixed by this patch.

[frederic-tingaud-sonarsource]

Hi @NoQ ,
Do the changes correspond to what you had in mind?

[steakhal]

Looks good to me, but I think you should wait for @NoQ for approval as he had some concerns previously.
@NoQ, do you think it's in a good shape for landing?

[frederic-tingaud-sonarsource]

Hi @NoQ . Would you have time to look if the changes I did to this PR solve your concerns?

[steakhal]

It feels like there is no objection. I think it looks great and you did everything you could to address the review comments.
I believe we should not postpone this any further.
Consider committing this tomorrow, unless in the meantime @NoQ objects.

[chrish_ericsson_atx]

This change affected analyzer findings for C language source as well. Mostly true positives, but there is one false positive we know about. I filed https://github.com/llvm/llvm-project/issues/57434 to track that new false positive.

clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp

for (CFGBlock::const_succ_iterator i = block->succ_begin(),
       if (const CFGBlock *succ = *i)
         worklist.push_back(succ);
   }
 }
 
 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();
     if (Op == BO_Assign || Op == BO_Comma) {
       Ex = BO->getRHS();
       continue;
     }
     break;
void observeStmt(const Stmt *S, const CFGBlock *block,
     if (const BinaryOperator* B = dyn_cast<BinaryOperator>(S)) {
       if (!B->isAssignmentOp()) return; // Skip non-assignments.
 
       if (DeclRefExpr *DR = dyn_cast<DeclRefExpr>(B->getLHS()))
         if (VarDecl *VD = dyn_cast<VarDecl>(DR->getDecl())) {
           // 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();
 
           QualType T = VD->getType();
           if (T.isVolatileQualified())
             return;
           if (T->isPointerType() || T->isObjCObjectPointerType()) {
             if (RHS->isNullPointerConstant(Ctx, Expr::NPC_ValueDependentIsNull))
               return;
           }
else if (const DeclStmt *DS = dyn_cast<DeclStmt>(S))
               //         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 (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.
                   //
                   //  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());
+              Report(V, DeadInit, Loc, V->getInit()->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 llvm::all_of(Candidate->inits(), [this](const Expr *Init) {
+      return isConstant(Init->IgnoreParenCasts());
+    });
   }
 
   /// 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())) {
+    if (const auto *ILE = dyn_cast<InitListExpr>(E)) {
       return isConstant(ILE);
     }
 
     return false;
   }
 };
 
 } // end anonymous namespace

clang/test/Analysis/dead-stores.cpp

 // 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-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
+//
+// 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).
 //===----------------------------------------------------------------------===//
 
 int j;
 int make_int();
 void test1() {
   int x = 4;
   x = x + 1; // non-nested-warning {{never read}}
 
   switch (j) {
   case 1:
     throw 1;
     (void)x;
     break;
   }
 
   int y;
   (void)y;
   if ((y = make_int())) // nested-warning {{Although the value stored}}
     return;
+
+  auto z = "text"; // non-nested-warning {{never read}}
 }
 
 //===----------------------------------------------------------------------===//
 // Dead store checking involving constructors.
 //===----------------------------------------------------------------------===//
 
 class Test2 {
   int &x;
 
 public:
   Test2(int &y) : x(y) {}
   ~Test2() { ++x; }
 };
 
 int test2(int x) {
   { Test2 a(x); } // no-warning
   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
 //===----------------------------------------------------------------------===//
 
 namespace TestTemp {
   template<typename _Tp>
   class pencil {
   public: