From 2bbbe50dbaa0bf231c16333b335304655deeb26b Mon Sep 17 00:00:00 2001 From: Tom Care Date: Thu, 2 Sep 2010 23:30:22 +0000 Subject: [PATCH] Reapply 112850 and 112839 with a constructor for the BinaryOperatorData struct. Clang would zero out the enum and pointer in the struct in some conditions, but GCC would never zero out the values. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@112909 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../clang/Checker/BugReporter/BugReporter.h | 3 + lib/Checker/BugReporterVisitors.cpp | 37 +++++++++ lib/Checker/IdempotentOperationChecker.cpp | 77 +++++++++++++++---- 3 files changed, 100 insertions(+), 17 deletions(-) diff --git a/include/clang/Checker/BugReporter/BugReporter.h b/include/clang/Checker/BugReporter/BugReporter.h index 3749b43d7e..370d96552c 100644 --- a/include/clang/Checker/BugReporter/BugReporter.h +++ b/include/clang/Checker/BugReporter/BugReporter.h @@ -471,6 +471,9 @@ void registerFindLastStore(BugReporterContext& BRC, const void *memregion, void registerNilReceiverVisitor(BugReporterContext &BRC); +void registerVarDeclsLastStore(BugReporterContext &BRC, const void *stmt, + const ExplodedNode *N); + } // end namespace clang::bugreporter //===----------------------------------------------------------------------===// diff --git a/lib/Checker/BugReporterVisitors.cpp b/lib/Checker/BugReporterVisitors.cpp index cddc86ee9d..91cf349107 100644 --- a/lib/Checker/BugReporterVisitors.cpp +++ b/lib/Checker/BugReporterVisitors.cpp @@ -419,3 +419,40 @@ public: void clang::bugreporter::registerNilReceiverVisitor(BugReporterContext &BRC) { BRC.addVisitor(new NilReceiverVisitor()); } + +// Registers every VarDecl inside a Stmt with a last store vistor. +void clang::bugreporter::registerVarDeclsLastStore(BugReporterContext &BRC, + const void *stmt, + const ExplodedNode *N) { + const Stmt *S = static_cast(stmt); + + std::deque WorkList; + + WorkList.push_back(S); + + while (!WorkList.empty()) { + const Stmt *Head = WorkList.front(); + WorkList.pop_front(); + + GRStateManager &StateMgr = BRC.getStateManager(); + const GRState *state = N->getState(); + + if (const DeclRefExpr *DR = dyn_cast(Head)) { + if (const VarDecl *VD = dyn_cast(DR->getDecl())) { + const VarRegion *R = + StateMgr.getRegionManager().getVarRegion(VD, N->getLocationContext()); + + // What did we load? + SVal V = state->getSVal(S); + + if (isa(V) || isa(V)) { + ::registerFindLastStore(BRC, R, V); + } + } + } + + for (Stmt::const_child_iterator I = Head->child_begin(); + I != Head->child_end(); ++I) + WorkList.push_back(*I); + } +} diff --git a/lib/Checker/IdempotentOperationChecker.cpp b/lib/Checker/IdempotentOperationChecker.cpp index 3d4be1a119..f6970de783 100644 --- a/lib/Checker/IdempotentOperationChecker.cpp +++ b/lib/Checker/IdempotentOperationChecker.cpp @@ -45,6 +45,7 @@ #include "GRExprEngineExperimentalChecks.h" #include "clang/Analysis/CFGStmtMap.h" #include "clang/Analysis/Analyses/PseudoConstantAnalysis.h" +#include "clang/Checker/BugReporter/BugReporter.h" #include "clang/Checker/BugReporter/BugType.h" #include "clang/Checker/PathSensitive/CheckerHelpers.h" #include "clang/Checker/PathSensitive/CheckerVisitor.h" @@ -64,6 +65,7 @@ class IdempotentOperationChecker public: static void *getTag(); void PreVisitBinaryOperator(CheckerContext &C, const BinaryOperator *B); + void PostVisitBinaryOperator(CheckerContext &C, const BinaryOperator *B); void VisitEndAnalysis(ExplodedGraph &G, BugReporter &B, GRExprEngine &Eng); private: @@ -87,10 +89,17 @@ class IdempotentOperationChecker AnalysisContext *AC); static bool containsNonLocalVarDecl(const Stmt *S); - // Hash table - typedef llvm::DenseMap > - AssumptionMap; + // Hash table and related data structures + struct BinaryOperatorData { + BinaryOperatorData() : assumption(Possible), analysisContext(0) {} + + Assumption assumption; + AnalysisContext *analysisContext; + ExplodedNodeSet explodedNodes; // Set of ExplodedNodes that refer to a + // BinaryOperator + }; + typedef llvm::DenseMap + AssumptionMap; AssumptionMap hash; }; } @@ -109,11 +118,12 @@ void IdempotentOperationChecker::PreVisitBinaryOperator( const BinaryOperator *B) { // Find or create an entry in the hash for this BinaryOperator instance. // If we haven't done a lookup before, it will get default initialized to - // 'Possible'. - std::pair &Data = hash[B]; - Assumption &A = Data.first; + // 'Possible'. At this stage we do not store the ExplodedNode, as it has not + // been created yet. + BinaryOperatorData &Data = hash[B]; + Assumption &A = Data.assumption; AnalysisContext *AC = C.getCurrentAnalysisContext(); - Data.second = AC; + Data.analysisContext = AC; // If we already have visited this node on a path that does not contain an // idempotent operation, return immediately. @@ -317,16 +327,29 @@ void IdempotentOperationChecker::PreVisitBinaryOperator( A = Impossible; } +// At the post visit stage, the predecessor ExplodedNode will be the +// BinaryOperator that was just created. We use this hook to collect the +// ExplodedNode. +void IdempotentOperationChecker::PostVisitBinaryOperator( + CheckerContext &C, + const BinaryOperator *B) { + // Add the ExplodedNode we just visited + BinaryOperatorData &Data = hash[B]; + Data.explodedNodes.Add(C.getPredecessor()); +} + void IdempotentOperationChecker::VisitEndAnalysis(ExplodedGraph &G, BugReporter &BR, GRExprEngine &Eng) { + BugType *BT = new BugType("Idempotent operation", "Dead code"); // Iterate over the hash to see if we have any paths with definite // idempotent operations. for (AssumptionMap::const_iterator i = hash.begin(); i != hash.end(); ++i) { // Unpack the hash contents - const std::pair &Data = i->second; - const Assumption &A = Data.first; - AnalysisContext *AC = Data.second; + const BinaryOperatorData &Data = i->second; + const Assumption &A = Data.assumption; + AnalysisContext *AC = Data.analysisContext; + const ExplodedNodeSet &ES = Data.explodedNodes; const BinaryOperator *B = i->first; @@ -348,11 +371,14 @@ void IdempotentOperationChecker::VisitEndAnalysis(ExplodedGraph &G, delete CBM; } - // Select the error message. + // Select the error message and SourceRanges to report. llvm::SmallString<128> buf; llvm::raw_svector_ostream os(buf); + bool LHSRelevant = false, RHSRelevant = false; switch (A) { case Equal: + LHSRelevant = true; + RHSRelevant = true; if (B->getOpcode() == BO_Assign) os << "Assigned value is always the same as the existing value"; else @@ -360,15 +386,19 @@ void IdempotentOperationChecker::VisitEndAnalysis(ExplodedGraph &G, << "' always have the same value"; break; case LHSis1: + LHSRelevant = true; os << "The left operand to '" << B->getOpcodeStr() << "' is always 1"; break; case RHSis1: + RHSRelevant = true; os << "The right operand to '" << B->getOpcodeStr() << "' is always 1"; break; case LHSis0: + LHSRelevant = true; os << "The left operand to '" << B->getOpcodeStr() << "' is always 0"; break; case RHSis0: + RHSRelevant = true; os << "The right operand to '" << B->getOpcodeStr() << "' is always 0"; break; case Possible: @@ -377,11 +407,24 @@ void IdempotentOperationChecker::VisitEndAnalysis(ExplodedGraph &G, llvm_unreachable(0); } - // Create the SourceRange Arrays - SourceRange S[2] = { i->first->getLHS()->getSourceRange(), - i->first->getRHS()->getSourceRange() }; - BR.EmitBasicReport("Idempotent operation", "Dead code", - os.str(), i->first->getOperatorLoc(), S, 2); + // Add a report for each ExplodedNode + for (ExplodedNodeSet::iterator I = ES.begin(), E = ES.end(); I != E; ++I) { + EnhancedBugReport *report = new EnhancedBugReport(*BT, os.str(), *I); + + // Add source ranges and visitor hooks + if (LHSRelevant) { + const Expr *LHS = i->first->getLHS(); + report->addRange(LHS->getSourceRange()); + report->addVisitorCreator(bugreporter::registerVarDeclsLastStore, LHS); + } + if (RHSRelevant) { + const Expr *RHS = i->first->getRHS(); + report->addRange(i->first->getRHS()->getSourceRange()); + report->addVisitorCreator(bugreporter::registerVarDeclsLastStore, RHS); + } + + BR.EmitReport(report); + } } } -- 2.40.0