[analyzer] Treat values passed as parameter as escaped

[t-8ch]

Summary

This fixes false-positives when a function or constructor takes address of the
parameter.

The motivation behind this change is a custom class that works similar to
std::string_view that takes the length by reference from an existing variable.
clang-tidy reported stores to the original length variable as dead although
they were actually observed by the string view.

[martong]

Thanks, looks good with some nits.

clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp

571–573

This could be hoisted into findCallReferenceParameters(const FunctionDecl *FD) thus avoiding code repetition.

[t-8ch]

Hoisted common code into common function.

[martong]

LGTM

[steakhal]

Please, consider stating the motivation behind this change.
For me, by looking at the modified test, it feels like we would lose legitimate findings (aka. true-positive reports) by applying this change.
From my perspective, the store to i is dead, since it will be immediately overwritten in the referenceParameters() function.

clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp

551–552

You don't seem to use the index, only for iterating over the parameters.
I believe, FD->parameters() would simplify this code.

553–560

By looking at e.g. findLambdaReferenceCaptures(), it seems like we should prefer continue statements for filtering; probably to avoid such deeply nested intendations.
Along with that, I think the llvm style guide prefers not wrapping single statement branches by courly braces.

564

I'm not sure getCalleeDecl() always returns a valid pointer.
I believe the dyn_cast_or_null would be more appropriate in this case.
This theory is underpinned by the other uses of the getCalleeDecl() API.

570

Why don't you use the CE->arguments() method instead? That would result in a range (begin + end).
I believe that is superior in most cases.

[steakhal]

clang/test/Analysis/dead-stores.cpp

243

By checking it on godbolt, this line never raised a warning.
https://godbolt.org/z/n7fP5od5q

You must be referring to L230 and L241 instead.

[t-8ch]

Implement stylistic change requested by reviewers and add better description.

[t-8ch]

@steakhal
Thanks for your review!
I think I have addressed all your points.

In D131067#3702044, @steakhal wrote:

Please, consider stating the motivation behind this change.

This is now added.

For me, by looking at the modified test, it feels like we would lose legitimate findings (aka. true-positive reports) by applying this change.

The change will indeed loose legitimate findings. We would have to also analyze the source code of the called function to see if it takes the address of the parameter.
But for callees which we don't have access to the AST this would still not work.

The existing escape-analysis works the same way, marking anything as escaped that *could* have escaped.
See the existing test basicLambda or the non-reference example:

void foo() {
  int x = 0;
  if (&x != &x) {
    return;
  }
  // This is clearly a dead store but as we have taken the address of x above it is treated as escaped.
  x = 3;
}

From my perspective, the store to i is dead, since it will be immediately overwritten in the referenceParameters() function.

[NoQ]

Thanks! I've seen a couple false positives like this and I'm shocked that this wasn't already how things worked.

I agree with @steakhal, we should add a test case that demonstrates an actual false positive suppressed by this patch. The tests that you've added are "FIXME" tests that demonstrate regressions caused by this patch - which is great and we should totally keep them! - but we should add a comment saying that in ideal world, if this checker's analysis was interprocedural, we would have noticed that this is still a dead store. But the purpose of tests is to avoid regressions, so it's important to add a test that demonstrates the improvement as well, so that this improvement wasn't lost. If we don't have improvements tested, somebody may remove your code and say "Hey, I haven't regressed anything and I even improved behavior on some tests!".

[t-8ch]

Rework tests

[steakhal]

In D131067#3705783, @t-8ch wrote:

In D131067#3702044, @steakhal wrote:

Please, consider stating the motivation behind this change.

This is now added.

I was about the empty revision summary.
Generally, we use that to highlight the motivation of some proposed change.
We also tend to keep it in sync which the commit message.
(I think arcanist auto updates the review version of the summary to match with the actual message once you commit your change.)

Sorry for blocking this change. It makes sense now.

clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp

552

I think as we are on C++17 now, we could use structured bindings. WDYT?

[t-8ch]

clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp

552

Sounds good, will update.
Note that "now" means since three days ago :-)

[t-8ch]

• Inline tiny functions
• Use structured binding
• Rename findReferenceParameter -> findReferenceParameters

[NoQ]

clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp

552

IIRC there are a few cases where parameters and arguments don't zip together nicely (cf. the hacks we needed to do in CXXMemberOperatorCall::getAdjustedParameterIndex(), CXXMemberOperatorCall::::getASTArgumentIndex()).

[t-8ch]

• [analyzer] Treat values passed as parameter as escaped
• [analyzer][deadstores] Handle member operator calls

Handle calls to member operators

[t-8ch]

clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp

552

Indeed. I added logic to handle it.
Unfortunately I didn't see how to use CXXMemberOperatorCall directly.
So I extracted the detection logic from CallEventManager::getSimpleCall() into isMemberOperatorCall().

