[clangd] [HeuristicResolver] Protect against infinite recursion on DependentNameTypes #83542
Conversation
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clangd Author: Nathan Ridge (HighCommander4) ChangesFixes clangd/clangd#1951 Full diff: https://github.com/llvm/llvm-project/pull/83542.diff 3 Files Affected:
diff --git a/clang-tools-extra/clangd/HeuristicResolver.cpp b/clang-tools-extra/clangd/HeuristicResolver.cpp
index 3c147b6b582bf0..26d54200eeffd2 100644
--- a/clang-tools-extra/clangd/HeuristicResolver.cpp
+++ b/clang-tools-extra/clangd/HeuristicResolver.cpp
@@ -16,6 +16,80 @@
namespace clang {
namespace clangd {
+namespace {
+
+// Helper class for implementing HeuristicResolver.
+// Unlike HeuristicResolver which is a long-lived class,
+// a new instance of this class is created for every external
+// call into a HeuristicResolver operation. That allows this
+// class to store state that's local to such a top-level call,
+// particularly "recursion protection sets" that keep track of
+// nodes that have already been seen to avoid infinite recursion.
+class HeuristicResolverImpl {
+public:
+ HeuristicResolverImpl(ASTContext &Ctx) : Ctx(Ctx) {}
+
+ // These functions match the public interface of HeuristicResolver
+ // (but aren't const since they may modify the recursion protection sets).
+ std::vector<const NamedDecl *>
+ resolveMemberExpr(const CXXDependentScopeMemberExpr *ME);
+ std::vector<const NamedDecl *>
+ resolveDeclRefExpr(const DependentScopeDeclRefExpr *RE);
+ std::vector<const NamedDecl *> resolveTypeOfCallExpr(const CallExpr *CE);
+ std::vector<const NamedDecl *> resolveCalleeOfCallExpr(const CallExpr *CE);
+ std::vector<const NamedDecl *>
+ resolveUsingValueDecl(const UnresolvedUsingValueDecl *UUVD);
+ std::vector<const NamedDecl *>
+ resolveDependentNameType(const DependentNameType *DNT);
+ std::vector<const NamedDecl *> resolveTemplateSpecializationType(
+ const DependentTemplateSpecializationType *DTST);
+ const Type *resolveNestedNameSpecifierToType(const NestedNameSpecifier *NNS);
+ const Type *getPointeeType(const Type *T);
+
+private:
+ ASTContext &Ctx;
+
+ // Recursion protection sets
+ llvm::SmallSet<const DependentNameType *, 4> SeenDependentNameTypes;
+
+ // Given a tag-decl type and a member name, heuristically resolve the
+ // name to one or more declarations.
+ // The current heuristic is simply to look up the name in the primary
+ // template. This is a heuristic because the template could potentially
+ // have specializations that declare different members.
+ // Multiple declarations could be returned if the name is overloaded
+ // (e.g. an overloaded method in the primary template).
+ // This heuristic will give the desired answer in many cases, e.g.
+ // for a call to vector<T>::size().
+ std::vector<const NamedDecl *>
+ resolveDependentMember(const Type *T, DeclarationName Name,
+ llvm::function_ref<bool(const NamedDecl *ND)> Filter);
+
+ // Try to heuristically resolve the type of a possibly-dependent expression
+ // `E`.
+ const Type *resolveExprToType(const Expr *E);
+ std::vector<const NamedDecl *> resolveExprToDecls(const Expr *E);
+
+ // Helper function for HeuristicResolver::resolveDependentMember()
+ // which takes a possibly-dependent type `T` and heuristically
+ // resolves it to a CXXRecordDecl in which we can try name lookup.
+ CXXRecordDecl *resolveTypeToRecordDecl(const Type *T);
+
+ // This is a reimplementation of CXXRecordDecl::lookupDependentName()
+ // so that the implementation can call into other HeuristicResolver helpers.
+ // FIXME: Once HeuristicResolver is upstreamed to the clang libraries
+ // (https://github.com/clangd/clangd/discussions/1662),
+ // CXXRecordDecl::lookupDepenedentName() can be removed, and its call sites
+ // can be modified to benefit from the more comprehensive heuristics offered
+ // by HeuristicResolver instead.
+ std::vector<const NamedDecl *>
+ lookupDependentName(CXXRecordDecl *RD, DeclarationName Name,
+ llvm::function_ref<bool(const NamedDecl *ND)> Filter);
+ bool findOrdinaryMemberInDependentClasses(const CXXBaseSpecifier *Specifier,
+ CXXBasePath &Path,
+ DeclarationName Name);
+};
+
// Convenience lambdas for use as the 'Filter' parameter of
// HeuristicResolver::resolveDependentMember().
const auto NoFilter = [](const NamedDecl *D) { return true; };
@@ -31,8 +105,6 @@ const auto TemplateFilter = [](const NamedDecl *D) {
return isa<TemplateDecl>(D);
};
-namespace {
-
const Type *resolveDeclsToType(const std::vector<const NamedDecl *> &Decls,
ASTContext &Ctx) {
if (Decls.size() != 1) // Names an overload set -- just bail.
@@ -46,12 +118,10 @@ const Type *resolveDeclsToType(const std::vector<const NamedDecl *> &Decls,
return nullptr;
}
-} // namespace
-
// Helper function for HeuristicResolver::resolveDependentMember()
// which takes a possibly-dependent type `T` and heuristically
// resolves it to a CXXRecordDecl in which we can try name lookup.
-CXXRecordDecl *HeuristicResolver::resolveTypeToRecordDecl(const Type *T) const {
+CXXRecordDecl *HeuristicResolverImpl::resolveTypeToRecordDecl(const Type *T) {
assert(T);
// Unwrap type sugar such as type aliases.
@@ -84,7 +154,7 @@ CXXRecordDecl *HeuristicResolver::resolveTypeToRecordDecl(const Type *T) const {
return TD->getTemplatedDecl();
}
-const Type *HeuristicResolver::getPointeeType(const Type *T) const {
+const Type *HeuristicResolverImpl::getPointeeType(const Type *T) {
if (!T)
return nullptr;
@@ -117,8 +187,8 @@ const Type *HeuristicResolver::getPointeeType(const Type *T) const {
return FirstArg.getAsType().getTypePtrOrNull();
}
-std::vector<const NamedDecl *> HeuristicResolver::resolveMemberExpr(
- const CXXDependentScopeMemberExpr *ME) const {
+std::vector<const NamedDecl *> HeuristicResolverImpl::resolveMemberExpr(
+ const CXXDependentScopeMemberExpr *ME) {
// If the expression has a qualifier, try resolving the member inside the
// qualifier's type.
// Note that we cannot use a NonStaticFilter in either case, for a couple
@@ -164,14 +234,14 @@ std::vector<const NamedDecl *> HeuristicResolver::resolveMemberExpr(
return resolveDependentMember(BaseType, ME->getMember(), NoFilter);
}
-std::vector<const NamedDecl *> HeuristicResolver::resolveDeclRefExpr(
- const DependentScopeDeclRefExpr *RE) const {
+std::vector<const NamedDecl *>
+HeuristicResolverImpl::resolveDeclRefExpr(const DependentScopeDeclRefExpr *RE) {
return resolveDependentMember(RE->getQualifier()->getAsType(),
RE->getDeclName(), StaticFilter);
}
std::vector<const NamedDecl *>
-HeuristicResolver::resolveTypeOfCallExpr(const CallExpr *CE) const {
+HeuristicResolverImpl::resolveTypeOfCallExpr(const CallExpr *CE) {
const auto *CalleeType = resolveExprToType(CE->getCallee());
if (!CalleeType)
return {};
@@ -187,7 +257,7 @@ HeuristicResolver::resolveTypeOfCallExpr(const CallExpr *CE) const {
}
std::vector<const NamedDecl *>
-HeuristicResolver::resolveCalleeOfCallExpr(const CallExpr *CE) const {
+HeuristicResolverImpl::resolveCalleeOfCallExpr(const CallExpr *CE) {
if (const auto *ND = dyn_cast_or_null<NamedDecl>(CE->getCalleeDecl())) {
return {ND};
}
@@ -195,29 +265,31 @@ HeuristicResolver::resolveCalleeOfCallExpr(const CallExpr *CE) const {
return resolveExprToDecls(CE->getCallee());
}
-std::vector<const NamedDecl *> HeuristicResolver::resolveUsingValueDecl(
- const UnresolvedUsingValueDecl *UUVD) const {
+std::vector<const NamedDecl *> HeuristicResolverImpl::resolveUsingValueDecl(
+ const UnresolvedUsingValueDecl *UUVD) {
return resolveDependentMember(UUVD->getQualifier()->getAsType(),
UUVD->getNameInfo().getName(), ValueFilter);
}
-std::vector<const NamedDecl *> HeuristicResolver::resolveDependentNameType(
- const DependentNameType *DNT) const {
+std::vector<const NamedDecl *>
+HeuristicResolverImpl::resolveDependentNameType(const DependentNameType *DNT) {
+ if (auto [_, inserted] = SeenDependentNameTypes.insert(DNT); !inserted)
+ return {};
return resolveDependentMember(
resolveNestedNameSpecifierToType(DNT->getQualifier()),
DNT->getIdentifier(), TypeFilter);
}
std::vector<const NamedDecl *>
-HeuristicResolver::resolveTemplateSpecializationType(
- const DependentTemplateSpecializationType *DTST) const {
+HeuristicResolverImpl::resolveTemplateSpecializationType(
+ const DependentTemplateSpecializationType *DTST) {
return resolveDependentMember(
resolveNestedNameSpecifierToType(DTST->getQualifier()),
DTST->getIdentifier(), TemplateFilter);
}
std::vector<const NamedDecl *>
-HeuristicResolver::resolveExprToDecls(const Expr *E) const {
+HeuristicResolverImpl::resolveExprToDecls(const Expr *E) {
if (const auto *ME = dyn_cast<CXXDependentScopeMemberExpr>(E)) {
return resolveMemberExpr(ME);
}
@@ -236,7 +308,7 @@ HeuristicResolver::resolveExprToDecls(const Expr *E) const {
return {};
}
-const Type *HeuristicResolver::resolveExprToType(const Expr *E) const {
+const Type *HeuristicResolverImpl::resolveExprToType(const Expr *E) {
std::vector<const NamedDecl *> Decls = resolveExprToDecls(E);
if (!Decls.empty())
return resolveDeclsToType(Decls, Ctx);
@@ -244,8 +316,8 @@ const Type *HeuristicResolver::resolveExprToType(const Expr *E) const {
return E->getType().getTypePtr();
}
-const Type *HeuristicResolver::resolveNestedNameSpecifierToType(
- const NestedNameSpecifier *NNS) const {
+const Type *HeuristicResolverImpl::resolveNestedNameSpecifierToType(
+ const NestedNameSpecifier *NNS) {
if (!NNS)
return nullptr;
@@ -270,8 +342,6 @@ const Type *HeuristicResolver::resolveNestedNameSpecifierToType(
return nullptr;
}
-namespace {
-
bool isOrdinaryMember(const NamedDecl *ND) {
return ND->isInIdentifierNamespace(Decl::IDNS_Ordinary | Decl::IDNS_Tag |
Decl::IDNS_Member);
@@ -287,11 +357,9 @@ bool findOrdinaryMember(const CXXRecordDecl *RD, CXXBasePath &Path,
return false;
}
-} // namespace
-
-bool HeuristicResolver::findOrdinaryMemberInDependentClasses(
+bool HeuristicResolverImpl::findOrdinaryMemberInDependentClasses(
const CXXBaseSpecifier *Specifier, CXXBasePath &Path,
- DeclarationName Name) const {
+ DeclarationName Name) {
CXXRecordDecl *RD =
resolveTypeToRecordDecl(Specifier->getType().getTypePtr());
if (!RD)
@@ -299,9 +367,9 @@ bool HeuristicResolver::findOrdinaryMemberInDependentClasses(
return findOrdinaryMember(RD, Path, Name);
}
-std::vector<const NamedDecl *> HeuristicResolver::lookupDependentName(
+std::vector<const NamedDecl *> HeuristicResolverImpl::lookupDependentName(
CXXRecordDecl *RD, DeclarationName Name,
- llvm::function_ref<bool(const NamedDecl *ND)> Filter) const {
+ llvm::function_ref<bool(const NamedDecl *ND)> Filter) {
std::vector<const NamedDecl *> Results;
// Lookup in the class.
@@ -332,9 +400,9 @@ std::vector<const NamedDecl *> HeuristicResolver::lookupDependentName(
return Results;
}
-std::vector<const NamedDecl *> HeuristicResolver::resolveDependentMember(
+std::vector<const NamedDecl *> HeuristicResolverImpl::resolveDependentMember(
const Type *T, DeclarationName Name,
- llvm::function_ref<bool(const NamedDecl *ND)> Filter) const {
+ llvm::function_ref<bool(const NamedDecl *ND)> Filter) {
if (!T)
return {};
if (auto *ET = T->getAs<EnumType>()) {
@@ -349,6 +417,44 @@ std::vector<const NamedDecl *> HeuristicResolver::resolveDependentMember(
}
return {};
}
+} // namespace
+
+std::vector<const NamedDecl *> HeuristicResolver::resolveMemberExpr(
+ const CXXDependentScopeMemberExpr *ME) const {
+ return HeuristicResolverImpl(Ctx).resolveMemberExpr(ME);
+}
+std::vector<const NamedDecl *> HeuristicResolver::resolveDeclRefExpr(
+ const DependentScopeDeclRefExpr *RE) const {
+ return HeuristicResolverImpl(Ctx).resolveDeclRefExpr(RE);
+}
+std::vector<const NamedDecl *>
+HeuristicResolver::resolveTypeOfCallExpr(const CallExpr *CE) const {
+ return HeuristicResolverImpl(Ctx).resolveTypeOfCallExpr(CE);
+}
+std::vector<const NamedDecl *>
+HeuristicResolver::resolveCalleeOfCallExpr(const CallExpr *CE) const {
+ return HeuristicResolverImpl(Ctx).resolveCalleeOfCallExpr(CE);
+}
+std::vector<const NamedDecl *> HeuristicResolver::resolveUsingValueDecl(
+ const UnresolvedUsingValueDecl *UUVD) const {
+ return HeuristicResolverImpl(Ctx).resolveUsingValueDecl(UUVD);
+}
+std::vector<const NamedDecl *> HeuristicResolver::resolveDependentNameType(
+ const DependentNameType *DNT) const {
+ return HeuristicResolverImpl(Ctx).resolveDependentNameType(DNT);
+}
+std::vector<const NamedDecl *>
+HeuristicResolver::resolveTemplateSpecializationType(
+ const DependentTemplateSpecializationType *DTST) const {
+ return HeuristicResolverImpl(Ctx).resolveTemplateSpecializationType(DTST);
+}
+const Type *HeuristicResolver::resolveNestedNameSpecifierToType(
+ const NestedNameSpecifier *NNS) const {
+ return HeuristicResolverImpl(Ctx).resolveNestedNameSpecifierToType(NNS);
+}
+const Type *HeuristicResolver::getPointeeType(const Type *T) const {
+ return HeuristicResolverImpl(Ctx).getPointeeType(T);
+}
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/HeuristicResolver.h b/clang-tools-extra/clangd/HeuristicResolver.h
index dc04123d37593c..dcc063bbc4adc0 100644
--- a/clang-tools-extra/clangd/HeuristicResolver.h
+++ b/clang-tools-extra/clangd/HeuristicResolver.h
@@ -77,43 +77,6 @@ class HeuristicResolver {
private:
ASTContext &Ctx;
-
- // Given a tag-decl type and a member name, heuristically resolve the
- // name to one or more declarations.
- // The current heuristic is simply to look up the name in the primary
- // template. This is a heuristic because the template could potentially
- // have specializations that declare different members.
- // Multiple declarations could be returned if the name is overloaded
- // (e.g. an overloaded method in the primary template).
- // This heuristic will give the desired answer in many cases, e.g.
- // for a call to vector<T>::size().
- std::vector<const NamedDecl *> resolveDependentMember(
- const Type *T, DeclarationName Name,
- llvm::function_ref<bool(const NamedDecl *ND)> Filter) const;
-
- // Try to heuristically resolve the type of a possibly-dependent expression
- // `E`.
- const Type *resolveExprToType(const Expr *E) const;
- std::vector<const NamedDecl *> resolveExprToDecls(const Expr *E) const;
-
- // Helper function for HeuristicResolver::resolveDependentMember()
- // which takes a possibly-dependent type `T` and heuristically
- // resolves it to a CXXRecordDecl in which we can try name lookup.
- CXXRecordDecl *resolveTypeToRecordDecl(const Type *T) const;
-
- // This is a reimplementation of CXXRecordDecl::lookupDependentName()
- // so that the implementation can call into other HeuristicResolver helpers.
- // FIXME: Once HeuristicResolver is upstreamed to the clang libraries
- // (https://github.com/clangd/clangd/discussions/1662),
- // CXXRecordDecl::lookupDepenedentName() can be removed, and its call sites
- // can be modified to benefit from the more comprehensive heuristics offered
- // by HeuristicResolver instead.
- std::vector<const NamedDecl *> lookupDependentName(
- CXXRecordDecl *RD, DeclarationName Name,
- llvm::function_ref<bool(const NamedDecl *ND)> Filter) const;
- bool findOrdinaryMemberInDependentClasses(const CXXBaseSpecifier *Specifier,
- CXXBasePath &Path,
- DeclarationName Name) const;
};
} // namespace clangd
diff --git a/clang-tools-extra/clangd/unittests/FindTargetTests.cpp b/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
index 29cff68cf03b2e..24d021a6c61006 100644
--- a/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ b/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -1009,6 +1009,16 @@ TEST_F(TargetDeclTest, DependentTypes) {
)cpp";
EXPECT_DECLS("DependentTemplateSpecializationTypeLoc",
"template <typename> struct B");
+
+ // Dependent name with recursive definition. We don't expect a
+ // result, but we shouldn't get into a stack overflow either.
+ Code = R"cpp(
+ template <int N>
+ struct waldo {
+ typedef typename waldo<N - 1>::type::[[next]] type;
+ };
+ )cpp";
+ EXPECT_DECLS("DependentNameTypeLoc");
}
TEST_F(TargetDeclTest, TypedefCascade) {
|
There was a problem hiding this comment.
Thank you for the quick fix! While I think the patch looks generally good to me, it would be better to add some analysis of the bug.
That said, I took a closer look at it, and I think I probably have another similar approach that doesn't involve extensive refactors. And here is my analysis:
(Let's use the example from your test case)
template <int N>
struct waldo {
using type = waldo<N - 1>::type::next;
};So, we're somehow attempting to resolve the type/decl for next, which is of DependentNameType. To achieve that, we want to inspect the type of its prefix i.e. the waldo<N - 1>::type, which is another NestedNameSpecifier and thus we shall find out the type of waldo<N - 1> first - which is of ClassTemplateSpecializationType. Then, we would perform a name lookup for type within the context thereof, which results in the solely TypedefType we're currently in.
Then we move on and try to find out what the next is with the TypedefType as its prefix. This would happen in resolveDependentMember, with T as TypedefType and Name as next. We expect that prefix to be of a RecordType so that we can perform a name lookup. That is resolveTypeToRecordDecl, and the type alias would get unwrapped and become a DependentNameType in the end.
And now we're successfully getting into an infinite loop: we want to know the type of type, which ends up a type alias type that would boil down to itself!
The PR resolves it by tracking every seen DependentNameType with a set. And since we don't want to turn the stateless HeuristicResolver to something stateful (e.g. adding extra data members), we have to add another layer to hide our implementation - this is what you've done.
However, I think we could track these DependentNameTypes by adding some extra parameters. Specifically, we can pass through the DNT from resolveDependentNameType to resolveDependentMember and resolveTypeToRecordDecl, then inside resolveTypeToRecordDecl, we bail out with null if the unwrapped DependentNameType from T is the same as the DNT - I did an experiment and it turns out to be working.
That said, I'd like to leave the decision up to you - my approach doesn't seem much clearer than yours!
(Please add some details to the commit message before merging :-)
Thanks for the detailed writeup of how the issue arises! This matches my understanding. Another way to think about it is that
Thanks for the alternative suggestion! I've considered this, but it's slightly less general than my fix. It handles the case where the we arrive back to the same DependentNameType in a single level of recursion, but we can also construct cases where it takes two or more levels. Here is an example with two levels: template <int N>
struct odd;
template <int N>
struct even {
using type = typename odd<N - 1>::type::next;
};
template <int N>
struct odd {
using type = typename even<N - 1>::type::next;
};To handle such cases, we need to keep track of a set of seen DependentNameTypes (hence the We could also consider a variation of your approach where we have a On the whole, I prefer the member variable approach for a couple of reasons:
|
|
Great example, thanks! Now I understand the "two-level" case better. I don't have any other concerns about the refactor, so feel free to land it or wait for other folks' opinions. |
…pendentNameTypes When resolving names inside templates that implement recursive compile-time functions (e.g. waldo<N>::type is defined in terms of waldo<N-1>::type), HeuristicResolver could get into an infinite recursion, specifically one where resolveDependentNameType() can be called recursively with the same DependentNameType*. To guard against this, HeuristicResolver tracks, for each external call into a HeuristicResolver function, the set of DependentNameTypes that it has seen, and bails if it sees the same DependentNameType again. To implement this, a helper class HeuristicResolverImpl is introduced to store state that persists for the duration of an external call into HeuristicResolver (but does not persist between such calls). Fixes clangd/clangd#1951
HighCommander4
force-pushed
the
issue-1951
branch
from
0ad99c6 to
1495cd5
Compare
|
Updated patch with two changes:
|
Thank you for reviewing! I'm going to merge this sooner rather than later, since due to the severe nature of the bug (crash on standard library headers), I'd like to backport it to LLVM 18. Of course, I'm happy to make follow-up changes if others have additional feedback. |
…pendentNameTypes (llvm#83542) When resolving names inside templates that implement recursive compile-time functions (e.g. waldo<N>::type is defined in terms of waldo<N-1>::type), HeuristicResolver could get into an infinite recursion, specifically one where resolveDependentNameType() can be called recursively with the same DependentNameType*. To guard against this, HeuristicResolver tracks, for each external call into a HeuristicResolver function, the set of DependentNameTypes that it has seen, and bails if it sees the same DependentNameType again. To implement this, a helper class HeuristicResolverImpl is introduced to store state that persists for the duration of an external call into HeuristicResolver (but does not persist between such calls). Fixes clangd/clangd#1951 (cherry picked from commit e6e53ca)
…pendentNameTypes (llvm#83542) When resolving names inside templates that implement recursive compile-time functions (e.g. waldo<N>::type is defined in terms of waldo<N-1>::type), HeuristicResolver could get into an infinite recursion, specifically one where resolveDependentNameType() can be called recursively with the same DependentNameType*. To guard against this, HeuristicResolver tracks, for each external call into a HeuristicResolver function, the set of DependentNameTypes that it has seen, and bails if it sees the same DependentNameType again. To implement this, a helper class HeuristicResolverImpl is introduced to store state that persists for the duration of an external call into HeuristicResolver (but does not persist between such calls). Fixes clangd/clangd#1951 (cherry picked from commit e6e53ca)
When resolving names inside templates that implement recursive
compile-time functions (e.g. waldo::type is defined in terms
of waldo::type), HeuristicResolver could get into an infinite
recursion, specifically one where resolveDependentNameType() can
be called recursively with the same DependentNameType*.
To guard against this, HeuristicResolver tracks, for each external
call into a HeuristicResolver function, the set of DependentNameTypes
that it has seen, and bails if it sees the same DependentNameType again.
To implement this, a helper class HeuristicResolverImpl is introduced
to store state that persists for the duration of an external call into
HeuristicResolver (but does not persist between such calls).
Fixes clangd/clangd#1951