diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h index 8e1d25b3eefa1..7152a76d8649c 100644 --- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h +++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h @@ -294,8 +294,9 @@ class PathSensitiveBugReport : public BugReport { protected: /// The ExplodedGraph node against which the report was thrown. It corresponds - /// to the end of the execution path that demonstrates the bug. - const ExplodedNode *ErrorNode = nullptr; + /// to the end of the execution path that demonstrates the bug. This is never + /// a nullpointer. + const ExplodedNode *ErrorNode; /// The range that corresponds to ErrorNode's program point. It is usually /// highlighted in the report. diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h index e995151927c96..1fceb65d7bc1a 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h @@ -298,7 +298,9 @@ class ExplodedNode : public llvm::FoldingSetNode { }; using InterExplodedGraphMap = - llvm::DenseMap; + llvm::DenseMap; + +using TrimGraphWorklist = SmallVector; class ExplodedGraph { protected: @@ -406,22 +408,28 @@ class ExplodedGraph { llvm::BumpPtrAllocator & getAllocator() { return BVC.getAllocator(); } BumpVectorContext &getNodeAllocator() { return BVC; } - using NodeMap = llvm::DenseMap; - /// Creates a trimmed version of the graph that only contains paths leading /// to the given nodes. /// - /// \param Nodes The nodes which must appear in the final graph. Presumably - /// these are end-of-path nodes (i.e. they have no successors). - /// \param[out] ForwardMap An optional map from nodes in this graph to nodes - /// in the returned graph. - /// \param[out] InverseMap An optional map from nodes in the returned graph to - /// nodes in this graph. + /// \param[in,out] Worklist Vector of nodes which must appear in the final + /// graph. Presumably these are end-of-path nodes + /// (i.e. they have no successors). This argument is + /// consumed and emptied by the trimming algorithm. + /// + /// \param[out] NodeMap If specified, this will be filled to map nodes from + /// this map to nodes in the returned graph. + /// /// \returns The trimmed graph std::unique_ptr - trim(ArrayRef Nodes, - InterExplodedGraphMap *ForwardMap = nullptr, - InterExplodedGraphMap *InverseMap = nullptr) const; + trim(TrimGraphWorklist &Worklist, + InterExplodedGraphMap *NodeMap = nullptr) const; + + std::unique_ptr + trim(ArrayRef Nodes, + InterExplodedGraphMap *NodeMap = nullptr) const { + TrimGraphWorklist Worklist{Nodes}; + return trim(Worklist, NodeMap); + } /// Enable tracking of recently allocated nodes for potential reclamation /// when calling reclaimRecentlyAllocatedNodes(). diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp index d5bc3ac2962d5..3b22b22be741d 100644 --- a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp @@ -2171,6 +2171,7 @@ PathSensitiveBugReport::PathSensitiveBugReport( : BugReport(Kind::PathSensitive, bt, shortDesc, desc), ErrorNode(errorNode), ErrorNodeRange(getStmt() ? getStmt()->getSourceRange() : SourceRange()), UniqueingLocation(LocationToUnique), UniqueingDecl(DeclToUnique) { + assert(ErrorNode && "The error node must not be null!"); assert(!isDependency(ErrorNode->getState() ->getAnalysisManager() .getCheckerManager() @@ -2627,27 +2628,27 @@ class BugPathGetter { } // namespace BugPathGetter::BugPathGetter(const ExplodedGraph *OriginalGraph, - ArrayRef &bugReports) { - SmallVector Nodes; - for (const auto I : bugReports) { - assert(I->isValid() && + ArrayRef &BugReports) { + TrimGraphWorklist Worklist; + for (PathSensitiveBugReport *BR : BugReports) { + assert(BR->isValid() && "We only allow BugReporterVisitors and BugReporter itself to " "invalidate reports!"); - Nodes.emplace_back(I->getErrorNode()); + Worklist.emplace_back(BR->getErrorNode()); } // The trimmed graph is created in the body of the constructor to ensure // that the DenseMaps have been initialized already. - InterExplodedGraphMap ForwardMap; - TrimmedGraph = OriginalGraph->trim(Nodes, &ForwardMap); + InterExplodedGraphMap NodeMap; + TrimmedGraph = OriginalGraph->trim(Worklist, &NodeMap); // Find the (first) error node in the trimmed graph. We just need to consult // the node map which maps from nodes in the original graph to nodes // in the new graph. llvm::SmallPtrSet RemainingNodes; - for (PathSensitiveBugReport *Report : bugReports) { - const ExplodedNode *NewNode = ForwardMap.lookup(Report->getErrorNode()); + for (PathSensitiveBugReport *Report : BugReports) { + const ExplodedNode *NewNode = NodeMap.lookup(Report->getErrorNode()); assert(NewNode && "Failed to construct a trimmed graph that contains this error " "node!"); diff --git a/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp b/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp index 098922d94061f..cef1f0051eed9 100644 --- a/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp @@ -439,63 +439,21 @@ ExplodedNode *ExplodedGraph::createUncachedNode(const ProgramPoint &L, } std::unique_ptr -ExplodedGraph::trim(ArrayRef Sinks, - InterExplodedGraphMap *ForwardMap, - InterExplodedGraphMap *InverseMap) const { - // FIXME: The two-pass algorithm of this function (which was introduced in - // 2008) is terribly overcomplicated and should be replaced by a single - // (backward) pass. - - if (Nodes.empty()) +ExplodedGraph::trim(TrimGraphWorklist &Worklist, + InterExplodedGraphMap *NodeMap) const { + if (Worklist.empty()) return nullptr; - using Pass1Ty = llvm::DenseSet; - Pass1Ty Pass1; + std::unique_ptr Trimmed = std::make_unique(); - using Pass2Ty = InterExplodedGraphMap; - InterExplodedGraphMap Pass2Scratch; - Pass2Ty &Pass2 = ForwardMap ? *ForwardMap : Pass2Scratch; + InterExplodedGraphMap Scratch; + if (!NodeMap) + NodeMap = &Scratch; - SmallVector WL1, WL2; + while (!Worklist.empty()) { + const ExplodedNode *N = Worklist.pop_back_val(); - // ===- Pass 1 (reverse DFS) -=== - for (const auto Sink : Sinks) - if (Sink) - WL1.push_back(Sink); - - // Process the first worklist until it is empty. - while (!WL1.empty()) { - const ExplodedNode *N = WL1.pop_back_val(); - - // Have we already visited this node? If so, continue to the next one. - if (!Pass1.insert(N).second) - continue; - - // If this is the root enqueue it to the second worklist. - if (N->Preds.empty()) { - assert(N == getRoot() && "Found non-root node with no predecessors!"); - WL2.push_back(N); - continue; - } - - // Visit our predecessors and enqueue them. - WL1.append(N->Preds.begin(), N->Preds.end()); - } - - // We didn't hit the root? Return with a null pointer for the new graph. - if (WL2.empty()) - return nullptr; - - assert(WL2.size() == 1 && "There must be only one root!"); - - // Create an empty graph. - std::unique_ptr G = std::make_unique(); - - // ===- Pass 2 (forward DFS to construct the new graph) -=== - while (!WL2.empty()) { - const ExplodedNode *N = WL2.pop_back_val(); - - auto [Place, Inserted] = Pass2.try_emplace(N); + auto [Place, Inserted] = NodeMap->try_emplace(N); // Skip this node if we have already processed it. if (!Inserted) @@ -503,48 +461,37 @@ ExplodedGraph::trim(ArrayRef Sinks, // Create the corresponding node in the new graph and record the mapping // from the old node to the new node. - ExplodedNode *NewN = G->createUncachedNode(N->getLocation(), N->State, - N->getID(), N->isSink()); + ExplodedNode *NewN = Trimmed->createUncachedNode(N->getLocation(), N->State, + N->getID(), N->isSink()); Place->second = NewN; - // Also record the reverse mapping from the new node to the old node. - if (InverseMap) (*InverseMap)[NewN] = N; - - // If this node is the root, designate it as such in the graph. - if (N->Preds.empty()) { - assert(N == getRoot()); - G->designateAsRoot(NewN); - } - - // In the case that some of the intended predecessors of NewN have already - // been created, we should hook them up as predecessors. + // If this is the root node, designate is as the root in the trimmed graph + // as well. + if (N == getRoot()) + Trimmed->designateAsRoot(NewN); - // Walk through the predecessors of 'N' and hook up their corresponding - // nodes in the new graph (if any) to the freshly created node. + // Iterate over the predecessors of the node: if they are already present + // in the trimmed graph, then add the corresponding edges with + // `addPredecessor()`, otherwise add them to the worklist. for (const ExplodedNode *Pred : N->Preds) { - Pass2Ty::iterator PI = Pass2.find(Pred); - if (PI == Pass2.end()) - continue; - - NewN->addPredecessor(const_cast(PI->second), *G); + if (ExplodedNode *Mapped = NodeMap->lookup(Pred)) + NewN->addPredecessor(Mapped, *Trimmed); + else + Worklist.push_back(Pred); } - // In the case that some of the intended successors of NewN have already - // been created, we should hook them up as successors. Otherwise, enqueue - // the new nodes from the original graph that should have nodes created - // in the new graph. - for (const ExplodedNode *Succ : N->Succs) { - Pass2Ty::iterator PI = Pass2.find(Succ); - if (PI != Pass2.end()) { - const_cast(PI->second)->addPredecessor(NewN, *G); - continue; - } - - // Enqueue nodes to the worklist that were marked during pass 1. - if (Pass1.count(Succ)) - WL2.push_back(Succ); - } + // Iterate over the successors of the node: if they are present in the + // trimmed graph, then add the corresponding edges with `addPredecessor()`. + // (If they are not present in the trimmed graph, we don't add them to the + // worklist. Maybe we'll reach them through a different direction, maybe + // they will be omitted from the trimmed graph.) + for (const ExplodedNode *Succ : N->Succs) + if (ExplodedNode *Mapped = NodeMap->lookup(Succ)) + Mapped->addPredecessor(NewN, *Trimmed); } - return G; + assert(Trimmed->getRoot() && + "The root must be reachable from any nonempty set of sinks!"); + + return Trimmed; }