[clang] 1ad9462 - [AST] Use data-recursion when building ParentMap, avoid stack overflow.
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Thu Sep 24 13:50:17 PDT 2020
Author: Sam McCall
Date: 2020-09-24T22:49:44+02:00
New Revision: 1ad94624f8a092fbfcb74685e11243c186b04c8f
URL: https://github.com/llvm/llvm-project/commit/1ad94624f8a092fbfcb74685e11243c186b04c8f
DIFF: https://github.com/llvm/llvm-project/commit/1ad94624f8a092fbfcb74685e11243c186b04c8f.diff
LOG: [AST] Use data-recursion when building ParentMap, avoid stack overflow.
The following crashes on my system before this patch, but not after:
void foo(int i) {
switch (i) {
case 1:
case 2:
... 100000 cases ...
;
}
}
clang-query -c="match stmt(hasAncestor(stmt()))" deep.c
I'm not sure it's actually a sane testcase to run though, it's pretty slow :-)
Differential Revision: https://reviews.llvm.org/D88222
Added:
Modified:
clang/lib/AST/ParentMapContext.cpp
Removed:
################################################################################
diff --git a/clang/lib/AST/ParentMapContext.cpp b/clang/lib/AST/ParentMapContext.cpp
index b73b32774b53..bae4d016ff6b 100644
--- a/clang/lib/AST/ParentMapContext.cpp
+++ b/clang/lib/AST/ParentMapContext.cpp
@@ -211,8 +211,6 @@ DynTypedNode createDynTypedNode(const NestedNameSpecifierLoc &Node) {
/// Note that the relationship described here is purely in terms of AST
/// traversal - there are other relationships (for example declaration context)
/// in the AST that are better modeled by special matchers.
-///
-/// FIXME: Currently only builds up the map using \c Stmt and \c Decl nodes.
class ParentMapContext::ParentMap::ASTVisitor
: public RecursiveASTVisitor<ASTVisitor> {
public:
@@ -227,51 +225,60 @@ class ParentMapContext::ParentMap::ASTVisitor
bool shouldVisitImplicitCode() const { return true; }
+ /// Record the parent of the node we're visiting.
+ /// MapNode is the child, the parent is on top of ParentStack.
+ /// Parents is the parent storage (either PointerParents or OtherParents).
+ template <typename MapNodeTy, typename MapTy>
+ void addParent(MapNodeTy MapNode, MapTy *Parents) {
+ if (ParentStack.empty())
+ return;
+
+ // FIXME: Currently we add the same parent multiple times, but only
+ // when no memoization data is available for the type.
+ // For example when we visit all subexpressions of template
+ // instantiations; this is suboptimal, but benign: the only way to
+ // visit those is with hasAncestor / hasParent, and those do not create
+ // new matches.
+ // The plan is to enable DynTypedNode to be storable in a map or hash
+ // map. The main problem there is to implement hash functions /
+ // comparison operators for all types that DynTypedNode supports that
+ // do not have pointer identity.
+ auto &NodeOrVector = (*Parents)[MapNode];
+ if (NodeOrVector.isNull()) {
+ if (const auto *D = ParentStack.back().get<Decl>())
+ NodeOrVector = D;
+ else if (const auto *S = ParentStack.back().get<Stmt>())
+ NodeOrVector = S;
+ else
+ NodeOrVector = new DynTypedNode(ParentStack.back());
+ } else {
+ if (!NodeOrVector.template is<ParentVector *>()) {
+ auto *Vector = new ParentVector(
+ 1, getSingleDynTypedNodeFromParentMap(NodeOrVector));
+ delete NodeOrVector.template dyn_cast<DynTypedNode *>();
+ NodeOrVector = Vector;
+ }
+
+ auto *Vector = NodeOrVector.template get<ParentVector *>();
+ // Skip duplicates for types that have memoization data.
+ // We must check that the type has memoization data before calling
+ // std::find() because DynTypedNode::operator== can't compare all
+ // types.
+ bool Found = ParentStack.back().getMemoizationData() &&
+ std::find(Vector->begin(), Vector->end(),
+ ParentStack.back()) != Vector->end();
+ if (!Found)
+ Vector->push_back(ParentStack.back());
+ }
+ }
+
template <typename T, typename MapNodeTy, typename BaseTraverseFn,
typename MapTy>
bool TraverseNode(T Node, MapNodeTy MapNode, BaseTraverseFn BaseTraverse,
MapTy *Parents) {
if (!Node)
return true;
- if (ParentStack.size() > 0) {
- // FIXME: Currently we add the same parent multiple times, but only
- // when no memoization data is available for the type.
- // For example when we visit all subexpressions of template
- // instantiations; this is suboptimal, but benign: the only way to
- // visit those is with hasAncestor / hasParent, and those do not create
- // new matches.
- // The plan is to enable DynTypedNode to be storable in a map or hash
- // map. The main problem there is to implement hash functions /
- // comparison operators for all types that DynTypedNode supports that
- // do not have pointer identity.
- auto &NodeOrVector = (*Parents)[MapNode];
- if (NodeOrVector.isNull()) {
- if (const auto *D = ParentStack.back().get<Decl>())
- NodeOrVector = D;
- else if (const auto *S = ParentStack.back().get<Stmt>())
- NodeOrVector = S;
- else
- NodeOrVector = new DynTypedNode(ParentStack.back());
- } else {
- if (!NodeOrVector.template is<ParentVector *>()) {
- auto *Vector = new ParentVector(
- 1, getSingleDynTypedNodeFromParentMap(NodeOrVector));
- delete NodeOrVector.template dyn_cast<DynTypedNode *>();
- NodeOrVector = Vector;
- }
-
- auto *Vector = NodeOrVector.template get<ParentVector *>();
- // Skip duplicates for types that have memoization data.
- // We must check that the type has memoization data before calling
- // std::find() because DynTypedNode::operator== can't compare all
- // types.
- bool Found = ParentStack.back().getMemoizationData() &&
- std::find(Vector->begin(), Vector->end(),
- ParentStack.back()) != Vector->end();
- if (!Found)
- Vector->push_back(ParentStack.back());
- }
- }
+ addParent(MapNode, Parents);
ParentStack.push_back(createDynTypedNode(Node));
bool Result = BaseTraverse();
ParentStack.pop_back();
@@ -283,20 +290,12 @@ class ParentMapContext::ParentMap::ASTVisitor
DeclNode, DeclNode, [&] { return VisitorBase::TraverseDecl(DeclNode); },
&Map.PointerParents);
}
-
- bool TraverseStmt(Stmt *StmtNode) {
- return TraverseNode(StmtNode, StmtNode,
- [&] { return VisitorBase::TraverseStmt(StmtNode); },
- &Map.PointerParents);
- }
-
bool TraverseTypeLoc(TypeLoc TypeLocNode) {
return TraverseNode(
TypeLocNode, DynTypedNode::create(TypeLocNode),
[&] { return VisitorBase::TraverseTypeLoc(TypeLocNode); },
&Map.OtherParents);
}
-
bool TraverseNestedNameSpecifierLoc(NestedNameSpecifierLoc NNSLocNode) {
return TraverseNode(
NNSLocNode, DynTypedNode::create(NNSLocNode),
@@ -304,6 +303,17 @@ class ParentMapContext::ParentMap::ASTVisitor
&Map.OtherParents);
}
+ // Using generic TraverseNode for Stmt would prevent data-recursion.
+ bool dataTraverseStmtPre(Stmt *StmtNode) {
+ addParent(StmtNode, &Map.PointerParents);
+ ParentStack.push_back(DynTypedNode::create(*StmtNode));
+ return true;
+ }
+ bool dataTraverseStmtPost(Stmt *StmtNode) {
+ ParentStack.pop_back();
+ return true;
+ }
+
ParentMap ⤅
llvm::SmallVector<DynTypedNode, 16> ParentStack;
};
More information about the cfe-commits
mailing list