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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
