Improved false positive rate for the idempotent operations checker and moved it into...

- Added checks for static local variables, self assigned parameters, and truncating/extending self assignments
- Removed command line option (now default with --analyze)
- Updated test cases to pass with idempotent operation warnings

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

diff --git a/include/clang/Driver/CC1Options.td b/include/clang/Driver/CC1Options.td
index 7076f30995d09e1f22ae087337a774382fc9a692..4502401c16fdff79c2706db9681bb681a4ceb701 100644 (file)
--- a/include/clang/Driver/CC1Options.td
+++ b/include/clang/Driver/CC1Options.td
@@ -81,8 +81,6 @@ def analyzer_display_progress : Flag<"-analyzer-display-progress">,
   HelpText<"Emit verbose output about the analyzer's progress">;
 def analyzer_experimental_checks : Flag<"-analyzer-experimental-checks">,
   HelpText<"Use experimental path-sensitive checks">;
-def analyzer_idempotent_operation : Flag<"-analyzer-idempotent-operation">,
-  HelpText<"Use experimental path-sensitive idempotent operation checker">;
 def analyzer_experimental_internal_checks :
   Flag<"-analyzer-experimental-internal-checks">,
   HelpText<"Use new default path-sensitive checks currently in testing">;

diff --git a/lib/Checker/AnalysisConsumer.cpp b/lib/Checker/AnalysisConsumer.cpp
index 524f37e396622e829484e183ab1eda3d5a4c4ada..6c7a294b1d83feff5526c2ed81b54ec4535a6f97 100644 (file)
--- a/lib/Checker/AnalysisConsumer.cpp
+++ b/lib/Checker/AnalysisConsumer.cpp
@@ -341,9 +341,6 @@ static void ActionGRExprEngine(AnalysisConsumer &C, AnalysisManager& mgr,
   if (C.Opts.EnableExperimentalChecks)
     RegisterExperimentalChecks(Eng);
 
-  if (C.Opts.EnableIdempotentOperationChecker)
-    RegisterIdempotentOperationChecker(Eng);
-
   // Set the graph auditor.
   llvm::OwningPtr<ExplodedNode::Auditor> Auditor;
   if (mgr.shouldVisualizeUbigraph()) {

diff --git a/lib/Checker/GRExprEngine.cpp b/lib/Checker/GRExprEngine.cpp
index 07fee9d39e493840485fb10d10f97b9f090fb414..1424820f3b88916b45b7a70277ffd1a6e21049ec 100644 (file)
--- a/lib/Checker/GRExprEngine.cpp
+++ b/lib/Checker/GRExprEngine.cpp
@@ -361,6 +361,7 @@ static void RegisterInternalChecks(GRExprEngine &Eng) {
   RegisterDereferenceChecker(Eng);
   RegisterVLASizeChecker(Eng);
   RegisterDivZeroChecker(Eng);
+  RegisterIdempotentOperationChecker(Eng);
   RegisterReturnUndefChecker(Eng);
   RegisterUndefinedArraySubscriptChecker(Eng);
   RegisterUndefinedAssignmentChecker(Eng);

diff --git a/lib/Checker/GRExprEngineExperimentalChecks.h b/lib/Checker/GRExprEngineExperimentalChecks.h
index 7d1eb77f9fd6e17d183b4f4469baab2083e1adb1..3c1c95ffefd4ff65780100829d38c8bc9bed7bbc 100644 (file)
--- a/lib/Checker/GRExprEngineExperimentalChecks.h
+++ b/lib/Checker/GRExprEngineExperimentalChecks.h
@@ -23,7 +23,6 @@ void RegisterCStringChecker(GRExprEngine &Eng);
 void RegisterPthreadLockChecker(GRExprEngine &Eng);
 void RegisterMallocChecker(GRExprEngine &Eng);
 void RegisterStreamChecker(GRExprEngine &Eng);
-void RegisterIdempotentOperationChecker(GRExprEngine &Eng);
 
 } // end clang namespace
 #endif

diff --git a/lib/Checker/GRExprEngineInternalChecks.h b/lib/Checker/GRExprEngineInternalChecks.h
index f91a759b3299ecc01fea3616440599aee091e598..9644eafcb39f7215317363dcfc2c3c5b55377971 100644 (file)
--- a/lib/Checker/GRExprEngineInternalChecks.h
+++ b/lib/Checker/GRExprEngineInternalChecks.h
@@ -30,6 +30,7 @@ void RegisterCastSizeChecker(GRExprEngine &Eng);
 void RegisterDereferenceChecker(GRExprEngine &Eng);
 void RegisterDivZeroChecker(GRExprEngine &Eng);
 void RegisterFixedAddressChecker(GRExprEngine &Eng);
+void RegisterIdempotentOperationChecker(GRExprEngine &Eng);
 void RegisterNoReturnFunctionChecker(GRExprEngine &Eng);
 void RegisterPointerArithChecker(GRExprEngine &Eng);
 void RegisterPointerSubChecker(GRExprEngine &Eng);

diff --git a/lib/Checker/IdempotentOperationChecker.cpp b/lib/Checker/IdempotentOperationChecker.cpp
index 6ed18417a2c2b0ae0972f9313777f58e2f174afc..744fe2ba99290406c363cfd63f3ef5ad3d9bd46b 100644 (file)
--- a/lib/Checker/IdempotentOperationChecker.cpp
+++ b/lib/Checker/IdempotentOperationChecker.cpp
@@ -48,7 +48,7 @@
 // - Handle mixed assumptions (which assumptions can belong together?)
 // - Finer grained false positive control (levels)
 
-#include "GRExprEngineExperimentalChecks.h"
+#include "GRExprEngineInternalChecks.h"
 #include "clang/Checker/BugReporter/BugType.h"
 #include "clang/Checker/PathSensitive/CheckerVisitor.h"
 #include "clang/Checker/PathSensitive/SVals.h"
@@ -78,6 +78,10 @@ class IdempotentOperationChecker
     ///   element somewhere. Used in static analysis to reduce false positives.
     static bool containsMacro(const Stmt *S);
     static bool containsEnum(const Stmt *S);
+    static bool containsStaticLocal(const Stmt *S);
+    static bool isParameterSelfAssign(const Expr *LHS, const Expr *RHS);
+    static bool isTruncationExtensionAssignment(const Expr *LHS,
+                                                const Expr *RHS);
     static bool containsBuiltinOffsetOf(const Stmt *S);
     static bool containsZeroConstant(const Stmt *S);
     static bool containsOneConstant(const Stmt *S);
@@ -128,7 +132,7 @@ void IdempotentOperationChecker::PreVisitBinaryOperator(
   // Skip binary operators containing common false positives
   if (containsMacro(B) || containsEnum(B) || containsStmt<SizeOfAlignOfExpr>(B)
       || containsZeroConstant(B) || containsOneConstant(B)
-      || containsBuiltinOffsetOf(B)) {
+      || containsBuiltinOffsetOf(B) || containsStaticLocal(B)) {
     A = Impossible;
     return;
   }
@@ -181,12 +185,19 @@ void IdempotentOperationChecker::PreVisitBinaryOperator(
     break; // We don't care about any other operators.
 
   // 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)) {
+      A = Impossible;
+      return;
+    }
+
   case BinaryOperator::SubAssign:
   case BinaryOperator::DivAssign:
   case BinaryOperator::AndAssign:
   case BinaryOperator::OrAssign:
   case BinaryOperator::XorAssign:
-  case BinaryOperator::Assign:
   case BinaryOperator::Sub:
   case BinaryOperator::Div:
   case BinaryOperator::And:
@@ -399,6 +410,70 @@ bool IdempotentOperationChecker::containsEnum(const Stmt *S) {
   return false;
 }
 
+// Recursively find any substatements containing static vars
+bool IdempotentOperationChecker::containsStaticLocal(const Stmt *S) {
+  const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(S);
+
+  if (DR)
+    if (const VarDecl *VD = dyn_cast<VarDecl>(DR->getDecl()))
+      if (VD->isStaticLocal())
+        return true;
+
+  for (Stmt::const_child_iterator I = S->child_begin(); I != S->child_end();
+      ++I)
+    if (const Stmt *child = *I)
+      if (containsStaticLocal(child))
+        return true;
+
+  return false;
+}
+
+// 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) {
+  LHS = LHS->IgnoreParenCasts();
+  RHS = RHS->IgnoreParenCasts();
+
+  const DeclRefExpr *LHS_DR = dyn_cast<DeclRefExpr>(LHS);
+  if (!LHS_DR)
+    return false;
+
+  const ParmVarDecl *PD = dyn_cast<ParmVarDecl>(LHS_DR->getDecl());
+  if (!PD)
+    return false;
+
+  const DeclRefExpr *RHS_DR = dyn_cast<DeclRefExpr>(RHS);
+  if (!RHS_DR)
+    return false;
+
+  return PD == RHS_DR->getDecl();
+}
+
+// Check for self casts truncating/extending a variable
+bool IdempotentOperationChecker::isTruncationExtensionAssignment(
+                                                              const Expr *LHS,
+                                                              const Expr *RHS) {
+
+  const DeclRefExpr *LHS_DR = dyn_cast<DeclRefExpr>(LHS->IgnoreParenCasts());
+  if (!LHS_DR)
+    return false;
+
+  const VarDecl *VD = dyn_cast<VarDecl>(LHS_DR->getDecl());
+  if (!VD)
+    return false;
+
+  const DeclRefExpr *RHS_DR = dyn_cast<DeclRefExpr>(RHS->IgnoreParenCasts());
+  if (!RHS_DR)
+    return false;
+
+  if (VD != RHS_DR->getDecl())
+     return false;
+
+  return dyn_cast<DeclRefExpr>(RHS->IgnoreParens()) == NULL;
+}
+
+
 // Recursively find any substatements containing __builtin_offset_of
 bool IdempotentOperationChecker::containsBuiltinOffsetOf(const Stmt *S) {
   const UnaryOperator *UO = dyn_cast<UnaryOperator>(S);
@@ -415,6 +490,7 @@ bool IdempotentOperationChecker::containsBuiltinOffsetOf(const Stmt *S) {
   return false;
 }
 
+// Check for a integer or float constant of 0
 bool IdempotentOperationChecker::containsZeroConstant(const Stmt *S) {
   const IntegerLiteral *IL = dyn_cast<IntegerLiteral>(S);
   if (IL && IL->getValue() == 0)
@@ -433,6 +509,7 @@ bool IdempotentOperationChecker::containsZeroConstant(const Stmt *S) {
   return false;
 }
 
+// Check for an integer or float constant of 1
 bool IdempotentOperationChecker::containsOneConstant(const Stmt *S) {
   const IntegerLiteral *IL = dyn_cast<IntegerLiteral>(S);
   if (IL && IL->getValue() == 1)

diff --git a/lib/Frontend/CompilerInvocation.cpp b/lib/Frontend/CompilerInvocation.cpp
index 418d25b0d47d520ac428177881fc6a9d9e114995..00363d91fd43e6f92822d5ab7b2040fad9cd585b 100644 (file)
--- a/lib/Frontend/CompilerInvocation.cpp
+++ b/lib/Frontend/CompilerInvocation.cpp
@@ -112,8 +112,6 @@ static void AnalyzerOptsToArgs(const AnalyzerOptions &Opts,
     Res.push_back("-analyzer-experimental-checks");
   if (Opts.EnableExperimentalInternalChecks)
     Res.push_back("-analyzer-experimental-internal-checks");
-  if (Opts.EnableIdempotentOperationChecker)
-    Res.push_back("-analyzer-idempotent-operation");
 }
 
 static void CodeGenOptsToArgs(const CodeGenOptions &Opts,
@@ -792,8 +790,6 @@ static void ParseAnalyzerArgs(AnalyzerOptions &Opts, ArgList &Args,
   Opts.EnableExperimentalChecks = Args.hasArg(OPT_analyzer_experimental_checks);
   Opts.EnableExperimentalInternalChecks =
     Args.hasArg(OPT_analyzer_experimental_internal_checks);
-  Opts.EnableIdempotentOperationChecker =
-    Args.hasArg(OPT_analyzer_idempotent_operation);
   Opts.TrimGraph = Args.hasArg(OPT_trim_egraph);
   Opts.MaxNodes = Args.getLastArgIntValue(OPT_analyzer_max_nodes, 150000,Diags);
   Opts.MaxLoop = Args.getLastArgIntValue(OPT_analyzer_max_loop, 3, Diags);

diff --git a/test/Analysis/constant-folding.c b/test/Analysis/constant-folding.c
index 6ed2b390cf7afbf88af6310c487b4434d020022c..5a7e6be1dc17c9f74f0c3c4e2aec548932945bcb 100644 (file)
--- a/test/Analysis/constant-folding.c
+++ b/test/Analysis/constant-folding.c
@@ -18,10 +18,10 @@ void testComparisons (int a) {
 }
 
 void testSelfOperations (int a) {
-  if ((a|a) != a) WARN;
-  if ((a&a) != a) WARN;
-  if ((a^a) != 0) WARN;
-  if ((a-a) != 0) WARN;
+  if ((a|a) != a) WARN; // expected-warning{{idempotent operation}}
+  if ((a&a) != a) WARN; // expected-warning{{idempotent operation}}
+  if ((a^a) != 0) WARN; // expected-warning{{idempotent operation}}
+  if ((a-a) != 0) WARN; // expected-warning{{idempotent operation}}
 }
 
 void testIdempotent (int a) {
@@ -68,5 +68,5 @@ void testLocations (char *a) {
   if (b!=a) WARN;
   if (b>a) WARN;
   if (b<a) WARN;
-  if (b-a) WARN;
+  if (b-a) WARN; // expected-warning{{idempotent operation}}
 }

diff --git a/test/Analysis/dead-stores.c b/test/Analysis/dead-stores.c
index defd7e0b7bdf9922bd2b62828522807a5b21dd7f..a295d96464356223e5c3e4b96ebe305c90bf710d 100644 (file)
--- a/test/Analysis/dead-stores.c
+++ b/test/Analysis/dead-stores.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -Wunused-variable -analyze -analyzer-experimental-internal-checks -analyzer-check-dead-stores -fblocks -verify -Wno-unreachable-code -analyzer-opt-analyze-nested-blocks %s
+// RUN: %clang_cc1 -Wunused-variable -analyze -analyzer-experimental-internal-checks -analyzer-check-objc-mem -analyzer-check-dead-stores -fblocks -verify -Wno-unreachable-code -analyzer-opt-analyze-nested-blocks %s
 // RUN: %clang_cc1 -Wunused-variable -analyze -analyzer-experimental-internal-checks -analyzer-check-objc-mem -analyzer-store=basic -analyzer-constraints=basic -analyzer-check-dead-stores -fblocks -verify -Wno-unreachable-code -analyzer-opt-analyze-nested-blocks %s
 // RUN: %clang_cc1 -Wunused-variable -analyze -analyzer-experimental-internal-checks -analyzer-check-objc-mem -analyzer-store=basic -analyzer-constraints=range -analyzer-check-dead-stores -fblocks -verify -Wno-unreachable-code -analyzer-opt-analyze-nested-blocks %s
 // RUN: %clang_cc1 -Wunused-variable -analyze -analyzer-experimental-internal-checks -analyzer-check-objc-mem -analyzer-store=region -analyzer-constraints=basic -analyzer-check-dead-stores -fblocks -verify -Wno-unreachable-code -analyzer-opt-analyze-nested-blocks %s
@@ -158,7 +158,7 @@ int f16(int x) {
 // Self-assignments should not be flagged as dead stores.
 void f17() {
   int x = 1;
-  x = x; // no-warning
+  x = x; // expected-warning{{idempotent operation}}
 }
 
 // <rdar://problem/6506065>
@@ -458,7 +458,7 @@ void rdar8014335() {
     // Note that the next value stored to 'i' is never executed
     // because the next statement to be executed is the 'break'
     // in the increment code of the first loop.
-    i = i * 3; // expected-warning{{Value stored to 'i' is never read}}
+    i = i * 3; // expected-warning{{Value stored to 'i' is never read}} expected-warning{{idempotent operation}}
   }
 }
 

diff --git a/test/Analysis/idempotent-operations.c b/test/Analysis/idempotent-operations.c
index 9cef08edc43f8f92efd5cf8678bc4e2906fc2c37..72adb8ef25cfc67ef4513412287330a04911d309 100644 (file)
--- a/test/Analysis/idempotent-operations.c
+++ b/test/Analysis/idempotent-operations.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -analyze -analyzer-idempotent-operation -analyzer-store=region -analyzer-constraints=range -fblocks -verify -analyzer-opt-analyze-nested-blocks -analyzer-check-objc-mem -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-store=region -analyzer-constraints=range -fblocks -verify -analyzer-opt-analyze-nested-blocks -analyzer-check-objc-mem -verify %s
 
 // Basic tests
 

diff --git a/test/Analysis/misc-ps.m b/test/Analysis/misc-ps.m
index b1d47e214ef79247cfaab8c7b3067889e0d3e2e6..7de1305049be03eac2518cbe486531b7a4bd0f71 100644 (file)
--- a/test/Analysis/misc-ps.m
+++ b/test/Analysis/misc-ps.m
@@ -86,11 +86,11 @@ unsigned r6268365Aux();
 
 void r6268365() {
   unsigned x = 0;
-  x &= r6268365Aux();
+  x &= r6268365Aux(); // expected-warning{{idempotent operation}}
   unsigned j = 0;
     
   if (x == 0) ++j;
-  if (x == 0) x = x / j; // no-warning
+  if (x == 0) x = x / j; // expected-warning{{idempotent operation}} expected-warning{{idempotent operation}}
 }
 
 void divzeroassume(unsigned x, unsigned j) {  

diff --git a/test/Analysis/null-deref-ps.c b/test/Analysis/null-deref-ps.c
index 7ca22ada7da7fd83684ea0ef84dc12ca5abfd859..0b4153c8344d616d06dd3840703f27d0895fb01c 100644 (file)
--- a/test/Analysis/null-deref-ps.c
+++ b/test/Analysis/null-deref-ps.c
@@ -280,7 +280,7 @@ void f12(HF12ITEM i, char *q) {
 // Test handling of translating between integer "pointers" and back.
 void f13() {
   int *x = 0;
-  if (((((int) x) << 2) + 1) >> 1) *x = 1; // no-warning
+  if (((((int) x) << 2) + 1) >> 1) *x = 1; // expected-warning{{idempotent operation}}
 }
 
 // PR 4759 - Attribute non-null checking by the analyzer was not correctly

diff --git a/test/Analysis/rdar-6541136-region.c b/test/Analysis/rdar-6541136-region.c
index 82232c6bb97db3f78ca72b0399ddaf4c985ec338..c1734eec9baa6193077e2b0c7babd4c7e9ea31f9 100644 (file)
--- 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;
+  cmd = cmd; // expected-warning{{idempotent operation}}
   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;
+  cmd = cmd; // expected-warning{{idempotent operation}}
   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 844a9367b4313305ea3acdd79083f8c59c256d63..c729cb518fe11a2ac49b0367bac62800d3497fff 100644 (file)
--- 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;
+  cmd = cmd; // expected-warning{{idempotent operation}}
   char *p = (void*) &wonky[1];
   *p = 1;
   kernel_tea_cheese_t *q = &wonky[1];