From: Tom Care Date: Mon, 30 Aug 2010 19:25:43 +0000 (+0000) Subject: Adjusted the semantics of assign checking in IdempotentOperationChecker X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=6216dc0c5b9071b4c10f78718a64ca916c00a384;p=clang 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 8a9e333ce5..3d4be1a119 100644 --- 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(E->IgnoreParenCasts()); + if (!DR) + return false; + + const VarDecl *VD = dyn_cast(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(Ex), AC); // The next cases require recursion for subexpressions @@ -593,14 +608,14 @@ bool IdempotentOperationChecker::CanVary(const Expr *Ex, return CanVary(cast(Ex)->getChosenSubExpr( AC->getASTContext()), AC); case Stmt::ConditionalOperatorClass: - return CanVary(cast(Ex)->getCond(), AC); + return CanVary(cast(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 a7e84615fd..53c873df55 100644 --- 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 debff0efff..5c9a59d736 100644 --- 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 d46437388d..38275335ee 100644 --- 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 6aac74b109..42eccfeec4 100644 --- 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]; }