Improvements to IdempotentOperationChecker and its use of PseudoConstantAnalysis

- Added wasReferenced function to PseudoConstantAnalysis to determine if a variable was ever referenced in a function (outside of a self-assignment)
- BlockDeclRefExpr referenced variables are now explicitly added to the non-constant list
- Remove unnecessary ignore of implicit casts
- Generalized parameter self-assign detection to detect deliberate self-assigns of variables to avoid unused variable warnings
- Updated test cases with deliberate self-assignments
- Fixed bug with C++ references and pseudoconstants
- Added test case for C++ references and pseudoconstants

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@111965 91177308-0d34-0410-b5e6-96231b3b80d8

8 files changed, 109 insertions, 26 deletions

diff --git a/include/clang/Analysis/Analyses/PseudoConstantAnalysis.h b/include/clang/Analysis/Analyses/PseudoConstantAnalysis.h
index 0dd534c2a8..ea26fd4a41 100644
--- a/include/clang/Analysis/Analyses/PseudoConstantAnalysis.h
+++ b/include/clang/Analysis/Analyses/PseudoConstantAnalysis.h
@@ -26,12 +26,14 @@ public:
   ~PseudoConstantAnalysis();
 
   bool isPseudoConstant(const VarDecl *VD);
+  bool wasReferenced(const VarDecl *VD);
 
 private:
   void RunAnalysis();
 
   // for storing the result of analyzed ValueDecls
   void *NonConstantsImpl;
+  void *UsedVarsImpl;
 
   const Stmt *DeclBody;
   bool Analyzed;
diff --git a/lib/Analysis/PseudoConstantAnalysis.cpp b/lib/Analysis/PseudoConstantAnalysis.cpp
index 7a2fa8a8bd..5deabacea4 100644
--- a/lib/Analysis/PseudoConstantAnalysis.cpp
+++ b/lib/Analysis/PseudoConstantAnalysis.cpp
@@ -28,10 +28,12 @@ typedef llvm::SmallPtrSet<const VarDecl*, VARDECL_SET_SIZE> VarDeclSet;
 PseudoConstantAnalysis::PseudoConstantAnalysis(const Stmt *DeclBody) :
       DeclBody(DeclBody), Analyzed(false) {
   NonConstantsImpl = new VarDeclSet;
+  UsedVarsImpl = new VarDeclSet;
 }
 
 PseudoConstantAnalysis::~PseudoConstantAnalysis() {
   delete (VarDeclSet*)NonConstantsImpl;
+  delete (VarDeclSet*)UsedVarsImpl;
 }
 
 // Returns true if the given ValueDecl is never written to in the given DeclBody
@@ -50,9 +52,22 @@ bool PseudoConstantAnalysis::isPseudoConstant(const VarDecl *VD) {
   return !NonConstants->count(VD);
 }
 
