-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[clang] fix infinite recursion #143244
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.
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
[clang] fix infinite recursion #143244
Conversation
@llvm/pr-subscribers-clang Author: Zhikai Zeng (Backl1ght) Changesfix #141789 The direct cause of infinite recursion is that
#104829 fix similar infinite recursion, but I think it is no longer needed so I kind of revert it. Full diff: https://github.com/llvm/llvm-project/pull/143244.diff 2 Files Affected:
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 5ef2ae3ee857f..9f66c194a45dd 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -7172,7 +7172,10 @@ void Sema::CheckCompletedCXXClass(Scope *S, CXXRecordDecl *Record) {
// "effectively constexpr" for better compatibility.
// See https://github.com/llvm/llvm-project/issues/102293 for more info.
if (isa<CXXDestructorDecl>(M)) {
- auto Check = [](QualType T, auto &&Check) -> bool {
+ llvm::DenseSet<QualType> Visited;
+ auto Check = [&Visited](QualType T, auto &&Check) -> bool {
+ if (!Visited.insert(T).second)
+ return false;
const CXXRecordDecl *RD =
T->getBaseElementTypeUnsafe()->getAsCXXRecordDecl();
if (!RD || !RD->isCompleteDefinition())
@@ -7181,16 +7184,11 @@ void Sema::CheckCompletedCXXClass(Scope *S, CXXRecordDecl *Record) {
if (!RD->hasConstexprDestructor())
return false;
- QualType CanUnqualT = T.getCanonicalType().getUnqualifiedType();
for (const CXXBaseSpecifier &B : RD->bases())
- if (B.getType().getCanonicalType().getUnqualifiedType() !=
- CanUnqualT &&
- !Check(B.getType(), Check))
+ if (!Check(B.getType(), Check))
return false;
for (const FieldDecl *FD : RD->fields())
- if (FD->getType().getCanonicalType().getUnqualifiedType() !=
- CanUnqualT &&
- !Check(FD->getType(), Check))
+ if (!Check(FD->getType(), Check))
return false;
return true;
};
diff --git a/clang/test/SemaCXX/gh102293.cpp b/clang/test/SemaCXX/gh102293.cpp
index d4218cc13dcec..fe417e697841b 100644
--- a/clang/test/SemaCXX/gh102293.cpp
+++ b/clang/test/SemaCXX/gh102293.cpp
@@ -45,3 +45,20 @@ class quux : quux { // expected-error {{base class has incomplete type}} \
virtual int c();
};
}
+
+// Ensure we don't get infinite recursion from the check, however. See GH141789
+namespace GH141789 {
+template <typename Ty>
+struct S {
+ Ty t; // expected-error {{field has incomplete type 'GH141789::X'}}
+};
+
+struct T {
+ ~T();
+};
+
+struct X { // expected-note {{definition of 'GH141789::X' is not complete until the closing '}'}}
+ S<X> next; // expected-note {{in instantiation of template class 'GH141789::S<GH141789::X>' requested here}}
+ T m;
+};
+}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a set is probably easier than trying to figure out all the ways in which this could possibly recurse.
This still needs a release note though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the fix, this overall lg. But perhaps a release note makes sense, and I agree with @Sirraide that canonical types need to be stored into the set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two minor comments but lgtm otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/169/builds/12235 Here is the relevant piece of the build log for the reference
|
fix llvm#141789 The direct cause of infinite recursion is that `T` is changing from `struct X` and `S<X>` infinitely, this pr add a check that if `T` visited before then return false directly. ```plaintext /home/backlight/llvm-project/clang/lib/Sema/SemaDeclCXX.cpp:7196] FD->getType().getAsString()=struct X, T.getAsString()=S<X>, FD->getType().getCanonicalType().getUnqualifiedType().getAsString()=struct X, CanUnqualT.getAsString()=struct S<struct X>, /home/backlight/llvm-project/clang/lib/Sema/SemaDeclCXX.cpp:7196] FD->getType().getAsString()=S<X>, T.getAsString()=struct X, FD->getType().getCanonicalType().getUnqualifiedType().getAsString()=struct S<struct X>, CanUnqualT.getAsString()=struct X, ``` llvm#104829 fix similar infinite recursion, but I think it is no longer needed so I kind of revert it.
fix #141789
The direct cause of infinite recursion is that
T
is changing fromstruct X
andS<X>
infinitely, this pr add a check that ifT
visited before then return false directly.#104829 fix similar infinite recursion, but I think it is no longer needed so I kind of revert it.