[clang] 0689d23 - [C++20][Modules] Prevent premature calls to PassInterestingDeclsToConsumer() within FinishedDeserializing(). (#129982)
via cfe-commits
cfe-commits at lists.llvm.org
Sat Mar 15 23:03:23 PDT 2025
Author: Michael Park
Date: 2025-03-15T23:03:20-07:00
New Revision: 0689d23ab3089eb9920b8f5caa92e423fe3475f8
URL: https://github.com/llvm/llvm-project/commit/0689d23ab3089eb9920b8f5caa92e423fe3475f8
DIFF: https://github.com/llvm/llvm-project/commit/0689d23ab3089eb9920b8f5caa92e423fe3475f8.diff
LOG: [C++20][Modules] Prevent premature calls to PassInterestingDeclsToConsumer() within FinishedDeserializing(). (#129982)
`ASTReader::FinishedDeserializing` uses `NumCurrentElementsDeserializing` to keep track of nested `Deserializing` RAII actions. The `FinishedDeserializing` only performs actions if it is the top-level `Deserializing` layer. This works fine in general, but there is a problematic edge case.
If a call to `redecls()` in `FinishedDeserializing` performs deserialization, we re-enter `FinishedDeserializing` while in the middle of the previous `FinishedDeserializing` call.
The known problematic part of this is that this inner `FinishedDeserializing` can go all the way to `PassInterestingDeclsToConsumer`, which operates on `PotentiallyInterestingDecls` data structure which contain decls that should be handled by the previous `FinishedDeserializing` stage.
The other shared data structures are also somewhat concerning at a high-level in that the inner `FinishedDeserializing` would be handling pending actions that are not "within its scope", but this part is not known to be problematic.
We already have a guard within `PassInterestingDeclsToConsumer` because we can end up with recursive deserialization within `PassInterestingDeclsToConsumer`. The implemented solution is to apply this guard to the portion of `FinishedDeserializing` that performs further deserialization as well. This ensures that recursive deserialization does not trigger `PassInterestingDeclsToConsumer` which may operate on entries that are not ready to be passed.
Added:
clang/test/Modules/pr121245.cpp
clang/test/Modules/pr129982.cpp
Modified:
clang/docs/ReleaseNotes.rst
clang/include/clang/Serialization/ASTReader.h
clang/lib/Serialization/ASTReader.cpp
clang/lib/Serialization/ASTReaderDecl.cpp
Removed:
################################################################################
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 07b8214246901..2a1c5ee2d788e 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -278,6 +278,9 @@ Bug Fixes in This Version
considered an error in C23 mode and are allowed as an extension in earlier language modes.
- Remove the ``static`` specifier for the value of ``_FUNCTION_`` for static functions, in MSVC compatibility mode.
+- Fixed a modules crash where exception specifications were not propagated properly (#GH121245, relanded in #GH129982)
+- Fixed a problematic case with recursive deserialization within ``FinishedDeserializing()`` where
+ ``PassInterestingDeclsToConsumer()`` was called before the declarations were safe to be passed. (#GH129982)
Bug Fixes to Compiler Builtins
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
index 143b798a8348a..2779b3d1cf2ea 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -1176,11 +1176,11 @@ class ASTReader
/// Number of Decl/types that are currently deserializing.
unsigned NumCurrentElementsDeserializing = 0;
- /// Set true while we are in the process of passing deserialized
- /// "interesting" decls to consumer inside FinishedDeserializing().
- /// This is used as a guard to avoid recursively repeating the process of
+ /// Set false while we are in a state where we cannot safely pass deserialized
+ /// "interesting" decls to the consumer inside FinishedDeserializing().
+ /// This is used as a guard to avoid recursively entering the process of
/// passing decls to consumer.
- bool PassingDeclsToConsumer = false;
+ bool CanPassDeclsToConsumer = true;
/// The set of identifiers that were read while the AST reader was
/// (recursively) loading declarations.
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 8e9978829c512..2c73501821bff 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -10182,12 +10182,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 =
@@ -10235,13 +10235,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,
@@ -10479,6 +10472,43 @@ 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();
+
+ assert(PendingIdentifierInfos.empty() &&
+ "Should be empty at the end of finishPendingActions");
+ assert(PendingDeducedFunctionTypes.empty() &&
+ "Should be empty at the end of finishPendingActions");
+ assert(PendingDeducedVarTypes.empty() &&
+ "Should be empty at the end of finishPendingActions");
+ assert(PendingDeclChains.empty() &&
+ "Should be empty at the end of finishPendingActions");
+ assert(PendingMacroIDs.empty() &&
+ "Should be empty at the end of finishPendingActions");
+ assert(PendingDeclContextInfos.empty() &&
+ "Should be empty at the end of finishPendingActions");
+ assert(PendingUpdateRecords.empty() &&
+ "Should be empty at the end of finishPendingActions");
+ assert(PendingObjCExtensionIvarRedeclarations.empty() &&
+ "Should be empty at the end of finishPendingActions");
+ assert(PendingFakeDefinitionData.empty() &&
+ "Should be empty at the end of finishPendingActions");
+ assert(PendingDefinitions.empty() &&
+ "Should be empty at the end of finishPendingActions");
+ assert(PendingWarningForDuplicatedDefsInModuleUnits.empty() &&
+ "Should be empty at the end of finishPendingActions");
+ assert(PendingBodies.empty() &&
+ "Should be empty at the end of finishPendingActions");
+ assert(PendingAddedClassMembers.empty() &&
+ "Should be empty at the end of finishPendingActions");
+ assert(PendingMergedDefinitionsToDeduplicate.empty() &&
+ "Should be empty at the end of finishPendingActions");
+ assert(PendingIncompleteDeclChains.empty() &&
+ "Should be empty at the end of finishPendingActions");
}
void ASTReader::diagnoseOdrViolations() {
@@ -10792,47 +10822,54 @@ void ASTReader::FinishedDeserializing() {
--NumCurrentElementsDeserializing;
if (NumCurrentElementsDeserializing == 0) {
- // Propagate exception specification and deduced type updates along
- // redeclaration chains.
- //
- // We do this now rather than in finishPendingActions because we want to
- // be able to walk the complete redeclaration chains of the updated decls.
- while (!PendingExceptionSpecUpdates.empty() ||
- !PendingDeducedTypeUpdates.empty() ||
- !PendingUndeducedFunctionDecls.empty()) {
- auto ESUpdates = std::move(PendingExceptionSpecUpdates);
- PendingExceptionSpecUpdates.clear();
- for (auto Update : ESUpdates) {
- ProcessingUpdatesRAIIObj ProcessingUpdates(*this);
- auto *FPT = Update.second->getType()->castAs<FunctionProtoType>();
- auto ESI = FPT->getExtProtoInfo().ExceptionSpec;
- if (auto *Listener = getContext().getASTMutationListener())
- Listener->ResolvedExceptionSpec(cast<FunctionDecl>(Update.second));
- for (auto *Redecl : Update.second->redecls())
- getContext().adjustExceptionSpec(cast<FunctionDecl>(Redecl), ESI);
- }
+ {
+ // Guard variable to avoid recursively entering the process of passing
+ // decls to consumer.
+ SaveAndRestore GuardPassingDeclsToConsumer(CanPassDeclsToConsumer, false);
- auto DTUpdates = std::move(PendingDeducedTypeUpdates);
- PendingDeducedTypeUpdates.clear();
- for (auto Update : DTUpdates) {
- ProcessingUpdatesRAIIObj ProcessingUpdates(*this);
- // FIXME: If the return type is already deduced, check that it matches.
- getContext().adjustDeducedFunctionResultType(Update.first,
- Update.second);
- }
+ // Propagate exception specification and deduced type updates along
+ // redeclaration chains.
+ //
+ // We do this now rather than in finishPendingActions because we want to
+ // be able to walk the complete redeclaration chains of the updated decls.
+ while (!PendingExceptionSpecUpdates.empty() ||
+ !PendingDeducedTypeUpdates.empty() ||
+ !PendingUndeducedFunctionDecls.empty()) {
+ auto ESUpdates = std::move(PendingExceptionSpecUpdates);
+ PendingExceptionSpecUpdates.clear();
+ for (auto Update : ESUpdates) {
+ ProcessingUpdatesRAIIObj ProcessingUpdates(*this);
+ auto *FPT = Update.second->getType()->castAs<FunctionProtoType>();
+ auto ESI = FPT->getExtProtoInfo().ExceptionSpec;
+ if (auto *Listener = getContext().getASTMutationListener())
+ Listener->ResolvedExceptionSpec(cast<FunctionDecl>(Update.second));
+ for (auto *Redecl : Update.second->redecls())
+ getContext().adjustExceptionSpec(cast<FunctionDecl>(Redecl), ESI);
+ }
- auto UDTUpdates = std::move(PendingUndeducedFunctionDecls);
- PendingUndeducedFunctionDecls.clear();
- // We hope we can find the deduced type for the functions by iterating
- // redeclarations in other modules.
- for (FunctionDecl *UndeducedFD : UDTUpdates)
- (void)UndeducedFD->getMostRecentDecl();
- }
+ auto DTUpdates = std::move(PendingDeducedTypeUpdates);
+ PendingDeducedTypeUpdates.clear();
+ for (auto Update : DTUpdates) {
+ ProcessingUpdatesRAIIObj ProcessingUpdates(*this);
+ // FIXME: If the return type is already deduced, check that it
+ // matches.
+ getContext().adjustDeducedFunctionResultType(Update.first,
+ Update.second);
+ }
- if (ReadTimer)
- ReadTimer->stopTimer();
+ auto UDTUpdates = std::move(PendingUndeducedFunctionDecls);
+ PendingUndeducedFunctionDecls.clear();
+ // We hope we can find the deduced type for the functions by iterating
+ // redeclarations in other modules.
+ for (FunctionDecl *UndeducedFD : UDTUpdates)
+ (void)UndeducedFD->getMostRecentDecl();
+ }
+
+ if (ReadTimer)
+ ReadTimer->stopTimer();
- diagnoseOdrViolations();
+ diagnoseOdrViolations();
+ }
// We are not in recursive loading, so it's safe to pass the "interesting"
// decls to the consumer.
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 262cb0f63b462..79bd41aa2644e 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -4309,12 +4309,12 @@ Decl *ASTReader::ReadDeclRecord(GlobalDeclID ID) {
void ASTReader::PassInterestingDeclsToConsumer() {
assert(Consumer);
- if (PassingDeclsToConsumer)
+ if (!CanPassDeclsToConsumer)
return;
// Guard variable to avoid recursively redoing the process of passing
// decls to consumer.
- SaveAndRestore GuardPassingDeclsToConsumer(PassingDeclsToConsumer, true);
+ SaveAndRestore GuardPassingDeclsToConsumer(CanPassDeclsToConsumer, false);
// Ensure that we've loaded all potentially-interesting declarations
// that need to be eagerly loaded.
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();
+}
diff --git a/clang/test/Modules/pr129982.cpp b/clang/test/Modules/pr129982.cpp
new file mode 100644
index 0000000000000..b19cccfd07687
--- /dev/null
+++ b/clang/test/Modules/pr129982.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++20 -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
More information about the cfe-commits
mailing list