[ASTMatchers] Fix classIsDerivedFrom for recusrive cases #67307
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-tidy ChangesBy 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. Full diff: https://github.com/llvm/llvm-project/pull/67307.diff 2 Files Affected:
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"))));
|
sam-mccall
approved these changes
Sep 25, 2023
|
Hi @ilya-biryukov - it seems this PR caused a couple test failures in RecursiveASTVisitorTests/ on the Fuchsia windows-x64 builder. Could you take a look to verify whether an additional fix (or revert) is needed? clang-windows-x64-test failures |
|
@Caslyn that failure doesn't look plausible for this commit, there's no dependency on AST matchers I'm not familiar with this buildbot, but it looks like there was an infra problem (failing install step) for a while that recently got fixed, and this test failure existed before that, e.g.: |
|
Hi @sam-mccall - thank you, and apologies for the false alarm. After closer inspection, it looks like #66114 (comment) explains the particular failures we're seeing. Indeed, no relation to this PR - please consider this matter closed here. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.