From: Tom Care Date: Tue, 24 Aug 2010 21:09:07 +0000 (+0000) Subject: Improvements to IdempotentOperationChecker and its use of PseudoConstantAnalysis X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=ef52bcb606c73950139a775af61495f63fbc3603;p=clang 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 --- 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 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 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(Head); - const Expr *LHS = BO->getLHS()->IgnoreParenImpCasts(); + const Expr *LHS = BO->getLHS()->IgnoreParenCasts(); const DeclRefExpr *DR = dyn_cast(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(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(VD->getBody())) - if (const VarDecl *RefVD = dyn_cast(DR->getDecl())) + if (const DeclRefExpr *DR = dyn_cast(VD->getInit())) + if (const VarDecl *RefVD = dyn_cast(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(Head); + if (const VarDecl *VD = dyn_cast(BDR->getDecl())) { + NonConstants->insert(VD); + UsedVars->insert(VD); + continue; } + break; + } + + // Case 5: Variable references + case Stmt::DeclRefExprClass: { + const DeclRefExpr *DR = cast(Head); + if (const VarDecl *VD = dyn_cast(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(LHS_DR->getDecl()); - if (!PD) + const VarDecl *VD = dyn_cast(LHS_DR->getDecl()); + if (!VD) return false; const DeclRefExpr *RHS_DR = dyn_cast(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; } // 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];