[clang] [WIP][analyzer] Refactor `ExplodedGraph::trim()` (PR #139939)
Donát Nagy via cfe-commits
cfe-commits at lists.llvm.org
Thu May 15 06:30:24 PDT 2025
https://github.com/NagyDonat updated https://github.com/llvm/llvm-project/pull/139939
>From 8a33087fcd94d326cac602a6be83a1b34b43e1ab Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Wed, 14 May 2025 19:24:37 +0200
Subject: [PATCH 01/10] [WIP][analyzer] Refactor `ExplodedGraph::trim()`
...because its implementation was using a complicated two-pass graph
traversal while in fact one pass is sufficient for performing that
trimming operation.
Note that this commit changes the interface of `trim`: it no longer
accepts nullpointers within the array `Sinks` and `BugReporter.cpp` (the
single non-debug caller) was modified accordingly. This also affects the
interface of `DumpGraph` (which calls `trim` under the hood), but that's
a debug helper which is only called if some developer compiles a tweaked
version of the analyzer for local debugging and it is unlikely that a
developer would pass nullpointers to it.
WORK IN PROGRESS -- DO NOT MERGE!
After this commit the analyzer will pick different representants from
the bug report equivalence classes (because some iteration order
changes), which breaks lots of testcases.
---
clang/lib/StaticAnalyzer/Core/BugReporter.cpp | 3 +-
.../lib/StaticAnalyzer/Core/ExplodedGraph.cpp | 110 ++++++------------
2 files changed, 35 insertions(+), 78 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
index d5bc3ac2962d5..709fb18f14789 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -2633,7 +2633,8 @@ BugPathGetter::BugPathGetter(const ExplodedGraph *OriginalGraph,
assert(I->isValid() &&
"We only allow BugReporterVisitors and BugReporter itself to "
"invalidate reports!");
- Nodes.emplace_back(I->getErrorNode());
+ if (const ExplodedNode *ErrNode = I->getErrorNode())
+ Nodes.emplace_back(ErrNode);
}
// The trimmed graph is created in the body of the constructor to ensure
diff --git a/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp b/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
index 098922d94061f..85f84af4167e0 100644
--- a/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
@@ -442,60 +442,21 @@ 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())
+ if (Sinks.empty())
return nullptr;
- using Pass1Ty = llvm::DenseSet<const ExplodedNode *>;
- Pass1Ty Pass1;
-
- using Pass2Ty = InterExplodedGraphMap;
- InterExplodedGraphMap Pass2Scratch;
- Pass2Ty &Pass2 = ForwardMap ? *ForwardMap : Pass2Scratch;
-
- SmallVector<const ExplodedNode*, 10> WL1, WL2;
-
- // ===- 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());
- }
+ std::unique_ptr<ExplodedGraph> Trimmed = std::make_unique<ExplodedGraph>();
- // We didn't hit the root? Return with a null pointer for the new graph.
- if (WL2.empty())
- return nullptr;
+ SmallVector<const ExplodedNode*, 32> Worklist{Sinks};
- assert(WL2.size() == 1 && "There must be only one root!");
+ InterExplodedGraphMap Scratch;
+ if (!ForwardMap)
+ ForwardMap = &Scratch;
- // Create an empty graph.
- std::unique_ptr<ExplodedGraph> G = std::make_unique<ExplodedGraph>();
+ while (!Worklist.empty()) {
+ const ExplodedNode *N = Worklist.pop_back_val();
- // ===- 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] = ForwardMap->try_emplace(N);
// Skip this node if we have already processed it.
if (!Inserted)
@@ -503,48 +464,43 @@ ExplodedGraph::trim(ArrayRef<const NodeTy *> 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,
+ 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);
+ auto Iterator = ForwardMap->find(Pred);
+ if (Iterator != ForwardMap->end()) {
+ NewN->addPredecessor(const_cast<ExplodedNode *>(Iterator->second), *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.
+ // 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) {
- Pass2Ty::iterator PI = Pass2.find(Succ);
- if (PI != Pass2.end()) {
- const_cast<ExplodedNode *>(PI->second)->addPredecessor(NewN, *G);
- continue;
+ auto Iterator = ForwardMap->find(Succ);
+ if (Iterator != ForwardMap->end()) {
+ const_cast<ExplodedNode *>(Iterator->second)->addPredecessor(NewN, *Trimmed);
}
-
- // Enqueue nodes to the worklist that were marked during pass 1.
- if (Pass1.count(Succ))
- WL2.push_back(Succ);
}
}
- return G;
+ assert(Trimmed->getRoot() && "The root must be reachable from any nonempty set of sinks!");
+
+ return Trimmed;
}
>From 348e740362e6f2bd9e747affa26e3a5bf826c4d9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Thu, 15 May 2025 13:57:48 +0200
Subject: [PATCH 02/10] Simplify map lookup; unbrace
---
clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp b/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
index 85f84af4167e0..121f1a5eadbe4 100644
--- a/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
@@ -479,12 +479,10 @@ ExplodedGraph::trim(ArrayRef<const NodeTy *> Sinks,
// in the trimmed graph, then add the corresponding edges with
// `addPredecessor()`, otherwise add them to the worklist.
for (const ExplodedNode *Pred : N->Preds) {
- auto Iterator = ForwardMap->find(Pred);
- if (Iterator != ForwardMap->end()) {
- NewN->addPredecessor(const_cast<ExplodedNode *>(Iterator->second), *Trimmed);
- } else {
+ if (const ExplodedNode *Mapped = ForwardMap->lookup(Pred))
+ NewN->addPredecessor(const_cast<ExplodedNode *>(Mapped), *Trimmed);
+ else
Worklist.push_back(Pred);
- }
}
// Iterate over the successors of the node: if they are present in the
@@ -492,12 +490,9 @@ ExplodedGraph::trim(ArrayRef<const NodeTy *> Sinks,
// (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) {
- auto Iterator = ForwardMap->find(Succ);
- if (Iterator != ForwardMap->end()) {
- const_cast<ExplodedNode *>(Iterator->second)->addPredecessor(NewN, *Trimmed);
- }
- }
+ for (const ExplodedNode *Succ : N->Succs)
+ if (const ExplodedNode *Mapped = ForwardMap->lookup(Succ))
+ const_cast<ExplodedNode *>(Mapped)->addPredecessor(NewN, *Trimmed);
}
assert(Trimmed->getRoot() && "The root must be reachable from any nonempty set of sinks!");
>From 300b7ce25a45ca1cd95a000de6c0568c6393e6c2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Thu, 15 May 2025 14:03:35 +0200
Subject: [PATCH 03/10] Get rid of InverseMap because it was unused
---
.../Core/PathSensitive/ExplodedGraph.h | 7 ++-----
clang/lib/StaticAnalyzer/Core/BugReporter.cpp | 6 +++---
clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp | 16 ++++++----------
3 files changed, 11 insertions(+), 18 deletions(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
index e995151927c96..cf913c064933e 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
@@ -413,15 +413,12 @@ class ExplodedGraph {
///
/// \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
+ /// \param[out] NodeMap 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.
/// \returns The trimmed graph
std::unique_ptr<ExplodedGraph>
trim(ArrayRef<const NodeTy *> Nodes,
- InterExplodedGraphMap *ForwardMap = nullptr,
- InterExplodedGraphMap *InverseMap = nullptr) const;
+ InterExplodedGraphMap *NodeMap = nullptr) const;
/// 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 709fb18f14789..188de7da50c62 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -2639,8 +2639,8 @@ BugPathGetter::BugPathGetter(const ExplodedGraph *OriginalGraph,
// 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(Nodes, &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
@@ -2648,7 +2648,7 @@ BugPathGetter::BugPathGetter(const ExplodedGraph *OriginalGraph,
llvm::SmallPtrSet<const ExplodedNode *, 32> RemainingNodes;
for (PathSensitiveBugReport *Report : bugReports) {
- const ExplodedNode *NewNode = ForwardMap.lookup(Report->getErrorNode());
+ 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 121f1a5eadbe4..7a13dbe5961b8 100644
--- a/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
@@ -440,8 +440,7 @@ ExplodedNode *ExplodedGraph::createUncachedNode(const ProgramPoint &L,
std::unique_ptr<ExplodedGraph>
ExplodedGraph::trim(ArrayRef<const NodeTy *> Sinks,
- InterExplodedGraphMap *ForwardMap,
- InterExplodedGraphMap *InverseMap) const {
+ InterExplodedGraphMap *NodeMap) const {
if (Sinks.empty())
return nullptr;
@@ -450,13 +449,13 @@ ExplodedGraph::trim(ArrayRef<const NodeTy *> Sinks,
SmallVector<const ExplodedNode*, 32> Worklist{Sinks};
InterExplodedGraphMap Scratch;
- if (!ForwardMap)
- ForwardMap = &Scratch;
+ if (!NodeMap)
+ NodeMap = &Scratch;
while (!Worklist.empty()) {
const ExplodedNode *N = Worklist.pop_back_val();
- auto [Place, Inserted] = ForwardMap->try_emplace(N);
+ auto [Place, Inserted] = NodeMap->try_emplace(N);
// Skip this node if we have already processed it.
if (!Inserted)
@@ -468,9 +467,6 @@ ExplodedGraph::trim(ArrayRef<const NodeTy *> Sinks,
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 is the root node, designate is as the root in the trimmed graph as well.
if (N == getRoot())
Trimmed->designateAsRoot(NewN);
@@ -479,7 +475,7 @@ ExplodedGraph::trim(ArrayRef<const NodeTy *> Sinks,
// in the trimmed graph, then add the corresponding edges with
// `addPredecessor()`, otherwise add them to the worklist.
for (const ExplodedNode *Pred : N->Preds) {
- if (const ExplodedNode *Mapped = ForwardMap->lookup(Pred))
+ if (const ExplodedNode *Mapped = NodeMap->lookup(Pred))
NewN->addPredecessor(const_cast<ExplodedNode *>(Mapped), *Trimmed);
else
Worklist.push_back(Pred);
@@ -491,7 +487,7 @@ ExplodedGraph::trim(ArrayRef<const NodeTy *> Sinks,
// 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 (const ExplodedNode *Mapped = ForwardMap->lookup(Succ))
+ if (const ExplodedNode *Mapped = NodeMap->lookup(Succ))
const_cast<ExplodedNode *>(Mapped)->addPredecessor(NewN, *Trimmed);
}
>From 7ea7d8330e4ac8b8d637336395342deb574e4f6f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Thu, 15 May 2025 14:07:07 +0200
Subject: [PATCH 04/10] Eliminate const_cast by using the proper type
---
.../StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h | 2 +-
clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp | 8 ++++----
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
index cf913c064933e..7fb1b8dc45bfd 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
@@ -298,7 +298,7 @@ class ExplodedNode : public llvm::FoldingSetNode {
};
using InterExplodedGraphMap =
- llvm::DenseMap<const ExplodedNode *, const ExplodedNode *>;
+ llvm::DenseMap<const ExplodedNode *, ExplodedNode *>;
class ExplodedGraph {
protected:
diff --git a/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp b/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
index 7a13dbe5961b8..ee76271e2eb5d 100644
--- a/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
@@ -475,8 +475,8 @@ ExplodedGraph::trim(ArrayRef<const NodeTy *> Sinks,
// in the trimmed graph, then add the corresponding edges with
// `addPredecessor()`, otherwise add them to the worklist.
for (const ExplodedNode *Pred : N->Preds) {
- if (const ExplodedNode *Mapped = NodeMap->lookup(Pred))
- NewN->addPredecessor(const_cast<ExplodedNode *>(Mapped), *Trimmed);
+ if (ExplodedNode *Mapped = NodeMap->lookup(Pred))
+ NewN->addPredecessor(Mapped, *Trimmed);
else
Worklist.push_back(Pred);
}
@@ -487,8 +487,8 @@ ExplodedGraph::trim(ArrayRef<const NodeTy *> Sinks,
// 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 (const ExplodedNode *Mapped = NodeMap->lookup(Succ))
- const_cast<ExplodedNode *>(Mapped)->addPredecessor(NewN, *Trimmed);
+ if (ExplodedNode *Mapped = NodeMap->lookup(Succ))
+ Mapped->addPredecessor(NewN, *Trimmed);
}
assert(Trimmed->getRoot() && "The root must be reachable from any nonempty set of sinks!");
>From c2a6891a417c854b2679236707a6a49f3a487b1a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Thu, 15 May 2025 14:59:50 +0200
Subject: [PATCH 05/10] Avoid copying a vector of node pointers
---
.../Core/PathSensitive/ExplodedGraph.h | 17 ++++++++++++-----
clang/lib/StaticAnalyzer/Core/BugReporter.cpp | 6 +++---
clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp | 6 ++----
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 3 ++-
4 files changed, 19 insertions(+), 13 deletions(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
index 7fb1b8dc45bfd..911d9d350acab 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
@@ -300,6 +300,9 @@ class ExplodedNode : public llvm::FoldingSetNode {
using InterExplodedGraphMap =
llvm::DenseMap<const ExplodedNode *, ExplodedNode *>;
+using TrimGraphWorklist =
+ SmallVector<const ExplodedNode*, 32>;
+
class ExplodedGraph {
protected:
friend class CoreEngine;
@@ -411,13 +414,17 @@ class ExplodedGraph {
/// 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] NodeMap An optional map from nodes in this graph to nodes
- /// in the returned 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,
+ trim(TrimGraphWorklist &Worklist,
InterExplodedGraphMap *NodeMap = nullptr) const;
/// Enable tracking of recently allocated nodes for potential reclamation
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
index 188de7da50c62..67972b2e8ec2c 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -2628,19 +2628,19 @@ class BugPathGetter {
BugPathGetter::BugPathGetter(const ExplodedGraph *OriginalGraph,
ArrayRef<PathSensitiveBugReport *> &bugReports) {
- SmallVector<const ExplodedNode *, 32> Nodes;
+ TrimGraphWorklist Worklist;
for (const auto I : bugReports) {
assert(I->isValid() &&
"We only allow BugReporterVisitors and BugReporter itself to "
"invalidate reports!");
if (const ExplodedNode *ErrNode = I->getErrorNode())
- Nodes.emplace_back(ErrNode);
+ Worklist.emplace_back(ErrNode);
}
// The trimmed graph is created in the body of the constructor to ensure
// that the DenseMaps have been initialized already.
InterExplodedGraphMap NodeMap;
- TrimmedGraph = OriginalGraph->trim(Nodes, &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
diff --git a/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp b/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
index ee76271e2eb5d..cbf160a83ab2a 100644
--- a/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
@@ -439,15 +439,13 @@ ExplodedNode *ExplodedGraph::createUncachedNode(const ProgramPoint &L,
}
std::unique_ptr<ExplodedGraph>
-ExplodedGraph::trim(ArrayRef<const NodeTy *> Sinks,
+ExplodedGraph::trim(TrimGraphWorklist &Worklist,
InterExplodedGraphMap *NodeMap) const {
- if (Sinks.empty())
+ if (Worklist.empty())
return nullptr;
std::unique_ptr<ExplodedGraph> Trimmed = std::make_unique<ExplodedGraph>();
- SmallVector<const ExplodedNode*, 32> Worklist{Sinks};
-
InterExplodedGraphMap Scratch;
if (!NodeMap)
NodeMap = &Scratch;
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index ebad83dad0c8f..6a7069954c731 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/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";
>From a8ced536abc7d31bf4479246e87e2a9f550d9f9a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Thu, 15 May 2025 15:03:09 +0200
Subject: [PATCH 06/10] Delete an unused type alias
---
.../clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
index 911d9d350acab..0e30a42800422 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
@@ -409,8 +409,6 @@ 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.
///
>From 42d30762c3dbb18914eaed9398eb28d28cedde7e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Thu, 15 May 2025 15:16:18 +0200
Subject: [PATCH 07/10] Capitalize the parameter 'bugReports'
---
clang/lib/StaticAnalyzer/Core/BugReporter.cpp | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
index 67972b2e8ec2c..f080516574db8 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -2627,9 +2627,9 @@ class BugPathGetter {
} // namespace
BugPathGetter::BugPathGetter(const ExplodedGraph *OriginalGraph,
- ArrayRef<PathSensitiveBugReport *> &bugReports) {
+ ArrayRef<PathSensitiveBugReport *> &BugReports) {
TrimGraphWorklist Worklist;
- for (const auto I : bugReports) {
+ for (const auto I : BugReports) {
assert(I->isValid() &&
"We only allow BugReporterVisitors and BugReporter itself to "
"invalidate reports!");
@@ -2647,7 +2647,7 @@ BugPathGetter::BugPathGetter(const ExplodedGraph *OriginalGraph,
// in the new graph.
llvm::SmallPtrSet<const ExplodedNode *, 32> RemainingNodes;
- for (PathSensitiveBugReport *Report : bugReports) {
+ for (PathSensitiveBugReport *Report : BugReports) {
const ExplodedNode *NewNode = NodeMap.lookup(Report->getErrorNode());
assert(NewNode &&
"Failed to construct a trimmed graph that contains this error "
>From f6b4ef520af22f8437b8b6423e68dcfae81c8690 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Thu, 15 May 2025 15:23:30 +0200
Subject: [PATCH 08/10] Clarify that PathSensitiveBugReport::ErrorNode is never
null
The declaration of this data member was specifying `nullptr` as an
initializer, but each constructor of the class initializes it to a
non-null value (which is taken as an argument) and nothing changes its
value later.
---
.../clang/StaticAnalyzer/Core/BugReporter/BugReporter.h | 5 +++--
clang/lib/StaticAnalyzer/Core/BugReporter.cpp | 4 ++--
2 files changed, 5 insertions(+), 4 deletions(-)
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/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
index f080516574db8..ef2bcdb79561c 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()
@@ -2633,8 +2634,7 @@ BugPathGetter::BugPathGetter(const ExplodedGraph *OriginalGraph,
assert(I->isValid() &&
"We only allow BugReporterVisitors and BugReporter itself to "
"invalidate reports!");
- if (const ExplodedNode *ErrNode = I->getErrorNode())
- Worklist.emplace_back(ErrNode);
+ Worklist.emplace_back(I->getErrorNode());
}
// The trimmed graph is created in the body of the constructor to ensure
>From fde270e59862924f2ce282323ce93e83d9cd4ff6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Thu, 15 May 2025 15:28:25 +0200
Subject: [PATCH 09/10] Fix misleading declaration of a loop variable
It looks like an iterator, let's clarify its type and use a better name.
---
clang/lib/StaticAnalyzer/Core/BugReporter.cpp | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
index ef2bcdb79561c..3b22b22be741d 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -2630,11 +2630,11 @@ class BugPathGetter {
BugPathGetter::BugPathGetter(const ExplodedGraph *OriginalGraph,
ArrayRef<PathSensitiveBugReport *> &BugReports) {
TrimGraphWorklist Worklist;
- for (const auto I : BugReports) {
- assert(I->isValid() &&
+ for (PathSensitiveBugReport *BR : BugReports) {
+ assert(BR->isValid() &&
"We only allow BugReporterVisitors and BugReporter itself to "
"invalidate reports!");
- Worklist.emplace_back(I->getErrorNode());
+ Worklist.emplace_back(BR->getErrorNode());
}
// The trimmed graph is created in the body of the constructor to ensure
>From 64b117aa17e4546e58edd49d139073939a080c5c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Thu, 15 May 2025 15:30:06 +0200
Subject: [PATCH 10/10] Satisfy git-clang-format
---
.../clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
index 0e30a42800422..235afa468692c 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
@@ -300,8 +300,7 @@ class ExplodedNode : public llvm::FoldingSetNode {
using InterExplodedGraphMap =
llvm::DenseMap<const ExplodedNode *, ExplodedNode *>;
-using TrimGraphWorklist =
- SmallVector<const ExplodedNode*, 32>;
+using TrimGraphWorklist = SmallVector<const ExplodedNode *, 32>;
class ExplodedGraph {
protected:
More information about the cfe-commits
mailing list