Adjusted the semantics of assign checking in IdempotentOperationChecker

- Fixed a regression where assigning '0' would be reported
- Changed the way self assignments are filtered to allow constant testing
- Added a test case for assign ops
- Fixed one test case where a function pointer was not considered constant
- Fixed test cases relating to 0 assignment

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

diff --git a/lib/Checker/IdempotentOperationChecker.cpp b/lib/Checker/IdempotentOperationChecker.cpp
index 8a9e333ce5c5b5aa1298991698e9163496c147b2..3d4be1a11937b57c187268ced04537d86f8af3df 100644 (file)
--- a/lib/Checker/IdempotentOperationChecker.cpp
+++ b/lib/Checker/IdempotentOperationChecker.cpp
@@ -74,15 +74,15 @@ class IdempotentOperationChecker
     void UpdateAssumption(Assumption &A, const Assumption &New);
 
     // False positive reduction methods
-    static bool isUnusedSelfAssign(const Expr *LHS,
-                                      const Expr *RHS,
-                                      AnalysisContext *AC);
+    static bool isSelfAssign(const Expr *LHS, const Expr *RHS);
+    static bool isUnused(const Expr *E, AnalysisContext *AC);
     static bool isTruncationExtensionAssignment(const Expr *LHS,
                                                 const Expr *RHS);
     static bool PathWasCompletelyAnalyzed(const CFG *C,
                                           const CFGBlock *CB,
                                           const GRCoreEngine &CE);
-    static bool CanVary(const Expr *Ex, AnalysisContext *AC);
+    static bool CanVary(const Expr *Ex,
+                        AnalysisContext *AC);
     static bool isConstantOrPseudoConstant(const DeclRefExpr *DR,
                                            AnalysisContext *AC);
     static bool containsNonLocalVarDecl(const Stmt *S);
@@ -184,19 +184,19 @@ void IdempotentOperationChecker::PreVisitBinaryOperator(
 
   // Fall through intentional
   case BO_Assign:
-    // 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;
+    // x Assign x can be used to silence unused variable warnings intentionally.
+    // If this is a self assignment and the variable is referenced elsewhere,
+    // then it is a false positive.
+    if (isSelfAssign(LHS, RHS)) {
+      if (!isUnused(LHS, AC)) {
+        UpdateAssumption(A, Equal);
+        return;
+      }
+      else {
+        A = Impossible;
+        return;
+      }
     }
-    if (LHSVal != RHSVal)
-      break;
-    UpdateAssumption(A, Equal);
-    return;
 
   case BO_SubAssign:
   case BO_DivAssign:
@@ -412,12 +412,9 @@ inline void IdempotentOperationChecker::UpdateAssumption(Assumption &A,
   }
 }
 
