From: Tom Care Date: Mon, 23 Aug 2010 19:51:57 +0000 (+0000) Subject: Several small changes to PseudoConstantAnalysis and the way IdempotentOperationChecke... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=db34ab70961ca4b24b600eb47053d7af304659f5;p=clang Several small changes to PseudoConstantAnalysis and the way IdempotentOperationChecker uses it. - Psuedo -> Pseudo (doh...) - C++ reference support - Added pseudoconstant test case for __block vars - Separated out static local checking from pseudoconstant analysis and generalized to non-local checking - Added missing test cases for storage false positives git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@111832 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Analysis/Analyses/PsuedoConstantAnalysis.h b/include/clang/Analysis/Analyses/PseudoConstantAnalysis.h similarity index 60% rename from include/clang/Analysis/Analyses/PsuedoConstantAnalysis.h rename to include/clang/Analysis/Analyses/PseudoConstantAnalysis.h index 080f6c1587..0dd534c2a8 100644 --- a/include/clang/Analysis/Analyses/PsuedoConstantAnalysis.h +++ b/include/clang/Analysis/Analyses/PseudoConstantAnalysis.h @@ -1,4 +1,4 @@ -//== PsuedoConstantAnalysis.h - Find Psuedo-constants in the AST -*- C++ -*-==// +//== PseudoConstantAnalysis.h - Find Pseudo-constants in the AST -*- C++ -*-==// // // The LLVM Compiler Infrastructure // @@ -13,27 +13,25 @@ // //===----------------------------------------------------------------------===// -#ifndef LLVM_CLANG_ANALYSIS_PSUEDOCONSTANTANALYSIS -#define LLVM_CLANG_ANALYSIS_PSUEDOCONSTANTANALYSIS +#ifndef LLVM_CLANG_ANALYSIS_PSEUDOCONSTANTANALYSIS +#define LLVM_CLANG_ANALYSIS_PSEUDOCONSTANTANALYSIS #include "clang/AST/Stmt.h" -// The number of ValueDecls we want to keep track of by default (per-function) -#define VALUEDECL_SET_SIZE 256 - namespace clang { -class PsuedoConstantAnalysis { +class PseudoConstantAnalysis { public: - PsuedoConstantAnalysis(const Stmt *DeclBody) : - DeclBody(DeclBody), Analyzed(false) {} - bool isPsuedoConstant(const ValueDecl *VD); + PseudoConstantAnalysis(const Stmt *DeclBody); + ~PseudoConstantAnalysis(); + + bool isPseudoConstant(const VarDecl *VD); private: void RunAnalysis(); // for storing the result of analyzed ValueDecls - llvm::SmallPtrSet NonConstants; + void *NonConstantsImpl; const Stmt *DeclBody; bool Analyzed; diff --git a/include/clang/Analysis/AnalysisContext.h b/include/clang/Analysis/AnalysisContext.h index 124a08e073..6091db4be9 100644 --- a/include/clang/Analysis/AnalysisContext.h +++ b/include/clang/Analysis/AnalysisContext.h @@ -30,7 +30,7 @@ class CFG; class CFGBlock; class LiveVariables; class ParentMap; -class PsuedoConstantAnalysis; +class PseudoConstantAnalysis; class ImplicitParamDecl; class LocationContextManager; class StackFrameContext; @@ -50,7 +50,7 @@ class AnalysisContext { bool builtCFG, builtCompleteCFG; LiveVariables *liveness; ParentMap *PM; - PsuedoConstantAnalysis *PCA; + PseudoConstantAnalysis *PCA; llvm::DenseMap *ReferencedBlockVars; llvm::BumpPtrAllocator A; bool UseUnoptimizedCFG; @@ -87,7 +87,7 @@ public: CFG *getUnoptimizedCFG(); ParentMap &getParentMap(); - PsuedoConstantAnalysis *getPsuedoConstantAnalysis(); + PseudoConstantAnalysis *getPseudoConstantAnalysis(); LiveVariables *getLiveVariables(); typedef const VarDecl * const * referenced_decls_iterator; diff --git a/lib/Analysis/AnalysisContext.cpp b/lib/Analysis/AnalysisContext.cpp index 934a031b3a..2c337f07c3 100644 --- a/lib/Analysis/AnalysisContext.cpp +++ b/lib/Analysis/AnalysisContext.cpp @@ -18,7 +18,7 @@ #include "clang/AST/ParentMap.h" #include "clang/AST/StmtVisitor.h" #include "clang/Analysis/Analyses/LiveVariables.h" -#include "clang/Analysis/Analyses/PsuedoConstantAnalysis.h" +#include "clang/Analysis/Analyses/PseudoConstantAnalysis.h" #include "clang/Analysis/AnalysisContext.h" #include "clang/Analysis/CFG.h" #include "clang/Analysis/Support/BumpVector.h" @@ -84,9 +84,9 @@ ParentMap &AnalysisContext::getParentMap() { return *PM; } -PsuedoConstantAnalysis *AnalysisContext::getPsuedoConstantAnalysis() { +PseudoConstantAnalysis *AnalysisContext::getPseudoConstantAnalysis() { if (!PCA) - PCA = new PsuedoConstantAnalysis(getBody()); + PCA = new PseudoConstantAnalysis(getBody()); return PCA; } diff --git a/lib/Analysis/CMakeLists.txt b/lib/Analysis/CMakeLists.txt index 514042bf9a..850e9b4681 100644 --- a/lib/Analysis/CMakeLists.txt +++ b/lib/Analysis/CMakeLists.txt @@ -7,7 +7,7 @@ add_clang_library(clangAnalysis FormatString.cpp LiveVariables.cpp PrintfFormatString.cpp - PsuedoConstantAnalysis.cpp + PseudoConstantAnalysis.cpp ReachableCode.cpp ScanfFormatString.cpp UninitializedValues.cpp diff --git a/lib/Analysis/PsuedoConstantAnalysis.cpp b/lib/Analysis/PseudoConstantAnalysis.cpp similarity index 60% rename from lib/Analysis/PsuedoConstantAnalysis.cpp rename to lib/Analysis/PseudoConstantAnalysis.cpp index a169f89fe1..7a2fa8a8bd 100644 --- a/lib/Analysis/PsuedoConstantAnalysis.cpp +++ b/lib/Analysis/PseudoConstantAnalysis.cpp @@ -1,4 +1,4 @@ -//== PsuedoConstantAnalysis.cpp - Find Psuedoconstants in the AST-*- C++ -*-==// +//== PseudoConstantAnalysis.cpp - Find Pseudoconstants in the AST-*- C++ -*-==// // // The LLVM Compiler Infrastructure // @@ -13,7 +13,7 @@ // //===----------------------------------------------------------------------===// -#include "clang/Analysis/Analyses/PsuedoConstantAnalysis.h" +#include "clang/Analysis/Analyses/PseudoConstantAnalysis.h" #include "clang/AST/Decl.h" #include "clang/AST/Expr.h" #include "clang/AST/Stmt.h" @@ -21,18 +21,38 @@ using namespace clang; +// The number of ValueDecls we want to keep track of by default (per-function) +#define VARDECL_SET_SIZE 256 +typedef llvm::SmallPtrSet VarDeclSet; + +PseudoConstantAnalysis::PseudoConstantAnalysis(const Stmt *DeclBody) : + DeclBody(DeclBody), Analyzed(false) { + NonConstantsImpl = new VarDeclSet; +} + +PseudoConstantAnalysis::~PseudoConstantAnalysis() { + delete (VarDeclSet*)NonConstantsImpl; +} + // Returns true if the given ValueDecl is never written to in the given DeclBody -bool PsuedoConstantAnalysis::isPsuedoConstant(const ValueDecl *VD) { +bool PseudoConstantAnalysis::isPseudoConstant(const VarDecl *VD) { + // Only local and static variables can be pseudoconstants + if (!VD->hasLocalStorage() && !VD->isStaticLocal()) + return false; + if (!Analyzed) { RunAnalysis(); Analyzed = true; } - return !NonConstants.count(VD); + VarDeclSet *NonConstants = (VarDeclSet*)NonConstantsImpl; + + return !NonConstants->count(VD); } -void PsuedoConstantAnalysis::RunAnalysis() { +void PseudoConstantAnalysis::RunAnalysis() { std::deque WorkList; + VarDeclSet *NonConstants = (VarDeclSet*)NonConstantsImpl; // Start with the top level statement of the function WorkList.push_back(DeclBody); @@ -65,10 +85,13 @@ void PsuedoConstantAnalysis::RunAnalysis() { case BinaryOperator::OrAssign: case BinaryOperator::XorAssign: case BinaryOperator::ShlAssign: - case BinaryOperator::ShrAssign: + case BinaryOperator::ShrAssign: { // The DeclRefExpr is being assigned to - mark it as non-constant - NonConstants.insert(DR->getDecl()); - continue; // Continue without looking at children + const VarDecl *VD = dyn_cast(DR->getDecl()); + if (VD) + NonConstants->insert(VD); + break; + } default: break; @@ -95,10 +118,14 @@ void PsuedoConstantAnalysis::RunAnalysis() { case UnaryOperator::PreDec: case UnaryOperator::PreInc: // The DeclRefExpr is being changed - mark it as non-constant - case UnaryOperator::AddrOf: + case UnaryOperator::AddrOf: { // If we are taking the address of the DeclRefExpr, assume it is // non-constant. - NonConstants.insert(DR->getDecl()); + const VarDecl *VD = dyn_cast(DR->getDecl()); + if (VD) + NonConstants->insert(VD); + break; + } default: break; @@ -106,6 +133,33 @@ void PsuedoConstantAnalysis::RunAnalysis() { break; } + // Case 3: Reference Declarations + case Stmt::DeclStmtClass: { + const DeclStmt *DS = cast(Head); + // Iterate over each decl and see if any of them contain reference decls + for (DeclStmt::const_decl_iterator I = DS->decl_begin(), E = DS->decl_end(); + I != E; ++I) { + // We only care about VarDecls + const VarDecl *VD = dyn_cast(*I); + if (!VD) + continue; + + // We found a VarDecl; make sure it is a reference type + 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())) + NonConstants->insert(RefVD); + } + } + default: break; } // switch (head->getStmtClass()) diff --git a/lib/Checker/IdempotentOperationChecker.cpp b/lib/Checker/IdempotentOperationChecker.cpp index 885123ae24..2cfcaa44d4 100644 --- a/lib/Checker/IdempotentOperationChecker.cpp +++ b/lib/Checker/IdempotentOperationChecker.cpp @@ -44,7 +44,7 @@ #include "GRExprEngineExperimentalChecks.h" #include "clang/Analysis/CFGStmtMap.h" -#include "clang/Analysis/Analyses/PsuedoConstantAnalysis.h" +#include "clang/Analysis/Analyses/PseudoConstantAnalysis.h" #include "clang/Checker/BugReporter/BugType.h" #include "clang/Checker/PathSensitive/CheckerHelpers.h" #include "clang/Checker/PathSensitive/CheckerVisitor.h" @@ -83,6 +83,7 @@ class IdempotentOperationChecker static bool CanVary(const Expr *Ex, AnalysisContext *AC); static bool isConstantOrPseudoConstant(const DeclRefExpr *DR, AnalysisContext *AC); + static bool containsNonLocalVarDecl(const Stmt *S); // Hash table typedef llvm::DenseMapgetLHS(); const Expr *RHS = B->getRHS(); - // Check if either side can vary. We only need to calculate this when we have - // no assumption. - bool LHSCanVary = true, RHSCanVary = true; + // At this stage we can calculate whether each side contains a false positive + // that applies to all operators. We only need to calculate this the first + // time. + bool LHSContainsFalsePositive = false, RHSContainsFalsePositive = false; if (A == Possible) { - LHSCanVary = CanVary(LHS, AC); - RHSCanVary = CanVary(RHS, AC); + // An expression contains a false positive if it can't vary, or if it + // contains a known false positive VarDecl. + LHSContainsFalsePositive = !CanVary(LHS, AC) + || containsNonLocalVarDecl(LHS); + RHSContainsFalsePositive = !CanVary(RHS, AC) + || containsNonLocalVarDecl(RHS); } const GRState *state = C.getState(); @@ -195,7 +201,8 @@ void IdempotentOperationChecker::PreVisitBinaryOperator( case BinaryOperator::Xor: case BinaryOperator::LOr: case BinaryOperator::LAnd: - if (LHSVal != RHSVal || !LHSCanVary || !RHSCanVary) + if (LHSVal != RHSVal || LHSContainsFalsePositive + || RHSContainsFalsePositive) break; UpdateAssumption(A, Equal); return; @@ -213,7 +220,7 @@ void IdempotentOperationChecker::PreVisitBinaryOperator( case BinaryOperator::Div: case BinaryOperator::LOr: case BinaryOperator::LAnd: - if (!RHSVal.isConstant(1) || !RHSCanVary) + if (!RHSVal.isConstant(1) || RHSContainsFalsePositive) break; UpdateAssumption(A, RHSis1); return; @@ -229,7 +236,7 @@ void IdempotentOperationChecker::PreVisitBinaryOperator( case BinaryOperator::Mul: case BinaryOperator::LOr: case BinaryOperator::LAnd: - if (!LHSVal.isConstant(1) || !LHSCanVary) + if (!LHSVal.isConstant(1) || LHSContainsFalsePositive) break; UpdateAssumption(A, LHSis1); return; @@ -257,7 +264,7 @@ void IdempotentOperationChecker::PreVisitBinaryOperator( case BinaryOperator::Shr: case BinaryOperator::LOr: case BinaryOperator::LAnd: - if (!RHSVal.isConstant(0) || !RHSCanVary) + if (!RHSVal.isConstant(0) || RHSContainsFalsePositive) break; UpdateAssumption(A, RHSis0); return; @@ -289,7 +296,7 @@ void IdempotentOperationChecker::PreVisitBinaryOperator( case BinaryOperator::Shr: case BinaryOperator::LOr: case BinaryOperator::LAnd: - if (!LHSVal.isConstant(0) || !LHSCanVary) + if (!LHSVal.isConstant(0) || LHSContainsFalsePositive) break; UpdateAssumption(A, LHSis0); return; @@ -578,16 +585,35 @@ bool IdempotentOperationChecker::isConstantOrPseudoConstant( if (isa(DR->getDecl())) return true; - // Check for a static variable - // FIXME: Analysis should model static vars - if (const VarDecl *VD = dyn_cast(DR->getDecl())) - if (VD->isStaticLocal()) - return true; + const VarDecl *VD = dyn_cast(DR->getDecl()); + if (!VD) + return true; - // Check if the Decl behaves like a constant - PsuedoConstantAnalysis *PCA = AC->getPsuedoConstantAnalysis(); - if (PCA->isPsuedoConstant(DR->getDecl())) + // Check if the Decl behaves like a constant. This check also takes care of + // static variables, which can only change between function calls if they are + // modified in the AST. + PseudoConstantAnalysis *PCA = AC->getPseudoConstantAnalysis(); + if (PCA->isPseudoConstant(VD)) return true; return false; } + +// Recursively find any substatements containing VarDecl's with storage other +// than local +bool IdempotentOperationChecker::containsNonLocalVarDecl(const Stmt *S) { + const DeclRefExpr *DR = dyn_cast(S); + + if (DR) + if (const VarDecl *VD = dyn_cast(DR->getDecl())) + if (!VD->hasLocalStorage()) + return true; + + for (Stmt::const_child_iterator I = S->child_begin(); I != S->child_end(); + ++I) + if (const Stmt *child = *I) + if (containsNonLocalVarDecl(child)) + return true; + + return false; +} diff --git a/test/Analysis/idempotent-operations.c b/test/Analysis/idempotent-operations.c index abf67103c1..179c7a4da3 100644 --- a/test/Analysis/idempotent-operations.c +++ b/test/Analysis/idempotent-operations.c @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -analyze -analyzer-store=region -analyzer-constraints=range -fblocks -verify -analyzer-opt-analyze-nested-blocks -analyzer-check-objc-mem -analyzer-check-idempotent-operations -verify %s +// 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 // Basic tests @@ -51,7 +51,7 @@ unsigned basic() { test(zero << x); // expected-warning {{The left operand to '<<' is always 0}} test(zero >> x); // expected-warning {{The left operand to '>>' is always 0}} - // Overwrite the values so these aren't marked as psuedoconstants + // Overwrite the values so these aren't marked as Pseudoconstants x = 1; zero = 2; one = 3; @@ -102,12 +102,38 @@ unsigned false3(int param) { return nonparam; } -// Psuedo-constants (vars only read) and constants should not be reported +// Pseudo-constants (vars only read) and constants should not be reported unsigned false4() { // Trivial constant - const int height = 1; // no-warning - // Psuedo-constant (never changes after decl) - int width = height; // no-warning + const int height = 1; + + // Pseudo-constant (never changes after decl) + int width = height; + + // Pseudo-constant (blockvar) + __block int a = 0; + int b = 10; + a *= b; // no-warning + test(a); return width * 10; // no-warning } + +// Static vars are common false positives +int false5() { + static int test = 0; + int a = 56; + a *= test; // no-warning + test++; + return a; +} + +// Non-local storage vars are considered false positives +int globalInt = 1; +int false6() { + int localInt = 23; + + localInt /= globalInt; + + return localInt; +}