[WIP][analyzer] Refactor ExplodedGraph::trim()

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.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h

@@ -298,7 +298,9 @@ class ExplodedNode : public llvm::FoldingSetNode {
 };
 
 using InterExplodedGraphMap =
-    llvm::DenseMap<const ExplodedNode *, const ExplodedNode *>;
+    llvm::DenseMap<const ExplodedNode *, ExplodedNode *>;
+
+using TrimGraphWorklist = SmallVector<const ExplodedNode *, 32>;
 
 class ExplodedGraph {
 protected:
@@ -406,22 +408,21 @@ class ExplodedGraph {
   llvm::BumpPtrAllocator & getAllocator() { return BVC.getAllocator(); }
   BumpVectorContext &getNodeAllocator() { return BVC; }
 
-  using NodeMap = llvm::DenseMap<const ExplodedNode *, ExplodedNode *>;
-
   /// 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<ExplodedGraph>
-  trim(ArrayRef<const NodeTy *> Nodes,
-       InterExplodedGraphMap *ForwardMap = nullptr,
-       InterExplodedGraphMap *InverseMap = nullptr) const;
+  trim(TrimGraphWorklist &Worklist,
+       InterExplodedGraphMap *NodeMap = nullptr) const;
 
   /// Enable tracking of recently allocated nodes for potential reclamation
   /// when calling reclaimRecentlyAllocatedNodes().

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<PathSensitiveBugReport *> &bugReports) {
-  SmallVector<const ExplodedNode *, 32> Nodes;
-  for (const auto I : bugReports) {
-    assert(I->isValid() &&
+                             ArrayRef<PathSensitiveBugReport *> &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<const ExplodedNode *, 32> 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!");

clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp

@@ -439,112 +439,59 @@ ExplodedNode *ExplodedGraph::createUncachedNode(const ProgramPoint &L,
 }
 
 std::unique_ptr<ExplodedGraph>
-ExplodedGraph::trim(ArrayRef<const NodeTy *> 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<const ExplodedNode *>;
-  Pass1Ty Pass1;
+  std::unique_ptr<ExplodedGraph> Trimmed = std::make_unique<ExplodedGraph>();
 
-  using Pass2Ty = InterExplodedGraphMap;
-  InterExplodedGraphMap Pass2Scratch;
-  Pass2Ty &Pass2 = ForwardMap ? *ForwardMap : Pass2Scratch;
+  InterExplodedGraphMap Scratch;
+  if (!NodeMap)
+    NodeMap = &Scratch;
 
-  SmallVector<const ExplodedNode*, 10> 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<ExplodedGraph> G = std::make_unique<ExplodedGraph>();
-
-  // ===- 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)
       continue;
 
     // 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<ExplodedNode *>(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<ExplodedNode *>(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;
 }

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

@@ -4096,7 +4096,8 @@ std::string ExprEngine::DumpGraph(bool trim, StringRef Filename) {
 
 std::string ExprEngine::DumpGraph(ArrayRef<const ExplodedNode *> Nodes,
                                   StringRef Filename) {
-  std::unique_ptr<ExplodedGraph> TrimmedG(G.trim(Nodes));
+  TrimGraphWorklist Worklist{Nodes};
+  std::unique_ptr<ExplodedGraph> TrimmedG(G.trim(Worklist));
 
   if (!TrimmedG) {
     llvm::errs() << "warning: Trimmed ExplodedGraph is empty.\n";