diff --git a/clang/include/clang/Driver/CC1Options.td b/clang/include/clang/Driver/CC1Options.td index 7076f30995d0..4502401c16fd 100644 --- a/clang/include/clang/Driver/CC1Options.td +++ b/clang/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/clang/lib/Checker/AnalysisConsumer.cpp b/clang/lib/Checker/AnalysisConsumer.cpp index 524f37e39662..6c7a294b1d83 100644 --- a/clang/lib/Checker/AnalysisConsumer.cpp +++ b/clang/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 Auditor; if (mgr.shouldVisualizeUbigraph()) { diff --git a/clang/lib/Checker/GRExprEngine.cpp b/clang/lib/Checker/GRExprEngine.cpp index 07fee9d39e49..1424820f3b88 100644 --- a/clang/lib/Checker/GRExprEngine.cpp +++ b/clang/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/clang/lib/Checker/GRExprEngineExperimentalChecks.h b/clang/lib/Checker/GRExprEngineExperimentalChecks.h index 7d1eb77f9fd6..3c1c95ffefd4 100644 --- a/clang/lib/Checker/GRExprEngineExperimentalChecks.h +++ b/clang/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/clang/lib/Checker/GRExprEngineInternalChecks.h b/clang/lib/Checker/GRExprEngineInternalChecks.h index f91a759b3299..9644eafcb39f 100644 --- a/clang/lib/Checker/GRExprEngineInternalChecks.h +++ b/clang/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/clang/lib/Checker/IdempotentOperationChecker.cpp b/clang/lib/Checker/IdempotentOperationChecker.cpp index 6ed18417a2c2..744fe2ba9929 100644 --- a/clang/lib/Checker/IdempotentOperationChecker.cpp +++ b/clang/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(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(S); + + if (DR) + if (const VarDecl *VD = dyn_cast(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(LHS); + if (!LHS_DR) + return false; + + const ParmVarDecl *PD = dyn_cast(LHS_DR->getDecl()); + if (!PD) + return false; + + const DeclRefExpr *RHS_DR = dyn_cast(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(LHS->IgnoreParenCasts()); + if (!LHS_DR) + return false; + + const VarDecl *VD = dyn_cast(LHS_DR->getDecl()); + if (!VD) + return false; + + const DeclRefExpr *RHS_DR = dyn_cast(RHS->IgnoreParenCasts()); + if (!RHS_DR) + return false; + + if (VD != RHS_DR->getDecl()) + return false; + + return dyn_cast(RHS->IgnoreParens()) == NULL; +} + + // Recursively find any substatements containing __builtin_offset_of bool IdempotentOperationChecker::containsBuiltinOffsetOf(const Stmt *S) { const UnaryOperator *UO = dyn_cast(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(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(S); if (IL && IL->getValue() == 1) diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp index 418d25b0d47d..00363d91fd43 100644 --- a/clang/lib/Frontend/CompilerInvocation.cpp +++ b/clang/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/clang/test/Analysis/constant-folding.c b/clang/test/Analysis/constant-folding.c index 6ed2b390cf7a..5a7e6be1dc17 100644 --- a/clang/test/Analysis/constant-folding.c +++ b/clang/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 @@ -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/clang/test/Analysis/idempotent-operations.c b/clang/test/Analysis/idempotent-operations.c index 9cef08edc43f..72adb8ef25cf 100644 --- a/clang/test/Analysis/idempotent-operations.c +++ b/clang/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/clang/test/Analysis/misc-ps.m b/clang/test/Analysis/misc-ps.m index b1d47e214ef7..7de1305049be 100644 --- a/clang/test/Analysis/misc-ps.m +++ b/clang/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/clang/test/Analysis/null-deref-ps.c b/clang/test/Analysis/null-deref-ps.c index 7ca22ada7da7..0b4153c8344d 100644 --- a/clang/test/Analysis/null-deref-ps.c +++ b/clang/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/clang/test/Analysis/rdar-6541136-region.c b/clang/test/Analysis/rdar-6541136-region.c index 82232c6bb97d..c1734eec9baa 100644 --- a/clang/test/Analysis/rdar-6541136-region.c +++ b/clang/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/clang/test/Analysis/rdar-6541136.c b/clang/test/Analysis/rdar-6541136.c index 844a9367b431..c729cb518fe1 100644 --- a/clang/test/Analysis/rdar-6541136.c +++ b/clang/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];