[clang] [ASTMatchers] Fix classIsDerivedFrom for recusrive cases (PR #67307)
Ilya Biryukov via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 25 03:08:17 PDT 2023
https://github.com/ilya-biryukov updated https://github.com/llvm/llvm-project/pull/67307
>From bd300e3bf41df7cc90e4d4acf5e46da9847be46a Mon Sep 17 00:00:00 2001
From: Ilya Biryukov <ibiryukov at google.com>
Date: Mon, 25 Sep 2023 11:46:03 +0200
Subject: [PATCH 1/2] [ASTMatchers] Fix classIsDerivedFrom for recusrive cases
By ensuring the base is only visited once. This avoids infinite
recursion and expontential running times in some corner cases.
See the added tests for examples.
Apart from the cases that caused infinite recursion and used to crash,
this change is an NFC and results of the matchers are the same.
---
clang/lib/ASTMatchers/ASTMatchFinder.cpp | 24 ++++++-
.../ASTMatchers/ASTMatchersNodeTest.cpp | 68 +++++++++++++++++++
2 files changed, 91 insertions(+), 1 deletion(-)
diff --git a/clang/lib/ASTMatchers/ASTMatchFinder.cpp b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
index f9bd1354fa8dc44..0bac2ed63a927ef 100644
--- a/clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -18,8 +18,10 @@
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/AST/ASTConsumer.h"
#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclCXX.h"
#include "clang/AST/RecursiveASTVisitor.h"
#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/Support/PrettyStackTrace.h"
#include "llvm/Support/Timer.h"
@@ -651,11 +653,20 @@ class MatchASTVisitor : public RecursiveASTVisitor<MatchASTVisitor>,
BoundNodesTreeBuilder *Builder,
bool Directly) override;
+private:
+ bool
+ classIsDerivedFromImpl(const CXXRecordDecl *Declaration,
+ const Matcher<NamedDecl> &Base,
+ BoundNodesTreeBuilder *Builder, bool Directly,
+ llvm::SmallPtrSetImpl<const CXXRecordDecl *> &Visited);
+
+public:
bool objcClassIsDerivedFrom(const ObjCInterfaceDecl *Declaration,
const Matcher<NamedDecl> &Base,
BoundNodesTreeBuilder *Builder,
bool Directly) override;
+public:
// Implements ASTMatchFinder::matchesChildOf.
bool matchesChildOf(const DynTypedNode &Node, ASTContext &Ctx,
const DynTypedMatcher &Matcher,
@@ -1361,8 +1372,18 @@ bool MatchASTVisitor::classIsDerivedFrom(const CXXRecordDecl *Declaration,
const Matcher<NamedDecl> &Base,
BoundNodesTreeBuilder *Builder,
bool Directly) {
+ llvm::SmallPtrSet<const CXXRecordDecl *, 8> Visited;
+ return classIsDerivedFromImpl(Declaration, Base, Builder, Directly, Visited);
+}
+
+bool MatchASTVisitor::classIsDerivedFromImpl(
+ const CXXRecordDecl *Declaration, const Matcher<NamedDecl> &Base,
+ BoundNodesTreeBuilder *Builder, bool Directly,
+ llvm::SmallPtrSetImpl<const CXXRecordDecl *> &Visited) {
if (!Declaration->hasDefinition())
return false;
+ if (!Visited.insert(Declaration).second)
+ return false;
for (const auto &It : Declaration->bases()) {
const Type *TypeNode = It.getType().getTypePtr();
@@ -1384,7 +1405,8 @@ bool MatchASTVisitor::classIsDerivedFrom(const CXXRecordDecl *Declaration,
*Builder = std::move(Result);
return true;
}
- if (!Directly && classIsDerivedFrom(ClassDecl, Base, Builder, Directly))
+ if (!Directly &&
+ classIsDerivedFromImpl(ClassDecl, Base, Builder, Directly, Visited))
return true;
}
return false;
diff --git a/clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp b/clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
index 7a6d6ef0a95554b..0a08a8b4c491dca 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ b/clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -2369,6 +2369,74 @@ TEST_P(ASTMatchersTest, LambdaCaptureTest_BindsToCaptureOfReferenceType) {
"}", matcher));
}
+TEST_P(ASTMatchersTest, IsDerivedFromRecursion) {
+ if (!GetParam().isCXX11OrLater())
+ return;
+
+ // Check we don't crash on cycles in the traversal and inheritance hierarchy.
+ // Clang will normally enforce there are no cycles, but matchers opted to
+ // traverse primary template for dependent specializations, spuriously
+ // creating the cycles.
+ DeclarationMatcher matcher = cxxRecordDecl(isDerivedFrom("X"));
+ EXPECT_TRUE(notMatches(R"cpp(
+ template <typename T1, typename T2 = void, typename T3 = void, typename
+ T4 = void> struct M; template <typename T1, typename T2> struct M<T1,
+ T2> {}; template <typename T1, typename T2> struct L : M<T1, T2> {};
+ template <typename T1, typename T2, typename T3, typename T4>
+ struct M : L<M<T1, T2>, M<T3, T4>> {};
+ )cpp",
+ matcher));
+
+ // Check the running time is not exponential. The number of subojects to
+ // traverse grows as fibonacci numbers even though the number of bases to
+ // traverse is quadratic.
+ // The test will hang if implementation of matchers traverses all subojects.
+ EXPECT_TRUE(notMatches(R"cpp(
+ template <class T> struct A0 {};
+ template <class T> struct A1 : A0<T> {};
+ template <class T> struct A2 : A1<T>, A0<T> {};
+ template <class T> struct A3 : A2<T>, A1<T> {};
+ template <class T> struct A4 : A3<T>, A2<T> {};
+ template <class T> struct A5 : A4<T>, A3<T> {};
+ template <class T> struct A6 : A5<T>, A4<T> {};
+ template <class T> struct A7 : A6<T>, A5<T> {};
+ template <class T> struct A8 : A7<T>, A6<T> {};
+ template <class T> struct A9 : A8<T>, A7<T> {};
+ template <class T> struct A10 : A9<T>, A8<T> {};
+ template <class T> struct A11 : A10<T>, A9<T> {};
+ template <class T> struct A12 : A11<T>, A10<T> {};
+ template <class T> struct A13 : A12<T>, A11<T> {};
+ template <class T> struct A14 : A13<T>, A12<T> {};
+ template <class T> struct A15 : A14<T>, A13<T> {};
+ template <class T> struct A16 : A15<T>, A14<T> {};
+ template <class T> struct A17 : A16<T>, A15<T> {};
+ template <class T> struct A18 : A17<T>, A16<T> {};
+ template <class T> struct A19 : A18<T>, A17<T> {};
+ template <class T> struct A20 : A19<T>, A18<T> {};
+ template <class T> struct A21 : A20<T>, A19<T> {};
+ template <class T> struct A22 : A21<T>, A20<T> {};
+ template <class T> struct A23 : A22<T>, A21<T> {};
+ template <class T> struct A24 : A23<T>, A22<T> {};
+ template <class T> struct A25 : A24<T>, A23<T> {};
+ template <class T> struct A26 : A25<T>, A24<T> {};
+ template <class T> struct A27 : A26<T>, A25<T> {};
+ template <class T> struct A28 : A27<T>, A26<T> {};
+ template <class T> struct A29 : A28<T>, A27<T> {};
+ template <class T> struct A30 : A29<T>, A28<T> {};
+ template <class T> struct A31 : A30<T>, A29<T> {};
+ template <class T> struct A32 : A31<T>, A30<T> {};
+ template <class T> struct A33 : A32<T>, A31<T> {};
+ template <class T> struct A34 : A33<T>, A32<T> {};
+ template <class T> struct A35 : A34<T>, A33<T> {};
+ template <class T> struct A36 : A35<T>, A34<T> {};
+ template <class T> struct A37 : A36<T>, A35<T> {};
+ template <class T> struct A38 : A37<T>, A36<T> {};
+ template <class T> struct A39 : A38<T>, A37<T> {};
+ template <class T> struct A40 : A39<T>, A38<T> {};
+)cpp",
+ matcher));
+}
+
TEST(ASTMatchersTestObjC, ObjCMessageCalees) {
StatementMatcher MessagingFoo =
objcMessageExpr(callee(objcMethodDecl(hasName("foo"))));
>From 5e7adf9b0236fda9086993048e259a56f6ad6540 Mon Sep 17 00:00:00 2001
From: Ilya Biryukov <ibiryukov at google.com>
Date: Mon, 25 Sep 2023 12:06:45 +0200
Subject: [PATCH 2/2] fixup! [ASTMatchers] Fix classIsDerivedFrom for recusrive
cases
---
.../ASTMatchers/ASTMatchersNodeTest.cpp | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp b/clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
index 0a08a8b4c491dca..8f0dd5602307c53 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ b/clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -2379,11 +2379,17 @@ TEST_P(ASTMatchersTest, IsDerivedFromRecursion) {
// creating the cycles.
DeclarationMatcher matcher = cxxRecordDecl(isDerivedFrom("X"));
EXPECT_TRUE(notMatches(R"cpp(
- template <typename T1, typename T2 = void, typename T3 = void, typename
- T4 = void> struct M; template <typename T1, typename T2> struct M<T1,
- T2> {}; template <typename T1, typename T2> struct L : M<T1, T2> {};
- template <typename T1, typename T2, typename T3, typename T4>
- struct M : L<M<T1, T2>, M<T3, T4>> {};
+ template <typename T1, typename T2>
+ struct M;
+
+ template <typename T1>
+ struct M<T1, void> {};
+
+ template <typename T1, typename T2>
+ struct L : M<T1, T2> {};
+
+ template <typename T1, typename T2>
+ struct M : L<M<T1, T2>, M<T1, T2>> {};
)cpp",
matcher));
More information about the cfe-commits
mailing list