Any hints on how to do this better are much appreciated.

clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp

 };
 
 } // end anonymous namespace
 
 //===----------------------------------------------------------------------===//
 // Driver function to invoke the Dead-Stores checker on a CFG.
 //===----------------------------------------------------------------------===//
 
+static bool isMemberOperatorCall(const CallExpr *CE) {
+  if (const auto *OpCE = dyn_cast<CXXOperatorCallExpr>(CE)) {
+    const FunctionDecl *DirectCallee = OpCE->getDirectCallee();
+    if (const auto *MD = dyn_cast<CXXMethodDecl>(DirectCallee))
+      return MD->isInstance();
+  }
+  return false;
+}
+
 namespace {
 class FindEscaped {
 public:
   llvm::SmallPtrSet<const VarDecl*, 20> Escaped;
 
   void operator()(const Stmt *S) {
     // Check for '&'. Any VarDecl whose address has been taken we treat as
     // escaped.
-    // FIXME: What about references?
     if (auto *LE = dyn_cast<LambdaExpr>(S)) {
       findLambdaReferenceCaptures(LE);
       return;
     }
 
+    if (auto *CE = dyn_cast<CallExpr>(S)) {
+      if (auto *FD = dyn_cast_or_null<FunctionDecl>(CE->getCalleeDecl())) {
+        CallExpr::const_arg_range args = CE->arguments();
+        if (isMemberOperatorCall(CE) && !args.empty()) {
+          // Implicit 'this' is part of args but not params, skip it.
+          args = {args.begin() + 1, args.end()};
+        }
+        findReferenceParameters(FD, args);
+      }
+      return;
+    }
+
+    if (auto *CE = dyn_cast<CXXConstructExpr>(S)) {
+      findReferenceParameters(CE->getConstructor(), CE->arguments());
+      return;
+    }
+
     const UnaryOperator *U = dyn_cast<UnaryOperator>(S);
     if (!U)
       return;
     if (U->getOpcode() != UO_AddrOf)
       return;
 
     const Expr *E = U->getSubExpr()->IgnoreParenCasts();
     if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(E))
for (const LambdaCapture &C : LE->captures()) {
       if (!FD || !isa<VarDecl>(VD))
         continue;
 
       // If the capture field is a reference type, it is capture-by-reference.
       if (FD->getType()->isReferenceType())
         Escaped.insert(cast<VarDecl>(VD));
     }
   }
+
+  void findReferenceParameters(const FunctionDecl *FD,
+                               CallExpr::const_arg_range Args) {
+    for (const auto &[Param, Arg] : llvm::zip(FD->parameters(), Args)) {
+      if (!Param->getType()->isReferenceType())
+        continue;
+
+      auto *DRE = dyn_cast<DeclRefExpr>(Arg);
+      if (!DRE)
+        continue;
+
+      if (auto *VD = dyn_cast<VarDecl>(DRE->getDecl()))
+        Escaped.insert(VD);
+    }
+  }
 };
 } // end anonymous namespace
 
 
 //===----------------------------------------------------------------------===//
 // DeadStoresChecker
 //===----------------------------------------------------------------------===//
 
 void DeadStoresChecker::checkASTCodeBody(const Decl *D, AnalysisManager &mgr,
                                          BugReporter &BR) const {
 
   // Don't do anything for template instantiations.
   // Proving that code in a template instantiation is "dead"
   // means proving that it is dead in all instantiations.
   // This same problem exists with -Wunreachable-code.
   if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D))
     if (FD->isTemplateInstantiation())
       return;

clang/test/Analysis/dead-stores.cpp

int basicLambda(int i, int j) {
   j = 6; // no warning
   [i] { (void)i; }();
   [&j] { (void)j; }();
   i = 2;
   j = 3;
   return i + j;
 }
 
+//===----------------------------------------------------------------------===//
+// Dead store checking involving reference parameters.
+//===----------------------------------------------------------------------===//
+
+struct ReferenceParameter {
+  int *ptr;
+
+  ReferenceParameter(int &i) : ptr(&i) {}
+  void function(int &i) {
+    ptr = &i;
+  }
+
+  void operator ()(int &i) {
+    ptr = &i;
+  }
+
+  int value() {
+    return *ptr;
+  }
+};
+
+void referenceParameters() {
+  int i = 7;
+  ReferenceParameter r(i);
+  i = 8;
+  if (r.value() == 8)
+    ;
+  i = 9; // FIXME this is a false-negative
+
+  int j = 10;
+  r.function(j);
+  j = 11;
+  if (r.value() == 11)
+    ;
+  j = 12; // FIXME this is a false-negative
+
+  int k = 13;
+  r(k);
+  k = 14;
+  if (r.value() == 14)
+    ;
+  k = 15; // FIXME this is a false-negative
+}