-// 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) {
+// Check for a statement where a variable is self assigned to possibly avoid an
+// unused variable warning.
+bool IdempotentOperationChecker::isSelfAssign(const Expr *LHS, const Expr *RHS) {
   LHS = LHS->IgnoreParenCasts();
   RHS = RHS->IgnoreParenCasts();
 
@@ -436,7 +433,24 @@ bool IdempotentOperationChecker::isUnusedSelfAssign(const Expr *LHS,
   if (VD != RHS_DR->getDecl())
     return false;
 
-  // If the var was used outside of a selfassign, then we should still report
+  return true;
+}
+
+// Returns true if the Expr points to a VarDecl that is not read anywhere
+// outside of self-assignments.
+bool IdempotentOperationChecker::isUnused(const Expr *E,
+                                          AnalysisContext *AC) {
+  if (!E)
+    return false;
+
+  const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(E->IgnoreParenCasts());
+  if (!DR)
+    return false;
+
+  const VarDecl *VD = dyn_cast<VarDecl>(DR->getDecl());
+  if (!VD)
+    return false;
+
   if (AC->getPseudoConstantAnalysis()->wasReferenced(VD))
     return false;
 
@@ -571,6 +585,7 @@ bool IdempotentOperationChecker::CanVary(const Expr *Ex,
     return SE->getTypeOfArgument()->isVariableArrayType();
   }
   case Stmt::DeclRefExprClass:
+    // Check for constants/pseudoconstants
     return !isConstantOrPseudoConstant(cast<DeclRefExpr>(Ex), AC);
 
   // The next cases require recursion for subexpressions
@@ -593,14 +608,14 @@ bool IdempotentOperationChecker::CanVary(const Expr *Ex,
     return CanVary(cast<const ChooseExpr>(Ex)->getChosenSubExpr(
         AC->getASTContext()), AC);
   case Stmt::ConditionalOperatorClass:
-      return CanVary(cast<const ConditionalOperator>(Ex)->getCond(), AC);
+    return CanVary(cast<const ConditionalOperator>(Ex)->getCond(), AC);
   }
 }
 
 // Returns true if a DeclRefExpr is or behaves like a constant.
 bool IdempotentOperationChecker::isConstantOrPseudoConstant(
-                                                         const DeclRefExpr *DR,
-                                                         AnalysisContext *AC) {
+                                                          const DeclRefExpr *DR,
+                                                          AnalysisContext *AC) {
   // Check if the type of the Decl is const-qualified
   if (DR->getType().isConstQualified())
     return true;

diff --git a/test/Analysis/func.c b/test/Analysis/func.c
index a7e84615fd392a6468e590eacbfafbefae78643e..53c873df55a95a7bf92ac24da6e9b79ba6c8e01e 100644 (file)
--- a/test/Analysis/func.c
+++ b/test/Analysis/func.c
@@ -4,7 +4,7 @@
 void f(void) {
   void (*p)(void);
   p = f;
-  p = &f; // expected-warning{{Assigned value is always the same as the existing value}}
+  p = &f;
   p();
   (*p)();
 }

diff --git a/test/Analysis/idempotent-operations.c b/test/Analysis/idempotent-operations.c
index debff0efff41a8df4364f05858d1ccdd62b95927..5c9a59d736166fd8f2fc2db42f7eb383e47898d3 100644 (file)
--- a/test/Analysis/idempotent-operations.c
+++ b/test/Analysis/idempotent-operations.c
@@ -172,3 +172,18 @@ int false6() {
 
   return localInt;
 }
+
+// Check that assignments filter out false positives correctly
+int false7() {
+  int zero = 0; // psuedo-constant
+  int one = 1;
+
+  int a = 55;
+  a = a; // expected-warning{{Assigned value is always the same as the existing value}}
+  a = enum1 * a; // no-warning
+
+  int b = 123;
+  b = b; // no-warning
+
+  return a;
+}

diff --git a/test/Analysis/misc-ps-region-store.m b/test/Analysis/misc-ps-region-store.m
index d46437388d32270448c5742b3eaf24901737f568..38275335ee95bbc5e4a725a6f913e66ba0ad0be2 100644 (file)
--- a/test/Analysis/misc-ps-region-store.m
+++ b/test/Analysis/misc-ps-region-store.m
@@ -708,7 +708,7 @@ int pr5857(char *src) {
   long long *z = (long long *) (intptr_t) src;
   long long w = 0;
   int n = 0;
-  for (n = 0; n < y; ++n) { // expected-warning{{Assigned value is always the same as the existing value}}
+  for (n = 0; n < y; ++n) {
     // Previously we crashed analyzing this statement.
     w = *z++;
   }

diff --git a/test/Analysis/misc-ps.m b/test/Analysis/misc-ps.m
index 6aac74b1091b141e619d8030433023540b962f84..42eccfeec43c508a54db7efb8c187b644ef6e9d3 100644 (file)
--- a/test/Analysis/misc-ps.m
+++ b/test/Analysis/misc-ps.m
@@ -458,7 +458,7 @@ void rdar_7062158_2() {
 // ElementRegion is created.
 unsigned char test_array_index_bitwidth(const unsigned char *p) {
   unsigned short i = 0;
-  for (i = 0; i < 2; i++) p = &p[i]; // expected-warning{{Assigned value is always the same as the existing value}}
+  for (i = 0; i < 2; i++) p = &p[i];
   return p[i+1];
 }