[clang] [Serialization] Introduce noload_redecls #170823
Open
+103
−15
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
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: Chuanqi Xu (ChuanqiXu9) ChangesI meant to add test but I found it is hard as now we will always load every redecls after ASTReader. So I just add an assert for it. Full diff: https://github.com/llvm/llvm-project/pull/170823.diff 11 Files Affected:
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index 2e8ceff453547..632a7326316a0 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -108,6 +108,9 @@ class TranslationUnitDecl : public Decl,
TranslationUnitDecl *getNextRedeclarationImpl() override {
return getNextRedeclaration();
}
+ TranslationUnitDecl *getNextRedeclarationNoUpdateImpl() override {
+ return getNextRedeclarationNoUpdate();
+ }
TranslationUnitDecl *getPreviousDeclImpl() override {
return getPreviousDecl();
@@ -607,6 +610,7 @@ class NamespaceDecl : public NamespaceBaseDecl,
using redeclarable_base = Redeclarable<NamespaceDecl>;
NamespaceDecl *getNextRedeclarationImpl() override;
+ NamespaceDecl *getNextRedeclarationNoUpdateImpl() override;
NamespaceDecl *getPreviousDeclImpl() override;
NamespaceDecl *getMostRecentDeclImpl() override;
@@ -1135,6 +1139,10 @@ class VarDecl : public DeclaratorDecl, public Redeclarable<VarDecl> {
return getNextRedeclaration();
}
+ VarDecl *getNextRedeclarationNoUpdateImpl() override {
+ return getNextRedeclarationNoUpdate();
+ }
+
VarDecl *getPreviousDeclImpl() override {
return getPreviousDecl();
}
@@ -2163,6 +2171,10 @@ class FunctionDecl : public DeclaratorDecl,
return getNextRedeclaration();
}
+ FunctionDecl *getNextRedeclarationNoUpdateImpl() override {
+ return getNextRedeclarationNoUpdate();
+ }
+
FunctionDecl *getPreviousDeclImpl() override {
return getPreviousDecl();
}
@@ -3586,6 +3598,10 @@ class TypedefNameDecl : public TypeDecl, public Redeclarable<TypedefNameDecl> {
return getNextRedeclaration();
}
+ TypedefNameDecl *getNextRedeclarationNoUpdateImpl() override {
+ return getNextRedeclarationNoUpdate();
+ }
+
TypedefNameDecl *getPreviousDeclImpl() override {
return getPreviousDecl();
}
@@ -3755,6 +3771,10 @@ class TagDecl : public TypeDecl,
return getNextRedeclaration();
}
+ TagDecl *getNextRedeclarationNoUpdateImpl() override {
+ return getNextRedeclarationNoUpdate();
+ }
+
TagDecl *getPreviousDeclImpl() override {
return getPreviousDecl();
}
diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h
index 5519787d71f88..ea30d1dfc8f37 100644
--- a/clang/include/clang/AST/DeclBase.h
+++ b/clang/include/clang/AST/DeclBase.h
@@ -989,6 +989,10 @@ class alignas(8) Decl {
/// Decl subclasses that can be redeclared should override this method so that
/// Decl::redecl_iterator can iterate over them.
virtual Decl *getNextRedeclarationImpl() { return this; }
+ /// Returns the next redeclaration without loading.
+ /// FIXME: We may be able to erase such unneccesary virtual function call by
+ /// introduce CRTP.
+ virtual Decl *getNextRedeclarationNoUpdateImpl() { return this; }
/// Implementation of getPreviousDecl(), to be overridden by any
/// subclass that has a redeclaration chain.
@@ -1000,7 +1004,8 @@ class alignas(8) Decl {
public:
/// Iterates through all the redeclarations of the same decl.
- class redecl_iterator {
+ template <bool Update = true>
+ class redecl_iterator_impl {
/// Current - The current declaration.
Decl *Current = nullptr;
Decl *Starter;
@@ -1012,36 +1017,37 @@ class alignas(8) Decl {
using iterator_category = std::forward_iterator_tag;
using difference_type = std::ptrdiff_t;
- redecl_iterator() = default;
- explicit redecl_iterator(Decl *C) : Current(C), Starter(C) {}
+ redecl_iterator_impl() = default;
+ explicit redecl_iterator_impl(Decl *C) : Current(C), Starter(C) {}
reference operator*() const { return Current; }
value_type operator->() const { return Current; }
- redecl_iterator& operator++() {
+ redecl_iterator_impl& operator++() {
assert(Current && "Advancing while iterator has reached end");
// Get either previous decl or latest decl.
- Decl *Next = Current->getNextRedeclarationImpl();
+ Decl *Next = Update ? Current->getNextRedeclarationImpl() : Current->getNextRedeclarationNoUpdateImpl();
assert(Next && "Should return next redeclaration or itself, never null!");
Current = (Next != Starter) ? Next : nullptr;
return *this;
}
- redecl_iterator operator++(int) {
+ redecl_iterator_impl operator++(int) {
redecl_iterator tmp(*this);
++(*this);
return tmp;
}
- friend bool operator==(redecl_iterator x, redecl_iterator y) {
+ friend bool operator==(redecl_iterator_impl x, redecl_iterator_impl y) {
return x.Current == y.Current;
}
- friend bool operator!=(redecl_iterator x, redecl_iterator y) {
+ friend bool operator!=(redecl_iterator_impl x, redecl_iterator_impl y) {
return x.Current != y.Current;
}
};
+ using redecl_iterator = redecl_iterator_impl</*update=*/true>;
using redecl_range = llvm::iterator_range<redecl_iterator>;
/// Returns an iterator range for all the redeclarations of the same
@@ -1056,6 +1062,19 @@ class alignas(8) Decl {
redecl_iterator redecls_end() const { return redecl_iterator(); }
+ using noload_redecl_iterator = redecl_iterator_impl</*update=*/false>;
+ using noload_redecl_range = llvm::iterator_range<noload_redecl_iterator>;
+
+ noload_redecl_range noload_redecls() const {
+ return noload_redecl_range(noload_redecls_begin(), noload_redecls_end());
+ }
+
+ noload_redecl_iterator noload_redecls_begin() const {
+ return noload_redecl_iterator(const_cast<Decl *>(this));
+ }
+
+ noload_redecl_iterator noload_redecls_end() const { return noload_redecl_iterator(); }
+
/// Retrieve the previous declaration that declares the same entity
/// as this declaration, or NULL if there is no previous declaration.
Decl *getPreviousDecl() { return getPreviousDeclImpl(); }
diff --git a/clang/include/clang/AST/DeclCXX.h b/clang/include/clang/AST/DeclCXX.h
index dfa3befb27dd0..4238885cc4678 100644
--- a/clang/include/clang/AST/DeclCXX.h
+++ b/clang/include/clang/AST/DeclCXX.h
@@ -3229,6 +3229,7 @@ class NamespaceAliasDecl : public NamespaceBaseDecl,
using redeclarable_base = Redeclarable<NamespaceAliasDecl>;
NamespaceAliasDecl *getNextRedeclarationImpl() override;
+ NamespaceAliasDecl *getNextRedeclarationNoUpdateImpl() override;
NamespaceAliasDecl *getPreviousDeclImpl() override;
NamespaceAliasDecl *getMostRecentDeclImpl() override;
@@ -3414,6 +3415,10 @@ class UsingShadowDecl : public NamedDecl, public Redeclarable<UsingShadowDecl> {
return getNextRedeclaration();
}
+ UsingShadowDecl *getNextRedeclarationNoUpdateImpl() override {
+ return getNextRedeclarationNoUpdate();
+ }
+
UsingShadowDecl *getPreviousDeclImpl() override {
return getPreviousDecl();
}
diff --git a/clang/include/clang/AST/DeclObjC.h b/clang/include/clang/AST/DeclObjC.h
index 2541edba83855..dfdefc6f90316 100644
--- a/clang/include/clang/AST/DeclObjC.h
+++ b/clang/include/clang/AST/DeclObjC.h
@@ -221,6 +221,7 @@ class ObjCMethodDecl : public NamedDecl, public DeclContext {
/// An interface declaration will return its definition.
/// Otherwise it will return itself.
ObjCMethodDecl *getNextRedeclarationImpl() override;
+ ObjCMethodDecl *getNextRedeclarationNoUpdateImpl() override;
public:
friend class ASTDeclReader;
@@ -1265,6 +1266,9 @@ class ObjCInterfaceDecl : public ObjCContainerDecl
ObjCInterfaceDecl *getNextRedeclarationImpl() override {
return getNextRedeclaration();
}
+ ObjCInterfaceDecl *getNextRedeclarationNoUpdateImpl() override {
+ return getNextRedeclarationNoUpdate();
+ }
ObjCInterfaceDecl *getPreviousDeclImpl() override {
return getPreviousDecl();
@@ -2122,6 +2126,9 @@ class ObjCProtocolDecl : public ObjCContainerDecl,
ObjCProtocolDecl *getNextRedeclarationImpl() override {
return getNextRedeclaration();
}
+ ObjCProtocolDecl *getNextRedeclarationNoUpdateImpl() override {
+ return getNextRedeclarationNoUpdate();
+ }
ObjCProtocolDecl *getPreviousDeclImpl() override {
return getPreviousDecl();
diff --git a/clang/include/clang/AST/DeclTemplate.h b/clang/include/clang/AST/DeclTemplate.h
index a4a1bb9c13c79..f836b32314eff 100644
--- a/clang/include/clang/AST/DeclTemplate.h
+++ b/clang/include/clang/AST/DeclTemplate.h
@@ -718,6 +718,9 @@ class RedeclarableTemplateDecl : public TemplateDecl,
RedeclarableTemplateDecl *getNextRedeclarationImpl() override {
return getNextRedeclaration();
}
+ RedeclarableTemplateDecl *getNextRedeclarationNoUpdateImpl() override {
+ return getNextRedeclarationNoUpdate();
+ }
RedeclarableTemplateDecl *getPreviousDeclImpl() override {
return getPreviousDecl();
diff --git a/clang/include/clang/AST/Redeclarable.h b/clang/include/clang/AST/Redeclarable.h
index 68516c66aaf65..2003d624e07db 100644
--- a/clang/include/clang/AST/Redeclarable.h
+++ b/clang/include/clang/AST/Redeclarable.h
@@ -117,6 +117,7 @@ class Redeclarable {
isa<UninitializedLatest>(cast<NotKnownLatest>(Link));
}
+ template <bool Update = true>
decl_type *getPrevious(const decl_type *D) const {
if (NotKnownLatest NKL = dyn_cast<NotKnownLatest>(Link)) {
if (auto *Prev = dyn_cast<Previous>(NKL))
@@ -128,7 +129,8 @@ class Redeclarable {
const_cast<decl_type *>(D));
}
- return static_cast<decl_type *>(cast<KnownLatest>(Link).get(D));
+ return Update ? static_cast<decl_type *>(cast<KnownLatest>(Link).get(D))
+ : static_cast<decl_type *>(cast<KnownLatest>(Link).getNotUpdated());
}
void setPrevious(decl_type *D) {
@@ -183,7 +185,11 @@ class Redeclarable {
decl_type *First;
decl_type *getNextRedeclaration() const {
- return RedeclLink.getPrevious(static_cast<const decl_type *>(this));
+ return RedeclLink.template getPrevious</*update=*/true>(static_cast<const decl_type *>(this));
+ }
+
+ decl_type *getNextRedeclarationNoUpdate() const {
+ return RedeclLink.template getPrevious</*update=*/false>(static_cast<const decl_type *>(this));
}
public:
diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
index d276f0d21b958..1ac4b2566b54c 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -2078,6 +2078,8 @@ class ASTReader
return static_cast<unsigned>(DeclsLoaded.size());
}
+ unsigned getNumDeclsLoaded() const;
+
/// Returns the number of submodules known.
unsigned getTotalNumSubmodules() const {
return static_cast<unsigned>(SubmodulesLoaded.size());
diff --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp
index 24e4f189cbe4a..60b15244535a4 100644
--- a/clang/lib/AST/DeclCXX.cpp
+++ b/clang/lib/AST/DeclCXX.cpp
@@ -3276,6 +3276,10 @@ NamespaceDecl *NamespaceDecl::getNextRedeclarationImpl() {
return getNextRedeclaration();
}
+NamespaceDecl *NamespaceDecl::getNextRedeclarationNoUpdateImpl() {
+ return getNextRedeclarationNoUpdate();
+}
+
NamespaceDecl *NamespaceDecl::getPreviousDeclImpl() {
return getPreviousDecl();
}
@@ -3290,6 +3294,10 @@ NamespaceAliasDecl *NamespaceAliasDecl::getNextRedeclarationImpl() {
return getNextRedeclaration();
}
+NamespaceAliasDecl *NamespaceAliasDecl::getNextRedeclarationNoUpdateImpl() {
+ return getNextRedeclarationNoUpdate();
+}
+
NamespaceAliasDecl *NamespaceAliasDecl::getPreviousDeclImpl() {
return getPreviousDecl();
}
diff --git a/clang/lib/AST/DeclObjC.cpp b/clang/lib/AST/DeclObjC.cpp
index a66eb72981084..e07607b075bdd 100644
--- a/clang/lib/AST/DeclObjC.cpp
+++ b/clang/lib/AST/DeclObjC.cpp
@@ -1006,6 +1006,11 @@ ObjCMethodDecl *ObjCMethodDecl::getNextRedeclarationImpl() {
return Redecl ? Redecl : this;
}
+// FIXME: make sure ObjCMethodDecl::getNextRedeclarationImpl wont't load.
+ObjCMethodDecl *ObjCMethodDecl::getNextRedeclarationNoUpdateImpl() {
+ return getNextRedeclarationImpl();
+}
+
ObjCMethodDecl *ObjCMethodDecl::getCanonicalDecl() {
auto *CtxD = cast<Decl>(getDeclContext());
const auto &Sel = getSelector();
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index aec61322fb8be..0d9edff273b6a 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -8847,14 +8847,17 @@ void ASTReader::StartTranslationUnit(ASTConsumer *Consumer) {
DeserializationListener->ReaderInitialized(this);
}
+unsigned ASTReader::getNumDeclsLoaded() const {
+ return DeclsLoaded.size() -
+ llvm::count(DeclsLoaded.materialized(), (Decl *)nullptr);
+}
+
void ASTReader::PrintStats() {
std::fprintf(stderr, "*** AST File Statistics:\n");
unsigned NumTypesLoaded =
TypesLoaded.size() - llvm::count(TypesLoaded.materialized(), QualType());
- unsigned NumDeclsLoaded =
- DeclsLoaded.size() -
- llvm::count(DeclsLoaded.materialized(), (Decl *)nullptr);
+ unsigned NumDeclsLoaded = getNumDeclsLoaded();
unsigned NumIdentifiersLoaded =
IdentifiersLoaded.size() -
llvm::count(IdentifiersLoaded, (IdentifierInfo *)nullptr);
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 882d54f31280a..6e180b48e9e95 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -2107,8 +2107,18 @@ void ASTDeclMerger::MergeDefinitionData(
auto *Def = DD.Definition;
DD = std::move(MergeDD);
DD.Definition = Def;
- for (auto *D : Def->redecls())
- cast<CXXRecordDecl>(D)->DefinitionData = ⅅ
+
+#ifndef NDEBUG
+ unsigned OldLoadedSize = Reader.getNumDeclsLoaded();
+#endif
+
+ for (auto *RD : Def->noload_redecls())
+ cast<CXXRecordDecl>(RD)->DefinitionData = ⅅ
+
+#ifndef NDEBUG
+ assert(Reader.getNumDeclsLoaded() == OldLoadedSize && "We shouldn't load new decls during merge definition data for class");
+#endif
+
return;
}
|
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions h,cpp -- clang/include/clang/AST/Decl.h clang/include/clang/AST/DeclBase.h clang/include/clang/AST/DeclCXX.h clang/include/clang/AST/DeclObjC.h clang/include/clang/AST/DeclTemplate.h clang/include/clang/AST/Redeclarable.h clang/include/clang/Serialization/ASTReader.h clang/lib/AST/DeclCXX.cpp clang/lib/AST/DeclObjC.cpp clang/lib/Serialization/ASTReader.cpp clang/lib/Serialization/ASTReaderDecl.cpp --diff_from_common_commit
View the diff from clang-format here.diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h
index ea30d1dfc..1c471b91c 100644
--- a/clang/include/clang/AST/DeclBase.h
+++ b/clang/include/clang/AST/DeclBase.h
@@ -1004,8 +1004,7 @@ protected:
public:
/// Iterates through all the redeclarations of the same decl.
- template <bool Update = true>
- class redecl_iterator_impl {
+ template <bool Update = true> class redecl_iterator_impl {
/// Current - The current declaration.
Decl *Current = nullptr;
Decl *Starter;
@@ -1023,10 +1022,11 @@ public:
reference operator*() const { return Current; }
value_type operator->() const { return Current; }
- redecl_iterator_impl& operator++() {
+ redecl_iterator_impl &operator++() {
assert(Current && "Advancing while iterator has reached end");
// Get either previous decl or latest decl.
- Decl *Next = Update ? Current->getNextRedeclarationImpl() : Current->getNextRedeclarationNoUpdateImpl();
+ Decl *Next = Update ? Current->getNextRedeclarationImpl()
+ : Current->getNextRedeclarationNoUpdateImpl();
assert(Next && "Should return next redeclaration or itself, never null!");
Current = (Next != Starter) ? Next : nullptr;
return *this;
@@ -1073,7 +1073,9 @@ public:
return noload_redecl_iterator(const_cast<Decl *>(this));
}
- noload_redecl_iterator noload_redecls_end() const { return noload_redecl_iterator(); }
+ noload_redecl_iterator noload_redecls_end() const {
+ return noload_redecl_iterator();
+ }
/// Retrieve the previous declaration that declares the same entity
/// as this declaration, or NULL if there is no previous declaration.
diff --git a/clang/include/clang/AST/Redeclarable.h b/clang/include/clang/AST/Redeclarable.h
index 2003d624e..bde0e162a 100644
--- a/clang/include/clang/AST/Redeclarable.h
+++ b/clang/include/clang/AST/Redeclarable.h
@@ -130,7 +130,8 @@ protected:
}
return Update ? static_cast<decl_type *>(cast<KnownLatest>(Link).get(D))
- : static_cast<decl_type *>(cast<KnownLatest>(Link).getNotUpdated());
+ : static_cast<decl_type *>(
+ cast<KnownLatest>(Link).getNotUpdated());
}
void setPrevious(decl_type *D) {
@@ -185,11 +186,13 @@ protected:
decl_type *First;
decl_type *getNextRedeclaration() const {
- return RedeclLink.template getPrevious</*update=*/true>(static_cast<const decl_type *>(this));
+ return RedeclLink.template getPrevious</*update=*/true>(
+ static_cast<const decl_type *>(this));
}
decl_type *getNextRedeclarationNoUpdate() const {
- return RedeclLink.template getPrevious</*update=*/false>(static_cast<const decl_type *>(this));
+ return RedeclLink.template getPrevious</*update=*/false>(
+ static_cast<const decl_type *>(this));
}
public:
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 0d9edff27..d18113fec 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -8849,7 +8849,7 @@ void ASTReader::StartTranslationUnit(ASTConsumer *Consumer) {
unsigned ASTReader::getNumDeclsLoaded() const {
return DeclsLoaded.size() -
- llvm::count(DeclsLoaded.materialized(), (Decl *)nullptr);
+ llvm::count(DeclsLoaded.materialized(), (Decl *)nullptr);
}
void ASTReader::PrintStats() {
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 6e180b48e..1532e48d8 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -2116,7 +2116,9 @@ void ASTDeclMerger::MergeDefinitionData(
cast<CXXRecordDecl>(RD)->DefinitionData = ⅅ
#ifndef NDEBUG
- assert(Reader.getNumDeclsLoaded() == OldLoadedSize && "We shouldn't load new decls during merge definition data for class");
+ assert(
+ Reader.getNumDeclsLoaded() == OldLoadedSize &&
+ "We shouldn't load new decls during merge definition data for class");
#endif
return;
|
Contributor
|
I ran a build of all of our code with the assertions-enabled clang. So far I'm seeing compilation timeouts on a large number of compilations. I'm trying to figure out whether this is an infinite loop or a superlinear algorithm. |
Contributor
The stack trace after a few minutes of execution looks like this: I can look a bit closer tomorrow. |
Contributor
|
And as usual with modules: producing a shareable test case is likely to take long, since the initial size of inputs is on the order of tens of megabytes of C++ source code. |
Member
Author
|
@alexfh if it is the case, it implies that #170090 (comment) can solve the problem for you (I remember the conclusion is the reproducer will fail on trunk before) because it loads redeclarations I think you don't need to prepare reproducer for this. |
Contributor
|
The infinite loop seems to be happening in this code: |
Member
Author
|
Oh, thank you. Maybe the patch has problems already, I'll take a look later. |
Contributor
|
I debugged the code a bit more and found out that the infinite loop is due to |
Member
Author
Thanks. It's the default implementation. But the iterator should stop if it see "this". |
Contributor
Maybe this will help understanding what's happening: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
I meant to add test but I found it is hard as now we will always load every redecls after ASTReader. So I just add an assert for it.