[C++20][Modules] Prevent premature calls to PassInterestingDeclsToConsumer() within FinishedDeserializing(). #129982
Conversation
|
@llvm/pr-subscribers-lldb @llvm/pr-subscribers-clang-modules Author: Michael Park (mpark) ChangesFull diff: https://github.com/llvm/llvm-project/pull/129982.diff 4 Files Affected:
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 11943c0b53591..e2002bb410110 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -9180,6 +9180,12 @@ bool Sema::hasAcceptableDefinition(NamedDecl *D, NamedDecl **Suggested,
if (!getLangOpts().Modules && !getLangOpts().ModulesLocalVisibility)
return true;
+ // The external source may have additional definitions of this entity that are
+ // visible, so complete the redeclaration chain now.
+ if (auto *Source = Context.getExternalSource()) {
+ Source->CompleteRedeclChain(D);
+ }
+
// If this definition was instantiated from a template, map back to the
// pattern from which it was instantiated.
if (isa<TagDecl>(D) && cast<TagDecl>(D)->isBeingDefined()) {
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index ca09c3d79d941..17d07f8535346 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -10178,12 +10178,12 @@ void ASTReader::visitTopLevelModuleMaps(
}
void ASTReader::finishPendingActions() {
- while (
- !PendingIdentifierInfos.empty() || !PendingDeducedFunctionTypes.empty() ||
- !PendingDeducedVarTypes.empty() || !PendingIncompleteDeclChains.empty() ||
- !PendingDeclChains.empty() || !PendingMacroIDs.empty() ||
- !PendingDeclContextInfos.empty() || !PendingUpdateRecords.empty() ||
- !PendingObjCExtensionIvarRedeclarations.empty()) {
+ while (!PendingIdentifierInfos.empty() ||
+ !PendingDeducedFunctionTypes.empty() ||
+ !PendingDeducedVarTypes.empty() || !PendingDeclChains.empty() ||
+ !PendingMacroIDs.empty() || !PendingDeclContextInfos.empty() ||
+ !PendingUpdateRecords.empty() ||
+ !PendingObjCExtensionIvarRedeclarations.empty()) {
// If any identifiers with corresponding top-level declarations have
// been loaded, load those declarations now.
using TopLevelDeclsMap =
@@ -10231,13 +10231,6 @@ void ASTReader::finishPendingActions() {
}
PendingDeducedVarTypes.clear();
- // For each decl chain that we wanted to complete while deserializing, mark
- // it as "still needs to be completed".
- for (unsigned I = 0; I != PendingIncompleteDeclChains.size(); ++I) {
- markIncompleteDeclChain(PendingIncompleteDeclChains[I]);
- }
- PendingIncompleteDeclChains.clear();
-
// Load pending declaration chains.
for (unsigned I = 0; I != PendingDeclChains.size(); ++I)
loadPendingDeclChain(PendingDeclChains[I].first,
@@ -10475,6 +10468,12 @@ void ASTReader::finishPendingActions() {
for (auto *ND : PendingMergedDefinitionsToDeduplicate)
getContext().deduplicateMergedDefinitonsFor(ND);
PendingMergedDefinitionsToDeduplicate.clear();
+
+ // For each decl chain that we wanted to complete while deserializing, mark
+ // it as "still needs to be completed".
+ for (Decl *D : PendingIncompleteDeclChains)
+ markIncompleteDeclChain(D);
+ PendingIncompleteDeclChains.clear();
}
void ASTReader::diagnoseOdrViolations() {
diff --git a/clang/test/Modules/mpark.cpp b/clang/test/Modules/mpark.cpp
new file mode 100644
index 0000000000000..d3b45767526f2
--- /dev/null
+++ b/clang/test/Modules/mpark.cpp
@@ -0,0 +1,107 @@
+// If this test fails, it should be investigated under Debug builds.
+// Before the PR, this test was violating an assertion.
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++17 -emit-obj -fmodules \
+// RUN: -fmodule-map-file=%t/module.modulemap \
+// RUN: -fmodules-cache-path=%t %t/a.cpp
+
+//--- module.modulemap
+module ebo {
+ header "ebo.h"
+}
+
+module fwd {
+ header "fwd.h"
+}
+
+module s {
+ header "s.h"
+ export *
+}
+
+module mod {
+ header "a.h"
+ header "b.h"
+}
+
+//--- ebo.h
+#pragma once
+
+namespace N { inline namespace __1 {
+
+template <typename T>
+struct EBO : T {
+ EBO() = default;
+};
+
+}}
+
+//--- fwd.h
+#pragma once
+
+namespace N { inline namespace __1 {
+
+template <typename T>
+struct Empty;
+
+template <typename T>
+struct BS;
+
+using S = BS<Empty<char>>;
+
+}}
+
+//--- s.h
+#pragma once
+
+#include "fwd.h"
+#include "ebo.h"
+
+namespace N { inline namespace __1 {
+
+template <typename T>
+struct Empty {};
+
+template <typename T>
+struct BS {
+ EBO<T> _;
+ void f();
+};
+
+extern template void BS<Empty<char>>::f();
+
+}}
+
+//--- b.h
+#pragma once
+
+#include "s.h"
+
+struct B {
+ void f() {
+ N::S{}.f();
+ }
+};
+
+//--- a.h
+#pragma once
+
+#include "s.h"
+
+struct A {
+ void f(int) {}
+ void f(const N::S &) {}
+
+ void g();
+};
+
+//--- a.cpp
+#include "a.h"
+
+void A::g() { f(0); }
+
+// expected-no-diagnostics
diff --git a/clang/test/Modules/pr121245.cpp b/clang/test/Modules/pr121245.cpp
new file mode 100644
index 0000000000000..0e276ad0e435d
--- /dev/null
+++ b/clang/test/Modules/pr121245.cpp
@@ -0,0 +1,93 @@
+// If this test fails, it should be investigated under Debug builds.
+// Before the PR, this test was encountering an `llvm_unreachable()`.
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+// RUN: cd %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-user-header %t/hu-01.h \
+// RUN: -fcxx-exceptions -o %t/hu-01.pcm
+
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-user-header %t/hu-02.h \
+// RUN: -Wno-experimental-header-units -fcxx-exceptions \
+// RUN: -fmodule-file=%t/hu-01.pcm -o %t/hu-02.pcm
+
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-user-header %t/hu-03.h \
+// RUN: -Wno-experimental-header-units -fcxx-exceptions \
+// RUN: -fmodule-file=%t/hu-01.pcm -o %t/hu-03.pcm
+
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-user-header %t/hu-04.h \
+// RUN: -Wno-experimental-header-units -fcxx-exceptions \
+// RUN: -fmodule-file=%t/hu-01.pcm -o %t/hu-04.pcm
+
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-user-header %t/hu-05.h \
+// RUN: -Wno-experimental-header-units -fcxx-exceptions \
+// RUN: -fmodule-file=%t/hu-03.pcm -fmodule-file=%t/hu-04.pcm \
+// RUN: -fmodule-file=%t/hu-01.pcm -o %t/hu-05.pcm
+
+// RUN: %clang_cc1 -std=c++20 -emit-obj %t/main.cpp \
+// RUN: -Wno-experimental-header-units -fcxx-exceptions \
+// RUN: -fmodule-file=%t/hu-02.pcm -fmodule-file=%t/hu-05.pcm \
+// RUN: -fmodule-file=%t/hu-04.pcm -fmodule-file=%t/hu-03.pcm \
+// RUN: -fmodule-file=%t/hu-01.pcm
+
+//--- hu-01.h
+template <typename T>
+struct A {
+ A() {}
+ ~A() {}
+};
+
+template <typename T>
+struct EBO : T {
+ EBO() = default;
+};
+
+template <typename T>
+struct HT : EBO<A<T>> {};
+
+//--- hu-02.h
+import "hu-01.h";
+
+inline void f() {
+ HT<int>();
+}
+
+//--- hu-03.h
+import "hu-01.h";
+
+struct C {
+ C();
+
+ HT<long> _;
+};
+
+//--- hu-04.h
+import "hu-01.h";
+
+void g(HT<long> = {});
+
+//--- hu-05.h
+import "hu-03.h";
+import "hu-04.h";
+import "hu-01.h";
+
+struct B {
+ virtual ~B() = default;
+
+ virtual void f() {
+ HT<long>();
+ }
+};
+
+//--- main.cpp
+import "hu-02.h";
+import "hu-05.h";
+import "hu-03.h";
+
+int main() {
+ f();
+ C();
+ B();
+}
|
|
@llvm/pr-subscribers-clang Author: Michael Park (mpark) ChangesFull diff: https://github.com/llvm/llvm-project/pull/129982.diff 4 Files Affected:
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 11943c0b53591..e2002bb410110 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -9180,6 +9180,12 @@ bool Sema::hasAcceptableDefinition(NamedDecl *D, NamedDecl **Suggested,
if (!getLangOpts().Modules && !getLangOpts().ModulesLocalVisibility)
return true;
+ // The external source may have additional definitions of this entity that are
+ // visible, so complete the redeclaration chain now.
+ if (auto *Source = Context.getExternalSource()) {
+ Source->CompleteRedeclChain(D);
+ }
+
// If this definition was instantiated from a template, map back to the
// pattern from which it was instantiated.
if (isa<TagDecl>(D) && cast<TagDecl>(D)->isBeingDefined()) {
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index ca09c3d79d941..17d07f8535346 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -10178,12 +10178,12 @@ void ASTReader::visitTopLevelModuleMaps(
}
void ASTReader::finishPendingActions() {
- while (
- !PendingIdentifierInfos.empty() || !PendingDeducedFunctionTypes.empty() ||
- !PendingDeducedVarTypes.empty() || !PendingIncompleteDeclChains.empty() ||
- !PendingDeclChains.empty() || !PendingMacroIDs.empty() ||
- !PendingDeclContextInfos.empty() || !PendingUpdateRecords.empty() ||
- !PendingObjCExtensionIvarRedeclarations.empty()) {
+ while (!PendingIdentifierInfos.empty() ||
+ !PendingDeducedFunctionTypes.empty() ||
+ !PendingDeducedVarTypes.empty() || !PendingDeclChains.empty() ||
+ !PendingMacroIDs.empty() || !PendingDeclContextInfos.empty() ||
+ !PendingUpdateRecords.empty() ||
+ !PendingObjCExtensionIvarRedeclarations.empty()) {
// If any identifiers with corresponding top-level declarations have
// been loaded, load those declarations now.
using TopLevelDeclsMap =
@@ -10231,13 +10231,6 @@ void ASTReader::finishPendingActions() {
}
PendingDeducedVarTypes.clear();
- // For each decl chain that we wanted to complete while deserializing, mark
- // it as "still needs to be completed".
- for (unsigned I = 0; I != PendingIncompleteDeclChains.size(); ++I) {
- markIncompleteDeclChain(PendingIncompleteDeclChains[I]);
- }
- PendingIncompleteDeclChains.clear();
-
// Load pending declaration chains.
for (unsigned I = 0; I != PendingDeclChains.size(); ++I)
loadPendingDeclChain(PendingDeclChains[I].first,
@@ -10475,6 +10468,12 @@ void ASTReader::finishPendingActions() {
for (auto *ND : PendingMergedDefinitionsToDeduplicate)
getContext().deduplicateMergedDefinitonsFor(ND);
PendingMergedDefinitionsToDeduplicate.clear();
+
+ // For each decl chain that we wanted to complete while deserializing, mark
+ // it as "still needs to be completed".
+ for (Decl *D : PendingIncompleteDeclChains)
+ markIncompleteDeclChain(D);
+ PendingIncompleteDeclChains.clear();
}
void ASTReader::diagnoseOdrViolations() {
diff --git a/clang/test/Modules/mpark.cpp b/clang/test/Modules/mpark.cpp
new file mode 100644
index 0000000000000..d3b45767526f2
--- /dev/null
+++ b/clang/test/Modules/mpark.cpp
@@ -0,0 +1,107 @@
+// If this test fails, it should be investigated under Debug builds.
+// Before the PR, this test was violating an assertion.
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++17 -emit-obj -fmodules \
+// RUN: -fmodule-map-file=%t/module.modulemap \
+// RUN: -fmodules-cache-path=%t %t/a.cpp
+
+//--- module.modulemap
+module ebo {
+ header "ebo.h"
+}
+
+module fwd {
+ header "fwd.h"
+}
+
+module s {
+ header "s.h"
+ export *
+}
+
+module mod {
+ header "a.h"
+ header "b.h"
+}
+
+//--- ebo.h
+#pragma once
+
+namespace N { inline namespace __1 {
+
+template <typename T>
+struct EBO : T {
+ EBO() = default;
+};
+
+}}
+
+//--- fwd.h
+#pragma once
+
+namespace N { inline namespace __1 {
+
+template <typename T>
+struct Empty;
+
+template <typename T>
+struct BS;
+
+using S = BS<Empty<char>>;
+
+}}
+
+//--- s.h
+#pragma once
+
+#include "fwd.h"
+#include "ebo.h"
+
+namespace N { inline namespace __1 {
+
+template <typename T>
+struct Empty {};
+
+template <typename T>
+struct BS {
+ EBO<T> _;
+ void f();
+};
+
+extern template void BS<Empty<char>>::f();
+
+}}
+
+//--- b.h
+#pragma once
+
+#include "s.h"
+
+struct B {
+ void f() {
+ N::S{}.f();
+ }
+};
+
+//--- a.h
+#pragma once
+
+#include "s.h"
+
+struct A {
+ void f(int) {}
+ void f(const N::S &) {}
+
+ void g();
+};
+
+//--- a.cpp
+#include "a.h"
+
+void A::g() { f(0); }
+
+// expected-no-diagnostics
diff --git a/clang/test/Modules/pr121245.cpp b/clang/test/Modules/pr121245.cpp
new file mode 100644
index 0000000000000..0e276ad0e435d
--- /dev/null
+++ b/clang/test/Modules/pr121245.cpp
@@ -0,0 +1,93 @@
+// If this test fails, it should be investigated under Debug builds.
+// Before the PR, this test was encountering an `llvm_unreachable()`.
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+// RUN: cd %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-user-header %t/hu-01.h \
+// RUN: -fcxx-exceptions -o %t/hu-01.pcm
+
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-user-header %t/hu-02.h \
+// RUN: -Wno-experimental-header-units -fcxx-exceptions \
+// RUN: -fmodule-file=%t/hu-01.pcm -o %t/hu-02.pcm
+
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-user-header %t/hu-03.h \
+// RUN: -Wno-experimental-header-units -fcxx-exceptions \
+// RUN: -fmodule-file=%t/hu-01.pcm -o %t/hu-03.pcm
+
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-user-header %t/hu-04.h \
+// RUN: -Wno-experimental-header-units -fcxx-exceptions \
+// RUN: -fmodule-file=%t/hu-01.pcm -o %t/hu-04.pcm
+
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-user-header %t/hu-05.h \
+// RUN: -Wno-experimental-header-units -fcxx-exceptions \
+// RUN: -fmodule-file=%t/hu-03.pcm -fmodule-file=%t/hu-04.pcm \
+// RUN: -fmodule-file=%t/hu-01.pcm -o %t/hu-05.pcm
+
+// RUN: %clang_cc1 -std=c++20 -emit-obj %t/main.cpp \
+// RUN: -Wno-experimental-header-units -fcxx-exceptions \
+// RUN: -fmodule-file=%t/hu-02.pcm -fmodule-file=%t/hu-05.pcm \
+// RUN: -fmodule-file=%t/hu-04.pcm -fmodule-file=%t/hu-03.pcm \
+// RUN: -fmodule-file=%t/hu-01.pcm
+
+//--- hu-01.h
+template <typename T>
+struct A {
+ A() {}
+ ~A() {}
+};
+
+template <typename T>
+struct EBO : T {
+ EBO() = default;
+};
+
+template <typename T>
+struct HT : EBO<A<T>> {};
+
+//--- hu-02.h
+import "hu-01.h";
+
+inline void f() {
+ HT<int>();
+}
+
+//--- hu-03.h
+import "hu-01.h";
+
+struct C {
+ C();
+
+ HT<long> _;
+};
+
+//--- hu-04.h
+import "hu-01.h";
+
+void g(HT<long> = {});
+
+//--- hu-05.h
+import "hu-03.h";
+import "hu-04.h";
+import "hu-01.h";
+
+struct B {
+ virtual ~B() = default;
+
+ virtual void f() {
+ HT<long>();
+ }
+};
+
+//--- main.cpp
+import "hu-02.h";
+import "hu-05.h";
+import "hu-03.h";
+
+int main() {
+ f();
+ C();
+ B();
+}
|
4187847 to
44246b4
Compare
44246b4 to
d6dbfd6
Compare
clang/lib/Sema/SemaType.cpp
Outdated
| // The external source may have additional definitions of this entity that are | ||
| // visible, so complete the redeclaration chain now. | ||
| if (auto *Source = Context.getExternalSource()) { | ||
| Source->CompleteRedeclChain(D); | ||
| } |
There was a problem hiding this comment.
the fix here looks a little bit random to me. If we need to look at other declarations, we need to call redecls(). But redecls() should complete the redecl chain, right?
There was a problem hiding this comment.
Yes, calling redecls or getMostRecentDecl (similar to CXXRecordDecl::dataPtr) here would work as well. The reason I used this "pattern" here is because of this code at the end of the same function.
I'm very much open to refining the solution. I wanted to submit this PR now because the unit-test-level repro was something we were missing from #126973
There was a problem hiding this comment.
To be honest, the completing redecl chain logic is more or less is in a mess. But let's try to do make things better if possible. I feel better to do this in redecls.
There was a problem hiding this comment.
Yeah, I agree. I'm kind of trying to figure out what the most recent pattern (😂) is for most recent decls.
Some options I've come across are:
CompleteRedeclChainifgetExternalSource()is present. e.g. SemaType.cpp (current state of the PR)
// The external source may have additional definitions of this entity that are
// visible, so complete the redeclaration chain now.
if (auto *Source = Context.getExternalSource()) {
Source->CompleteRedeclChain(D);
}- "Dummy" invocation of
getMostRecentDecl()ifgetExternalSource()is present. e.g. DeclBase.cpp
// If we have an external source, ensure that any later redeclarations of this
// context have been loaded, since they may add names to the result of this
// lookup (or add external visible storage).
ExternalASTSource *Source = getParentASTContext().getExternalSource();
if (Source)
(void)cast<Decl>(this)->getMostRecentDecl();- Unconditional "dummy" invocation of
getMostRecentDecl(). e.g. CXXRecordDecl::dataPtr, ASTReader.cpp, RecordLayoutBuilder.cpp
I'm not quite sure I fully understood what you meant by:
I feel better to do this in redecls.
We don't need to iterate through the redecls() here, and while I see dummy invocations of getMostRecentDecl as a way to trigger redecl chain completion, I don't see examples of redecls being called to force that to happen.
There was a problem hiding this comment.
Maybe some gaps here. What I asked previously is:
- According to the comments, it looks like we're trying to load all other redeclarations of the current declaration, since we want to see if any redeclaration of the current declaration is acceptable.
- In Sema, ideally, when we want to the things above, we should call
redecls().
then my question is, what is the problem?
- There is a path in Sema that meant to look at redeclarations but not called redecls().
- Redecls get called but didn't work as expceted (not work well).
- Or do I misunderstand the problem?
There was a problem hiding this comment.
On one thought, it seems like the decls inside DC should be notified of the fact that there have been new additions made to Lookups[DC].Table... but on the other hand, it's not clear to me whether this is an expected sequence of events in the first place.
If this is an expected sequence of events, I'm a bit surprised that this doesn't seem to be a bigger issue. If this is not an expected sequence of events, do we expect that the first call to CompleteRedeclChain should have the Lookups populated with everything already?
CC @ChuanqiXu9, maybe @Bigcheese can help here too?
There was a problem hiding this comment.
CC @cor3ntin, @mizvekov, @dmpolukhin since they helped look at #121245 and might have some answers around invariants/expectations here.
There was a problem hiding this comment.
@mpark thanks for the analysis. It is pretty clear. And I am wondering, why it was not a problem but it is after we delay pending the complete decl chain? Maybe this can help us to understand the problem better.
it seems like the decls inside DC should be notified of the fact that there have been new additions made to Lookups[DC].Table... but on the other hand, it's not clear to me whether this is an expected sequence of events in the first place.
I did similar experiments but it shows it has a pretty bad performance. And I don't feel it is wanted. We have similar (but not the same mechanism for hasNeedToReconcileExternalVisibleStorage())
There was a problem hiding this comment.
Thanks @ChuanqiXu9 for following up!
why it was not a problem but it is after we delay pending the complete decl chain?
I'll look into this again in more detail, but previous investigations what I know is that the RD->addedMember(MD); adds more entries to PendingIncompleteDeclChains. Somewhat surprisingly, the entries that get added are the RD portion of the call: Empty<char>. When markIncompleteDeclChain happens prior to this (the current state, after the revert), we leave finishPendingActions with Empty<char> still in PendingIncompleteDeclChains, which is not actually marked as incomplete until the next time finishPendingActions is invoked. With #121245, the markIncompleteDeclChain happens after this, which marks Empty<char> incomplete and clears out PendingIncompleteDeclChain fully before leaving finishPendingActions.
That tends to change the order of how things get processed quite a bit. One place this effects for sure is this call to getMostRecentDecl(). In the current state (where Empty<char> is left in PendingIncompleteDeclChains), when we come across that getMostRecentDecl() call it does nothing. With #121245, Empty<char> has been marked incomplete, the getMostRecentDecl() call there actually does work.
I know these are rather specific, narrow-scoped observations. I'll need to dig a bit deeper to understand the bigger picture of what really changes.
There was a problem hiding this comment.
Thanks. This is really in the dark corner.
d6dbfd6 to
3134ad3
Compare
3134ad3 to
e38c598
Compare
|
Alright folks, I've finally figured this out! I'll describe the high-level problem, what's happening specifically in the test case, and the solution. High-Level Problem
If a call to It looks something like: The known problematic part of this is that this inner The other shared data structures are also somewhat concerning at a high-level in that the inner SpecificsWe perform a look-up on Without #121245, the With #121245, the Since we're in the The implementation of
There is an instance of The claim in this fix is that the real problem is the invocation of SolutionsAs described in the High-level Problem section, the big picture problem is the recursive nature of Proposed SolutionWe already have a guard within Alternative: Save and restore the
|
e38c598 to
fb2de5d
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
fb2de5d to
af2d121
Compare
af2d121 to
20999a1
Compare
❤️
I've added a couple of bullet points in the release notes. Please let me know if this is along the lines of what's expected. |
3b29e91 to
5d21759
Compare
…ap build on OS X with XCode.
…l… (llvm#127136) This reverts commit 912b154.
…avoid entering PassInterestingDeclsToConsumer() during FinishedDeserializing().
…y pending actions.
5d21759 to
d6ebb73
Compare
|
Thank you! I can imagine figuring out what was wrong here was not easy! |
Addressing [nit comments](#129982 (comment)) from @shafik
Addressing [nit comments](llvm/llvm-project#129982 (comment)) from @shafik
Context
In
ASTReader::finishPendingActions, there is a loop that clears out pending actions. After this loop however, more stuff happen, including a call toFD->hasBody(Defn)which ends up adding stuff back intoPendingIncompleteDeclChains. As such, we leavefinishPendingActionswith pending actions remaining. This causes an assertion failure for the test casepr121245.cpp. The fix implemented in #121245 for this was to move the clearing ofPendingIncompleteDeclChainsto the end offinishPendingActions.#126973 ended up reverting #121245 because it caused a breakage of bootstrap build on Mac, using XCode. The reduced test case for this is
pr129982.cpp. The discovery here is that, in addition toFD->hasBody(Defn), the call toRD->addedMember(MD);can also add more decls toPendingIncompleteDeclChains. However, in this situation marking these newly added decls as incomplete causespr129982.cppto fail.Big Picture
There are several places in this part of the code that mention something along the lines of:
What does this mean really? and is that requirement actually satisfied by the current implementation?
The way in which a "complete redeclaration chain" is requested is by invoking
getMostRecentDecl(). There is an underlyingLazyGenerationalUpdatePtrthat invokesASTReader::CompleteRedeclChain.The start of
ASTReader::CompleteRedeclChainis:The only place that
ASTReader::finishPendingActionsis called is here:So this means that:
finishPendingActions,NumCurrentElementsDeserializingis always at least1.getMostRecentDecl()(ultimately toCompleteRedeclChain) withinfinishPendingActionswill always simply add the decl toPendingIncompleteDeclChains.But we find these 2 comments within
finishPendingActionsafter the clearing loop:Example 1:
PendingDefinitionsExample 2:
PendingBodiesBut have the redeclaration chains been fully wired at either of these points...? The clearing loop does prepare the decls to become fully wired upon request, but if that request happens within
finishPendingActions, it's just going to be added toPendingIncompleteDeclChainsanyway.The following is an example that I believe is correct:
Example 3: Exception Spec Propagation
This one is outside of
finishPendingActions, insideFinishedDeserializing, withNumCurrentElementsDeserializing == 0. As such, a call togetMostRecentDecl()here will actually perform the completion of redecl chains.With these observations, I tried moving everything after the clearing loop from
finishPendingActionstoFinishedDeserializingsuch that the comments about completing redecl chains would become accurate andfinishPendingActionswould have the post-condition that all pending actions have been cleared. This however caused 2 test failures, both of which seemed to suffer from recursion issues. During name look-up, we have some deserialization work that callsFD->hasBody(Defn)which callsgetMostRecentDecl(). Before this call togetMostRecentDecladded to thePendingIncompleteDeclChainand bailed. With the change, it actually ends up doing work which involves the name look-up of the same name we started with... and weirdness happens.