Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
NagyDonat wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8a33087
[WIP][analyzer] Refactor `ExplodedGraph::trim()`
NagyDonat May 14, 2025
348e740
Simplify map lookup; unbrace
NagyDonat May 15, 2025
300b7ce
Get rid of InverseMap because it was unused
NagyDonat May 15, 2025
7ea7d83
Eliminate const_cast by using the proper type
NagyDonat May 15, 2025
c2a6891
Avoid copying a vector of node pointers
NagyDonat May 15, 2025
a8ced53
Delete an unused type alias
NagyDonat May 15, 2025
42d3076
Capitalize the parameter 'bugReports'
NagyDonat May 15, 2025
f6b4ef5
Clarify that PathSensitiveBugReport::ErrorNode is never null
NagyDonat May 15, 2025
fde270e
Fix misleading declaration of a loop variable
NagyDonat May 15, 2025
64b117a
Satisfy git-clang-format
NagyDonat May 15, 2025
fab536f
Satisfy git-clang-format (really)
NagyDonat May 15, 2025
e01e8bd
Provide 'ExplodedGraph::trim(ArrayRef, ...)'
NagyDonat May 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 14 additions & 12 deletions clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,10 @@ 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:
Expand Down Expand Up @@ -406,22 +409,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().
Expand Down
16 changes: 8 additions & 8 deletions clang/lib/StaticAnalyzer/Core/BugReporter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2627,28 +2627,28 @@ class BugPathGetter {
} // namespace

BugPathGetter::BugPathGetter(const ExplodedGraph *OriginalGraph,
ArrayRef<PathSensitiveBugReport *> &bugReports) {
SmallVector<const ExplodedNode *, 32> Nodes;
for (const auto I : bugReports) {
ArrayRef<PathSensitiveBugReport *> &BugReports) {
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 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!");
Expand Down
26 changes: 10 additions & 16 deletions clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,24 +439,21 @@ ExplodedNode *ExplodedGraph::createUncachedNode(const ProgramPoint &L,
}

std::unique_ptr<ExplodedGraph>
ExplodedGraph::trim(ArrayRef<const NodeTy *> Sinks,
InterExplodedGraphMap *ForwardMap,
InterExplodedGraphMap *InverseMap) const {
if (Sinks.empty())
ExplodedGraph::trim(TrimGraphWorklist &Worklist,
InterExplodedGraphMap *NodeMap) const {
if (Worklist.empty())
return nullptr;

std::unique_ptr<ExplodedGraph> Trimmed = std::make_unique<ExplodedGraph>();

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)
Expand All @@ -468,9 +465,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);
Expand All @@ -479,8 +473,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 = ForwardMap->lookup(Pred))
NewN->addPredecessor(const_cast<ExplodedNode *>(Mapped), *Trimmed);
if (ExplodedNode *Mapped = NodeMap->lookup(Pred))
NewN->addPredecessor(Mapped, *Trimmed);
else
Worklist.push_back(Pred);
}
Expand All @@ -491,8 +485,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 = ForwardMap->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!");
Expand Down
3 changes: 2 additions & 1 deletion clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Contributor

@steakhal steakhal May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is Worklist hoised into a variable?

Copy link
Contributor

@steakhal steakhal May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah because it's now an in-out ref parameter. I don't understand why it needs to be an lvalue.

Copy link
Contributor Author

@NagyDonat NagyDonat May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see... I can just pass Nodes directly to G.trim and the type conversion would force the materialization of an unnamed temporary object, which is exactly the right thing in this situation. I'll remove the named temporary Worklist -- let's enjoy magic when it's convenient.

Copy link
Contributor Author

@NagyDonat NagyDonat May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, does not work right away:

Non-const lvalue reference to type 'clang::ento::TrimGraphWorklist' (aka 'SmallVector<const clang::ento::ExplodedNode *, 32>') cannot bind to a value of unrelated type 'ArrayRef<const clang::ento::ExplodedNode *>' (clang lvalue_reference_bind_to_unrelated)

Copy link
Contributor

@steakhal steakhal May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One should prefer views at parameter boundaries.
You can take an arrayref parameter type, and then inside your function materialize a small vector out of it to use it as a worklist.

Copy link
Contributor Author

@NagyDonat NagyDonat May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed yet another small commit where I provide an "easy-to-call" overload for trim that takes an ArrayRef instead of a worklist: e01e8bd

However I'm also open to reverting the commit where I start to consume the array of nodes if you think that this is premature optimiziation.


if (!TrimmedG) {
llvm::errs() << "warning: Trimmed ExplodedGraph is empty.\n";
Expand Down
Loading