llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-modules Author: Richard Smith (zygoloid) <details> <summary>Changes</summary> Clang's lambda-merging logic was implemented in the ASTReader, meaning that it only applied for lambdas that were imported from AST files. This caused us to fail to merge lambdas in the case where both lambdas were parsed as part of the current compilation, for example if they are included into distinct headers in the same header module. Fix this by moving the merging logic out of ASTReader and into Sema, and moving the tracking of lambdas that need to be merged out of ASTReader and into ASTContext. This removes the AssignedLambdaNumbering callback from ExternalSemaSource, which was only being used for this purpose. Fixes #<!-- -->214560. Assisted-by: Gemini for the mechanical reorganization. Rework of handleLambdaNumbering done by hand. --- Full diff: https://github.com/llvm/llvm-project/pull/215661.diff 10 Files Affected: - (modified) clang/docs/ReleaseNotes.md (+3) - (modified) clang/include/clang/AST/ASTContext.h (+9) - (modified) clang/include/clang/Sema/ExternalSemaSource.h (-5) - (modified) clang/include/clang/Sema/MultiplexExternalSemaSource.h (-3) - (modified) clang/include/clang/Serialization/ASTReader.h (-6) - (modified) clang/lib/Sema/MultiplexExternalSemaSource.cpp (-6) - (modified) clang/lib/Sema/SemaLambda.cpp (+33-9) - (modified) clang/lib/Serialization/ASTReader.cpp (-23) - (modified) clang/lib/Serialization/ASTReaderDecl.cpp (+3-3) - (added) clang/test/Modules/lambda-merge-local.cpp (+18) ``````````diff diff --git a/clang/docs/ReleaseNotes.md b/clang/docs/ReleaseNotes.md index d9b9c92950c98..6bc492d157051 100644 --- a/clang/docs/ReleaseNotes.md +++ b/clang/docs/ReleaseNotes.md @@ -455,6 +455,9 @@ features cannot lower the translation-unit ABI level; affect C++26 constexpr structured bindings and expansion statements, but also affects some uses of plain structured bindings. (#GH211930) +- Fixed merging of lambdas across modules in the case where neither lambda is + imported from an AST file. (#GH214560) + #### Bug Fixes to AST Handling - Fixed a non-deterministic ordering of unused local typedefs that made diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h index 42c9aa0fcf73b..6b30a626d4861 100644 --- a/clang/include/clang/AST/ASTContext.h +++ b/clang/include/clang/AST/ASTContext.h @@ -696,6 +696,10 @@ class ASTContext : public RefCountedBase<ASTContext> { using ParameterIndexTable = llvm::DenseMap<const VarDecl *, unsigned>; ParameterIndexTable ParamIndices; + /// Map from numbering information for lambdas to the corresponding lambdas. + llvm::DenseMap<std::pair<const Decl *, unsigned>, CXXRecordDecl *> + LambdaDeclarationsForMerging; + public: struct CXXRecordDeclRelocationInfo { unsigned IsRelocatable; @@ -705,6 +709,11 @@ class ASTContext : public RefCountedBase<ASTContext> { void setRelocationInfoForCXXRecord(const CXXRecordDecl *, CXXRecordDeclRelocationInfo); + llvm::DenseMap<std::pair<const Decl *, unsigned>, CXXRecordDecl *> & + getLambdaDeclarationsForMerging() { + return LambdaDeclarationsForMerging; + } + /// Examines a given type, and returns whether the type itself /// is address discriminated, or any transitively embedded types /// contain data that is address discriminated. This includes diff --git a/clang/include/clang/Sema/ExternalSemaSource.h b/clang/include/clang/Sema/ExternalSemaSource.h index f98b0f3585c03..03e8e8a66d148 100644 --- a/clang/include/clang/Sema/ExternalSemaSource.h +++ b/clang/include/clang/Sema/ExternalSemaSource.h @@ -240,11 +240,6 @@ class ExternalSemaSource : public ExternalASTSource { return false; } - /// Notify the external source that a lambda was assigned a mangling number. - /// This enables the external source to track the correspondence between - /// lambdas and mangling numbers if necessary. - virtual void AssignedLambdaNumbering(CXXRecordDecl *Lambda) {} - /// LLVM-style RTTI. /// \{ bool isA(const void *ClassID) const override { diff --git a/clang/include/clang/Sema/MultiplexExternalSemaSource.h b/clang/include/clang/Sema/MultiplexExternalSemaSource.h index 6c117feaa47e0..8e25b6266d9ad 100644 --- a/clang/include/clang/Sema/MultiplexExternalSemaSource.h +++ b/clang/include/clang/Sema/MultiplexExternalSemaSource.h @@ -392,9 +392,6 @@ class MultiplexExternalSemaSource : public ExternalSemaSource { bool MaybeDiagnoseMissingCompleteType(SourceLocation Loc, QualType T) override; - // Inform all attached sources that a mangling number was assigned. - void AssignedLambdaNumbering(CXXRecordDecl *Lambda) override; - /// LLVM-style RTTI. /// \{ bool isA(const void *ClassID) const override { diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h index d800af83d350b..0c8c92feee176 100644 --- a/clang/include/clang/Serialization/ASTReader.h +++ b/clang/include/clang/Serialization/ASTReader.h @@ -646,10 +646,6 @@ class ASTReader : public ExternalPreprocessorSource, llvm::DenseMap<Decl*, llvm::SmallVector<NamedDecl*, 2>> AnonymousDeclarationsForMerging; - /// Map from numbering information for lambdas to the corresponding lambdas. - llvm::DenseMap<std::pair<const Decl *, unsigned>, NamedDecl *> - LambdaDeclarationsForMerging; - /// Key used to identify LifetimeExtendedTemporaryDecl for merging, /// containing the lifetime-extending declaration and the mangling number. using LETemporaryKey = std::pair<Decl *, unsigned>; @@ -2342,8 +2338,6 @@ class ASTReader : public ExternalPreprocessorSource, llvm::MapVector<const FunctionDecl *, std::unique_ptr<LateParsedTemplate>> &LPTMap) override; - void AssignedLambdaNumbering(CXXRecordDecl *Lambda) override; - /// Load a selector from disk, registering its ID if it exists. void LoadSelector(Selector Sel); diff --git a/clang/lib/Sema/MultiplexExternalSemaSource.cpp b/clang/lib/Sema/MultiplexExternalSemaSource.cpp index 49bd9596f77a7..6f6ea9f0df261 100644 --- a/clang/lib/Sema/MultiplexExternalSemaSource.cpp +++ b/clang/lib/Sema/MultiplexExternalSemaSource.cpp @@ -370,9 +370,3 @@ bool MultiplexExternalSemaSource::MaybeDiagnoseMissingCompleteType( } return false; } - -void MultiplexExternalSemaSource::AssignedLambdaNumbering( - CXXRecordDecl *Lambda) { - for (auto &Source : Sources) - Source->AssignedLambdaNumbering(Lambda); -} diff --git a/clang/lib/Sema/SemaLambda.cpp b/clang/lib/Sema/SemaLambda.cpp index bbe93f6ab8a40..2437329e458b9 100644 --- a/clang/lib/Sema/SemaLambda.cpp +++ b/clang/lib/Sema/SemaLambda.cpp @@ -516,10 +516,6 @@ void Sema::handleLambdaNumbering( // numbering state before final numbering is assigned below. if (ContextDecl) Class->setLambdaContextDecl(ContextDecl); - if (NumberingOverride) { - Class->setLambdaNumbering(*NumberingOverride); - return; - } CXXRecordDecl::LambdaNumbering Numbering; if (!MCtx && (getLangOpts().CUDA || getLangOpts().SYCLIsDevice || @@ -535,15 +531,43 @@ void Sema::handleLambdaNumbering( assert(MCtx && "Retrieving mangle numbering context failed!"); Numbering.HasKnownInternalLinkage = true; } - if (MCtx) { + + if (!MCtx) { + // This lambda doesn't need a mangle numbering. + return; + } + + if (NumberingOverride) { + Numbering = *NumberingOverride; + } else { Numbering.IndexInContext = MCtx->getNextLambdaIndex(); Numbering.ManglingNumber = MCtx->getManglingNumber(Method); Numbering.DeviceManglingNumber = MCtx->getDeviceManglingNumber(Method); - Class->setLambdaNumbering(Numbering); + } - if (auto *Source = - dyn_cast_or_null<ExternalSemaSource>(Context.getExternalSource())) - Source->AssignedLambdaNumbering(Class); + Class->setLambdaNumbering(Numbering); + + // If there is no context declaration (e.g. this lambda is defined at the + // top-level in the global namespace), there is no need to register it for + // merging. + if (!ContextDecl) { + return; + } + + auto LambdaInfo = + std::make_pair(Class->getLambdaContextDecl()->getCanonicalDecl(), + Class->getLambdaIndexInContext()); + // This lambda might redeclare a previous lambda if this is not the first + // definition of the context declaration. We might have a definition from + // another translation unit. + auto*& Slot = Context.getLambdaDeclarationsForMerging()[LambdaInfo]; + if (auto* Previous = Slot) { + Class->setPreviousDecl(Previous); + makeMergedDefinitionVisible(Previous); + } else { + // Keep track of this lambda so it can be merged with another lambda that is + // parsed or loaded later. + Slot = Class; } } diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index ffac764e46d4f..c4c4b22f85093 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -9870,29 +9870,6 @@ void ASTReader::ReadLateParsedTemplates( LateParsedTemplates.clear(); } -void ASTReader::AssignedLambdaNumbering(CXXRecordDecl *Lambda) { - if (!Lambda->getLambdaContextDecl()) - return; - - auto LambdaInfo = - std::make_pair(Lambda->getLambdaContextDecl()->getCanonicalDecl(), - Lambda->getLambdaIndexInContext()); - - // Handle the import and then include case for lambdas. - if (auto Iter = LambdaDeclarationsForMerging.find(LambdaInfo); - Iter != LambdaDeclarationsForMerging.end() && - Iter->second->isFromASTFile() && Lambda->getFirstDecl() == Lambda) { - CXXRecordDecl *Previous = - cast<CXXRecordDecl>(Iter->second)->getMostRecentDecl(); - Lambda->setPreviousDecl(Previous); - return; - } - - // Keep track of this lambda so it can be merged with another lambda that - // is loaded later. - LambdaDeclarationsForMerging.insert({LambdaInfo, Lambda}); -} - void ASTReader::LoadSelector(Selector Sel) { // It would be complicated to avoid reading the methods anyway. So don't. ReadMethodPool(Sel); diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp index 4d5c8648fe611..7e0d47c3c3dfc 100644 --- a/clang/lib/Serialization/ASTReaderDecl.cpp +++ b/clang/lib/Serialization/ASTReaderDecl.cpp @@ -2959,10 +2959,10 @@ void ASTDeclMerger::mergeLambda(CXXRecordDecl *D, RedeclarableResult &Redecl, // Look up this lambda to see if we've seen it before. If so, merge with the // one we already loaded. - NamedDecl *&Slot = Reader.LambdaDeclarationsForMerging[{ + auto *&Slot = Reader.getContext().getLambdaDeclarationsForMerging()[{ Context.getCanonicalDecl(), IndexInContext}]; - if (Slot) - mergeRedeclarable(D, cast<TagDecl>(Slot), Redecl); + if (TagDecl* PrevDecl = Slot) + mergeRedeclarable(D, PrevDecl, Redecl); else Slot = D; } diff --git a/clang/test/Modules/lambda-merge-local.cpp b/clang/test/Modules/lambda-merge-local.cpp new file mode 100644 index 0000000000000..868596cb91302 --- /dev/null +++ b/clang/test/Modules/lambda-merge-local.cpp @@ -0,0 +1,18 @@ +// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visibility -verify %s -o - + +#pragma clang module build M +module M { module X {} module Y {} } +#pragma clang module contents +#pragma clang module begin M.X +inline auto a = [] {}; +#pragma clang module end +#pragma clang module begin M.Y +inline auto a = [] {}; +#pragma clang module end +#pragma clang module endbuild + +#pragma clang module import M.X +#pragma clang module import M.Y + +//expected-no-diagnostics +void use_a() { a(); } `````````` </details> https://github.com/llvm/llvm-project/pull/215661 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
