From b9bd6ebe1dca7978b86f27c583e5b471b447108b Mon Sep 17 00:00:00 2001 From: Kristof Umann Date: Tue, 13 Aug 2019 13:09:48 +0000 Subject: [PATCH] [analyzer][NFC] Refactoring BugReporter.cpp P1.: Store interesting symbols/regions in a simple set The goal of this refactoring effort was to better understand how interestingness was propagated in BugReporter.cpp, which eventually turned out to be a dead end, but with such a twist, I wouldn't even want to spoil it ahead of time. However, I did get to learn a lot about how things are working in there. In these series of patches, as well as cleaning up the code big time, I invite you to study how BugReporter.cpp operates, and discuss how we could design this file to reduce the horrible mess that it is. This patch reverts a great part of rC162028, which holds the title "Allow multiple PathDiagnosticConsumers to be used with a BugReporter at the same time.". This, however doesn't imply that there's any need for multiple "layers" or stacks of interesting symbols and regions, quite the contrary, I would argue that we would like to generate the same amount of information for all output types, and only process them differently. Differential Revision: https://reviews.llvm.org/D65378 llvm-svn: 368689 --- .../StaticAnalyzer/Core/BugReporter/BugReporter.h | 23 ++-------- clang/lib/StaticAnalyzer/Core/BugReporter.cpp | 53 +++------------------- 2 files changed, 12 insertions(+), 64 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h index d30ad19..f99af2b 100644 --- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h +++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h @@ -107,22 +107,19 @@ protected: ExtraTextList ExtraText; NoteList Notes; - using Symbols = llvm::DenseSet; - using Regions = llvm::DenseSet; - /// A (stack of) a set of symbols that are registered with this /// report as being "interesting", and thus used to help decide which /// diagnostics to include when constructing the final path diagnostic. /// The stack is largely used by BugReporter when generating PathDiagnostics /// for multiple PathDiagnosticConsumers. - SmallVector interestingSymbols; + llvm::DenseSet InterestingSymbols; /// A (stack of) set of regions that are registered with this report as being /// "interesting", and thus used to help decide which diagnostics /// to include when constructing the final path diagnostic. /// The stack is largely used by BugReporter when generating PathDiagnostics /// for multiple PathDiagnosticConsumers. - SmallVector interestingRegions; + llvm::DenseSet InterestingRegions; /// A set of location contexts that correspoind to call sites which should be /// considered "interesting". @@ -156,15 +153,6 @@ protected: /// Conditions we're already tracking. llvm::SmallSet TrackedConditions; -private: - // Used internally by BugReporter. - Symbols &getInterestingSymbols(); - Regions &getInterestingRegions(); - - void lazyInitializeInterestingSets(); - void pushInterestingSymbolsAndRegions(); - void popInterestingSymbolsAndRegions(); - public: BugReport(const BugType& bt, StringRef desc, const ExplodedNode *errornode) : BT(bt), Description(desc), ErrorNode(errornode) {} @@ -189,7 +177,7 @@ public: : BT(bt), Description(desc), UniqueingLocation(LocationToUnique), UniqueingDecl(DeclToUnique), ErrorNode(errornode) {} - virtual ~BugReport(); + virtual ~BugReport() = default; const BugType& getBugType() const { return BT; } //BugType& getBugType() { return BT; } @@ -381,7 +369,6 @@ class BugReportEquivClass : public llvm::FoldingSetNode { public: BugReportEquivClass(std::unique_ptr R) { AddReport(std::move(R)); } - ~BugReportEquivClass(); void Profile(llvm::FoldingSetNodeID& ID) const { assert(!Reports.empty()); @@ -404,7 +391,7 @@ public: class BugReporterData { public: - virtual ~BugReporterData(); + virtual ~BugReporterData() = default; virtual DiagnosticsEngine& getDiagnostic() = 0; virtual ArrayRef getPathDiagnosticConsumers() = 0; @@ -526,7 +513,7 @@ public: GRBugReporter(BugReporterData& d, ExprEngine& eng) : BugReporter(d, GRBugReporterKind), Eng(eng) {} - ~GRBugReporter() override; + ~GRBugReporter() override = default;; /// getGraph - Get the exploded graph created by the analysis engine /// for the analyzed method or function. diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp index e5a0794..262c6a1 100644 --- a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp @@ -2058,12 +2058,6 @@ void BugReport::clearVisitors() { Callbacks.clear(); } -BugReport::~BugReport() { - while (!interestingSymbols.empty()) { - popInterestingSymbolsAndRegions(); - } -} - const Decl *BugReport::getDeclWithIssue() const { if (DeclWithIssue) return DeclWithIssue; @@ -2101,10 +2095,10 @@ void BugReport::markInteresting(SymbolRef sym) { if (!sym) return; - getInterestingSymbols().insert(sym); + InterestingSymbols.insert(sym); if (const auto *meta = dyn_cast(sym)) - getInterestingRegions().insert(meta->getRegion()); + InterestingRegions.insert(meta->getRegion()); } void BugReport::markInteresting(const MemRegion *R) { @@ -2112,10 +2106,10 @@ void BugReport::markInteresting(const MemRegion *R) { return; R = R->getBaseRegion(); - getInterestingRegions().insert(R); + InterestingRegions.insert(R); if (const auto *SR = dyn_cast(R)) - getInterestingSymbols().insert(SR->getSymbol()); + InterestingSymbols.insert(SR->getSymbol()); } void BugReport::markInteresting(SVal V) { @@ -2138,18 +2132,18 @@ bool BugReport::isInteresting(SymbolRef sym) { return false; // We don't currently consider metadata symbols to be interesting // even if we know their region is interesting. Is that correct behavior? - return getInterestingSymbols().count(sym); + return InterestingSymbols.count(sym); } bool BugReport::isInteresting(const MemRegion *R) { if (!R) return false; R = R->getBaseRegion(); - bool b = getInterestingRegions().count(R); + bool b = InterestingRegions.count(R); if (b) return true; if (const auto *SR = dyn_cast(R)) - return getInterestingSymbols().count(SR->getSymbol()); + return InterestingSymbols.count(SR->getSymbol()); return false; } @@ -2159,33 +2153,6 @@ bool BugReport::isInteresting(const LocationContext *LC) { return InterestingLocationContexts.count(LC); } -void BugReport::lazyInitializeInterestingSets() { - if (interestingSymbols.empty()) { - interestingSymbols.push_back(new Symbols()); - interestingRegions.push_back(new Regions()); - } -} - -BugReport::Symbols &BugReport::getInterestingSymbols() { - lazyInitializeInterestingSets(); - return *interestingSymbols.back(); -} - -BugReport::Regions &BugReport::getInterestingRegions() { - lazyInitializeInterestingSets(); - return *interestingRegions.back(); -} - -void BugReport::pushInterestingSymbolsAndRegions() { - interestingSymbols.push_back(new Symbols(getInterestingSymbols())); - interestingRegions.push_back(new Regions(getInterestingRegions())); -} - -void BugReport::popInterestingSymbolsAndRegions() { - delete interestingSymbols.pop_back_val(); - delete interestingRegions.pop_back_val(); -} - const Stmt *BugReport::getStmt() const { if (!ErrorNode) return nullptr; @@ -2236,12 +2203,6 @@ PathDiagnosticLocation BugReport::getLocation(const SourceManager &SM) const { // Methods for BugReporter and subclasses. //===----------------------------------------------------------------------===// -BugReportEquivClass::~BugReportEquivClass() = default; - -GRBugReporter::~GRBugReporter() = default; - -BugReporterData::~BugReporterData() = default; - ExplodedGraph &GRBugReporter::getGraph() { return Eng.getGraph(); } ProgramStateManager& -- 2.7.4