+// Returns true if the variable was used (self assignments don't count)
+bool PseudoConstantAnalysis::wasReferenced(const VarDecl *VD) {
+  if (!Analyzed) {
+    RunAnalysis();
+    Analyzed = true;
+  }
+
+  VarDeclSet *UsedVars = (VarDeclSet*)UsedVarsImpl;
+
+  return UsedVars->count(VD);
+}
+
 void PseudoConstantAnalysis::RunAnalysis() {
   std::deque<const Stmt *> WorkList;
   VarDeclSet *NonConstants = (VarDeclSet*)NonConstantsImpl;
+  VarDeclSet *UsedVars = (VarDeclSet*)UsedVarsImpl;
 
   // Start with the top level statement of the function
   WorkList.push_back(DeclBody);
@@ -65,7 +80,7 @@ void PseudoConstantAnalysis::RunAnalysis() {
     // Case 1: Assignment operators modifying ValueDecl
     case Stmt::BinaryOperatorClass: {
       const BinaryOperator *BO = cast<BinaryOperator>(Head);
-      const Expr *LHS = BO->getLHS()->IgnoreParenImpCasts();
+      const Expr *LHS = BO->getLHS()->IgnoreParenCasts();
       const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(LHS);
 
       // We only care about DeclRefExprs on the LHS
@@ -76,7 +91,16 @@ void PseudoConstantAnalysis::RunAnalysis() {
       // for any of the assignment operators, implying that this Decl is being
       // written to.
       switch (BO->getOpcode()) {
-      case BinaryOperator::Assign:
+      case BinaryOperator::Assign: {
+        const Expr *RHS = BO->getRHS()->IgnoreParenCasts();
+        if (const DeclRefExpr *RHSDecl = dyn_cast<DeclRefExpr>(RHS)) {
+          // Self-assignments don't count as use of a variable
+          if (DR->getDecl() == RHSDecl->getDecl())
+            // Do not visit the children
+            continue;
+        }
+
+      }
       case BinaryOperator::AddAssign:
       case BinaryOperator::SubAssign:
       case BinaryOperator::MulAssign:
@@ -148,16 +172,37 @@ void PseudoConstantAnalysis::RunAnalysis() {
         if (!VD->getType().getTypePtr()->isReferenceType())
           continue;
 
-        // Ignore VarDecls without a body
-        if (!VD->getBody())
-          continue;
-
         // If the reference is to another var, add the var to the non-constant
         // list
-        if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(VD->getBody()))
-          if (const VarDecl *RefVD = dyn_cast<VarDecl>(DR->getDecl()))
+        if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(VD->getInit()))
+          if (const VarDecl *RefVD = dyn_cast<VarDecl>(DR->getDecl())) {
             NonConstants->insert(RefVD);
+            continue;
+          }
+      }
+      break;
+    }
+
+    // Case 4: Block variable references
+    case Stmt::BlockDeclRefExprClass: {
+      // Any block variables are assumed to be non-constant
+      const BlockDeclRefExpr *BDR = cast<BlockDeclRefExpr>(Head);
+      if (const VarDecl *VD = dyn_cast<VarDecl>(BDR->getDecl())) {
+        NonConstants->insert(VD);
+        UsedVars->insert(VD);
+        continue;
       }
+      break;
+    }
+
+    // Case 5: Variable references
+    case Stmt::DeclRefExprClass: {
+      const DeclRefExpr *DR = cast<DeclRefExpr>(Head);
+      if (const VarDecl *VD = dyn_cast<VarDecl>(DR->getDecl())) {
+        UsedVars->insert(VD);
+        continue;
+      }
+      break;
     }
 
       default:
diff --git a/lib/Checker/IdempotentOperationChecker.cpp b/lib/Checker/IdempotentOperationChecker.cpp
index 2cfcaa44d4..d8936203ea 100644
--- a/lib/Checker/IdempotentOperationChecker.cpp
+++ b/lib/Checker/IdempotentOperationChecker.cpp
@@ -74,7 +74,9 @@ class IdempotentOperationChecker
     void UpdateAssumption(Assumption &A, const Assumption &New);
 
     // False positive reduction methods
-    static bool isParameterSelfAssign(const Expr *LHS, const Expr *RHS);
+    static bool isUnusedSelfAssign(const Expr *LHS,
+                                      const Expr *RHS,
+                                      AnalysisContext *AC);
     static bool isTruncationExtensionAssignment(const Expr *LHS,
                                                 const Expr *RHS);
     static bool PathWasCompletelyAnalyzed(const CFG *C,
@@ -182,12 +184,19 @@ void IdempotentOperationChecker::PreVisitBinaryOperator(
 
   // Fall through intentional
   case BinaryOperator::Assign:
-    // x Assign x has a few more false positives we can check for
-    if (isParameterSelfAssign(RHS, LHS)
-        || isTruncationExtensionAssignment(RHS, LHS)) {
+    // x Assign x can be used to silence unused variable warnings intentionally,
+    // and has a slightly different definition for false positives.
+    if (isUnusedSelfAssign(RHS, LHS, AC)
+        || isTruncationExtensionAssignment(RHS, LHS)
+        || containsNonLocalVarDecl(RHS)
+        || containsNonLocalVarDecl(LHS)) {
       A = Impossible;
       return;
     }
+    if (LHSVal != RHSVal)
+      break;
+    UpdateAssumption(A, Equal);
+    return;
 
   case BinaryOperator::SubAssign:
   case BinaryOperator::DivAssign:
@@ -397,10 +406,12 @@ inline void IdempotentOperationChecker::UpdateAssumption(Assumption &A,
   }
 }
 
-// Check for a statement were a parameter is self assigned (to avoid an unused
-// variable warning)
-bool IdempotentOperationChecker::isParameterSelfAssign(const Expr *LHS,
-                                                       const Expr *RHS) {
+// Check for a statement where a variable is self assigned to avoid an unused
+// variable warning. We still report if the variable is used after the self
+// assignment.
+bool IdempotentOperationChecker::isUnusedSelfAssign(const Expr *LHS,
+                                                    const Expr *RHS,
+                                                    AnalysisContext *AC) {
   LHS = LHS->IgnoreParenCasts();
   RHS = RHS->IgnoreParenCasts();
 
@@ -408,15 +419,22 @@ bool IdempotentOperationChecker::isParameterSelfAssign(const Expr *LHS,
   if (!LHS_DR)
     return false;
 
-  const ParmVarDecl *PD = dyn_cast<ParmVarDecl>(LHS_DR->getDecl());
-  if (!PD)
+  const VarDecl *VD = dyn_cast<VarDecl>(LHS_DR->getDecl());
+  if (!VD)
     return false;
 
   const DeclRefExpr *RHS_DR = dyn_cast<DeclRefExpr>(RHS);
   if (!RHS_DR)
     return false;
 
-  return PD == RHS_DR->getDecl();
+  if (VD != RHS_DR->getDecl())
+    return false;
+
+  // If the var was used outside of a selfassign, then we should still report
+  if (AC->getPseudoConstantAnalysis()->wasReferenced(VD))
+    return false;
+
+  return true;
 }
 
 // Check for self casts truncating/extending a variable
diff --git a/test/Analysis/dead-stores.c b/test/Analysis/dead-stores.c
index 46b4038cce..57d5d112d7 100644
--- a/test/Analysis/dead-stores.c
+++ b/test/Analysis/dead-stores.c
@@ -158,7 +158,7 @@ int f16(int x) {
 // Self-assignments should not be flagged as dead stores.
 void f17() {
   int x = 1;
-  x = x; // expected-warning{{Assigned value is always the same as the existing value}}
+  x = x;
 }
 
 // <rdar://problem/6506065>
diff --git a/test/Analysis/idempotent-operations.c b/test/Analysis/idempotent-operations.c
index 3724b93cc7..a730d03127 100644
--- a/test/Analysis/idempotent-operations.c
+++ b/test/Analysis/idempotent-operations.c
@@ -91,15 +91,18 @@ unsigned false2() {
   return enum1 + a; // no-warning
 }
 
-// Self assignments of parameters are common false positives
-unsigned false3(int param) {
+// Self assignments of unused variables are common false positives
+unsigned false3(int param, int param2) {
   param = param; // no-warning
 
+  // if a self assigned variable is used later, then it should be reported still
+  param2 = param2; // expected-warning{{Assigned value is always the same as the existing value}}
+
   unsigned nonparam = 5;
 
   nonparam = nonparam; // expected-warning{{Assigned value is always the same as the existing value}}
 
-  return nonparam;
+  return param2 + nonparam;
 }
 
 // Pseudo-constants (vars only read) and constants should not be reported
diff --git a/test/Analysis/idempotent-operations.cpp b/test/Analysis/idempotent-operations.cpp
new file mode 100644
index 0000000000..c5d1ceb8af
--- /dev/null
+++ b/test/Analysis/idempotent-operations.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -analyze -analyzer-store=region -analyzer-constraints=range -fblocks -analyzer-opt-analyze-nested-blocks -analyzer-check-objc-mem -analyzer-check-idempotent-operations -verify %s
+
+// C++ specific false positives
+
+extern void test(int i);
+extern void test_ref(int &i);
+
+// Test references affecting pseudoconstants
+void false1() {
+  int a = 0;
+  int five = 5;
+  int &b = a;
+   test(five * a); // expected-warning {{The right operand to '*' is always 0}}
+   b = 4;
+}
diff --git a/test/Analysis/rdar-6541136-region.c b/test/Analysis/rdar-6541136-region.c
index 80c04ae8ca..82232c6bb9 100644
--- a/test/Analysis/rdar-6541136-region.c
+++ b/test/Analysis/rdar-6541136-region.c
@@ -9,7 +9,7 @@ extern kernel_tea_cheese_t _wonky_gesticulate_cheese;
 void test1( void ) {
   kernel_tea_cheese_t *wonky = &_wonky_gesticulate_cheese;
   struct load_wine *cmd = (void*) &wonky[1];
-  cmd = cmd; // expected-warning{{Assigned value is always the same as the existing value}}
+  cmd = cmd;
   char *p = (void*) &wonky[1];
   kernel_tea_cheese_t *q = &wonky[1];
   // This test case tests both the RegionStore logic (doesn't crash) and
@@ -21,7 +21,7 @@ void test1( void ) {
 void test1_b( void ) {
   kernel_tea_cheese_t *wonky = &_wonky_gesticulate_cheese;
   struct load_wine *cmd = (void*) &wonky[1];
-  cmd = cmd; // expected-warning{{Assigned value is always the same as the existing value}}
+  cmd = cmd;
   char *p = (void*) &wonky[1];
   *p = 1;  // expected-warning{{Access out-of-bound array element (buffer overflow)}}
 }
diff --git a/test/Analysis/rdar-6541136.c b/test/Analysis/rdar-6541136.c
index 911fb4aa2f..844a9367b4 100644
--- a/test/Analysis/rdar-6541136.c
+++ b/test/Analysis/rdar-6541136.c
@@ -12,7 +12,7 @@ void foo( void )
 {
   kernel_tea_cheese_t *wonky = &_wonky_gesticulate_cheese;
   struct load_wine *cmd = (void*) &wonky[1];
-  cmd = cmd; // expected-warning{{Assigned value is always the same as the existing value}}
+  cmd = cmd;
   char *p = (void*) &wonky[1];
   *p = 1;
   kernel_tea_cheese_t *q = &wonky[1];