Commit b9bd6ebe authored by Kristof Umann's avatar Kristof Umann
Browse files

[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
parent 7f7b2966

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h

+5 −18
Original line number Diff line number Diff line
@@ -107,22 +107,19 @@ protected: 
  ExtraTextList ExtraText;

  NoteList Notes;



  using Symbols = llvm::DenseSet<SymbolRef>;

  using Regions = llvm::DenseSet<const MemRegion *>;



  /// 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<Symbols *, 2> interestingSymbols;

  llvm::DenseSet<SymbolRef> 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<Regions *, 2> interestingRegions;

  llvm::DenseSet<const MemRegion *> 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<const ExplodedNode *, 4> 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<BugReport> 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<PathDiagnosticConsumer*> 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.

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

+7 −46
Original line number Diff line number Diff line
@@ -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<SymbolMetadata>(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<SymbolicRegion>(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<SymbolicRegion>(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&