https://github.com/hokein updated https://github.com/llvm/llvm-project/pull/139253
>From b8ba1ae30227fd7c946c5f5e57fb83e47f3bbe69 Mon Sep 17 00:00:00 2001 From: Haojian Wu <hokein...@gmail.com> Date: Fri, 9 May 2025 10:24:01 +0200 Subject: [PATCH 1/6] Reland "Reland [Modules] Remove unnecessary check when generating name lookup table in ASTWriter" This relands the patch https://github.com/llvm/llvm-project/commit/67b298f6d82e0b4bb648ac0dabe895e816a77ef1 --- clang/include/clang/Serialization/ASTWriter.h | 2 - clang/lib/Serialization/ASTWriter.cpp | 117 ++++++------------ clang/test/Modules/pr61065.cppm | 6 +- 3 files changed, 44 insertions(+), 81 deletions(-) diff --git a/clang/include/clang/Serialization/ASTWriter.h b/clang/include/clang/Serialization/ASTWriter.h index 9f0570eddc34e..cf4ae610ea51f 100644 --- a/clang/include/clang/Serialization/ASTWriter.h +++ b/clang/include/clang/Serialization/ASTWriter.h @@ -592,8 +592,6 @@ class ASTWriter : public ASTDeserializationListener, void WriteTypeAbbrevs(); void WriteType(ASTContext &Context, QualType T); - bool isLookupResultExternal(StoredDeclsList &Result, DeclContext *DC); - void GenerateSpecializationInfoLookupTable( const NamedDecl *D, llvm::SmallVectorImpl<const Decl *> &Specializations, llvm::SmallVectorImpl<char> &LookupTable, bool IsPartial); diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index 8c5adc3959398..4211a4a7275e5 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -4504,12 +4504,6 @@ uint64_t ASTWriter::WriteSpecializationInfoLookupTable( return Offset; } -bool ASTWriter::isLookupResultExternal(StoredDeclsList &Result, - DeclContext *DC) { - return Result.hasExternalDecls() && - DC->hasNeedToReconcileExternalVisibleStorage(); -} - /// Returns ture if all of the lookup result are either external, not emitted or /// predefined. In such cases, the lookup result is not interesting and we don't /// need to record the result in the current being written module. Return false @@ -4561,14 +4555,12 @@ void ASTWriter::GenerateNameLookupTable( // order. SmallVector<DeclarationName, 16> Names; - // We also build up small sets of the constructor and conversion function - // names which are visible. - llvm::SmallPtrSet<DeclarationName, 8> ConstructorNameSet, ConversionNameSet; - - for (auto &Lookup : *DC->buildLookup()) { - auto &Name = Lookup.first; - auto &Result = Lookup.second; + // We also track whether we're writing out the DeclarationNameKey for + // constructors or conversion functions. + bool IncludeConstructorNames = false; + bool IncludeConversionNames = false; + for (auto &[Name, Result] : *DC->buildLookup()) { // If there are no local declarations in our lookup result, we // don't need to write an entry for the name at all. If we can't // write out a lookup set without performing more deserialization, @@ -4577,15 +4569,8 @@ void ASTWriter::GenerateNameLookupTable( // Also in reduced BMI, we'd like to avoid writing unreachable // declarations in GMF, so we need to avoid writing declarations // that entirely external or unreachable. - // - // FIMXE: It looks sufficient to test - // isLookupResultNotInteresting here. But due to bug we have - // to test isLookupResultExternal here. See - // https://github.com/llvm/llvm-project/issues/61065 for details. - if ((GeneratingReducedBMI || isLookupResultExternal(Result, DC)) && - isLookupResultNotInteresting(*this, Result)) + if (GeneratingReducedBMI && isLookupResultNotInteresting(*this, Result)) continue; - // We also skip empty results. If any of the results could be external and // the currently available results are empty, then all of the results are // external and we skip it above. So the only way we get here with an empty @@ -4600,24 +4585,26 @@ void ASTWriter::GenerateNameLookupTable( // results for them. This in almost certainly a bug in Clang's name lookup, // but that is likely to be hard or impossible to fix and so we tolerate it // here by omitting lookups with empty results. - if (Lookup.second.getLookupResult().empty()) + if (Result.getLookupResult().empty()) continue; - switch (Lookup.first.getNameKind()) { + switch (Name.getNameKind()) { default: - Names.push_back(Lookup.first); + Names.push_back(Name); break; case DeclarationName::CXXConstructorName: - assert(isa<CXXRecordDecl>(DC) && - "Cannot have a constructor name outside of a class!"); - ConstructorNameSet.insert(Name); + // assert(isa<CXXRecordDecl>(DC) && + // "Cannot have a constructor name outside of a class!"); + // ConstructorNameSet.insert(Name); + IncludeConstructorNames = true; break; case DeclarationName::CXXConversionFunctionName: - assert(isa<CXXRecordDecl>(DC) && - "Cannot have a conversion function name outside of a class!"); - ConversionNameSet.insert(Name); + // assert(isa<CXXRecordDecl>(DC) && + // "Cannot have a conversion function name outside of a class!"); + // ConversionNameSet.insert(Name); + IncludeConversionNames = true; break; } } @@ -4625,57 +4612,35 @@ void ASTWriter::GenerateNameLookupTable( // Sort the names into a stable order. llvm::sort(Names); - if (auto *D = dyn_cast<CXXRecordDecl>(DC)) { + // if (auto *D = dyn_cast<CXXRecordDecl>(DC)) { + if (IncludeConstructorNames || IncludeConversionNames) { // We need to establish an ordering of constructor and conversion function - // names, and they don't have an intrinsic ordering. - - // First we try the easy case by forming the current context's constructor - // name and adding that name first. This is a very useful optimization to - // avoid walking the lexical declarations in many cases, and it also - // handles the only case where a constructor name can come from some other - // lexical context -- when that name is an implicit constructor merged from - // another declaration in the redecl chain. Any non-implicit constructor or - // conversion function which doesn't occur in all the lexical contexts - // would be an ODR violation. - auto ImplicitCtorName = Context.DeclarationNames.getCXXConstructorName( - Context.getCanonicalType(Context.getRecordType(D))); - if (ConstructorNameSet.erase(ImplicitCtorName)) - Names.push_back(ImplicitCtorName); - - // If we still have constructors or conversion functions, we walk all the - // names in the decl and add the constructors and conversion functions - // which are visible in the order they lexically occur within the context. - if (!ConstructorNameSet.empty() || !ConversionNameSet.empty()) - for (Decl *ChildD : cast<CXXRecordDecl>(DC)->decls()) - if (auto *ChildND = dyn_cast<NamedDecl>(ChildD)) { - auto Name = ChildND->getDeclName(); - switch (Name.getNameKind()) { - default: - continue; - - case DeclarationName::CXXConstructorName: - if (ConstructorNameSet.erase(Name)) - Names.push_back(Name); - break; + // names, and they don't have an intrinsic ordering. We also need to write + // out all constructor and conversion function results if we write out any + // of them, because they're all tracked under the same lookup key. + llvm::SmallPtrSet<DeclarationName, 8> AddedNames; + for (Decl *ChildD : cast<CXXRecordDecl>(DC)->decls()) { + if (auto *ChildND = dyn_cast<NamedDecl>(ChildD)) { + auto Name = ChildND->getDeclName(); + switch (Name.getNameKind()) { + default: + continue; - case DeclarationName::CXXConversionFunctionName: - if (ConversionNameSet.erase(Name)) - Names.push_back(Name); - break; - } + case DeclarationName::CXXConstructorName: + if (!IncludeConstructorNames) + continue; + break; - if (ConstructorNameSet.empty() && ConversionNameSet.empty()) - break; + case DeclarationName::CXXConversionFunctionName: + if (!IncludeConversionNames) + continue; + break; } - - assert(ConstructorNameSet.empty() && "Failed to find all of the visible " - "constructors by walking all the " - "lexical members of the context."); - assert(ConversionNameSet.empty() && "Failed to find all of the visible " - "conversion functions by walking all " - "the lexical members of the context."); + if (AddedNames.insert(Name).second) + Names.push_back(Name); + } + } } - // Next we need to do a lookup with each name into this decl context to fully // populate any results from external sources. We don't actually use the // results of these lookups because we only want to use the results after all diff --git a/clang/test/Modules/pr61065.cppm b/clang/test/Modules/pr61065.cppm index c79d7ac4457a1..f6a0d149c93f8 100644 --- a/clang/test/Modules/pr61065.cppm +++ b/clang/test/Modules/pr61065.cppm @@ -18,9 +18,9 @@ // RUN: %clang_cc1 -std=c++20 %t/a.cppm -emit-reduced-module-interface -o %t/a.pcm // RUN: %clang_cc1 -std=c++20 %t/b.cppm -emit-reduced-module-interface -o %t/b.pcm \ // RUN: -fprebuilt-module-path=%t -// DISABLED: %clang_cc1 -std=c++20 %t/c.cppm -emit-reduced-module-interface -o %t/c.pcm \ -// DISABLED: -fprebuilt-module-path=%t -// DISABLED: %clang_cc1 -std=c++20 %t/d.cpp -fsyntax-only -verify -fprebuilt-module-path=%t +// RUN: %clang_cc1 -std=c++20 %t/c.cppm -emit-reduced-module-interface -o %t/c.pcm \ +// RUN: -fprebuilt-module-path=%t +// RUN: %clang_cc1 -std=c++20 %t/d.cpp -fsyntax-only -verify -fprebuilt-module-path=%t //--- a.cppm >From 7ca897c038a8869ac3c46c37c228096d0853667d Mon Sep 17 00:00:00 2001 From: Viktoriia Bakalova <bakal...@google.com> Date: Mon, 12 May 2025 15:18:05 +0200 Subject: [PATCH 2/6] Add test cases. --- clang/test/Modules/GH61065-2.cppm | 34 +++++++++++++++++++++++++ clang/test/Modules/GH61065-3.cppm | 42 +++++++++++++++++++++++++++++++ clang/test/Modules/GH61065.cppm | 15 +++++++++++ 3 files changed, 91 insertions(+) create mode 100644 clang/test/Modules/GH61065-2.cppm create mode 100644 clang/test/Modules/GH61065-3.cppm create mode 100644 clang/test/Modules/GH61065.cppm diff --git a/clang/test/Modules/GH61065-2.cppm b/clang/test/Modules/GH61065-2.cppm new file mode 100644 index 0000000000000..57a1f1102ff63 --- /dev/null +++ b/clang/test/Modules/GH61065-2.cppm @@ -0,0 +1,34 @@ +// RUN: rm -rf %t +// RUN: mkdir %t +// RUN: split-file %s %t +// +// RUN: %clang -std=c++20 -Wno-experimental-header-units -fmodule-header %t/RelaxedAtomic.h -o %t/RelaxedAtomic.pcm +// RUN: %clang -std=c++20 -Wno-experimental-header-units -fmodule-header -fmodule-file=%t/RelaxedAtomic.pcm %t/SharedMutex.h -o %t/SharedMutex.pcm +// RUN: %clang -std=c++20 -Wno-experimental-header-units -fmodule-header -fmodule-file=%t/SharedMutex.pcm -fmodule-file=%t/RelaxedAtomic.pcm %t/ThreadLocalDetail.h -o %t/ThreadLocalDetail.pcm +//--- RelaxedAtomic.h +struct relaxed_atomic_base { + relaxed_atomic_base(int) {} +}; + +struct relaxed_atomic : relaxed_atomic_base { + using relaxed_atomic_base::relaxed_atomic_base; // constructor +}; + +//--- SharedMutex.h +import "RelaxedAtomic.h"; + +inline void getMaxDeferredReaders() { + static relaxed_atomic cache{0}; +} + +//--- ThreadLocalDetail.h +import "RelaxedAtomic.h"; + +struct noncopyable { + noncopyable(const noncopyable&) = delete; +}; + +struct StaticMetaBase { + relaxed_atomic nextId_{0}; + noncopyable ncp; +}; diff --git a/clang/test/Modules/GH61065-3.cppm b/clang/test/Modules/GH61065-3.cppm new file mode 100644 index 0000000000000..e19a7a9616d71 --- /dev/null +++ b/clang/test/Modules/GH61065-3.cppm @@ -0,0 +1,42 @@ +// RUN: rm -rf %t +// RUN: mkdir %t +// RUN: split-file %s %t +// +// RUN: %clang -std=c++20 -x c++-module --precompile -c %t/a.cpp -o %t/a.pcm +// RUN: %clang -std=c++20 -fmodule-file=a=%t/a.pcm --precompile -x c++-module -c %t/b.cpp -o %t/b.pcm +// RUN: %clang -std=c++20 -fmodule-file=a=%t/a.pcm -fmodule-file=b=%t/b.pcm -x c++-module --precompile -c %t/c.cpp -o /dev/null + +//--- a.cpp +export module a; + +struct base { + base(int) {} +}; + +export struct a : base { + using base::base; +}; + +//--- b.cpp +export module b; + +import a; + +a b() { + return a(1); +} + +//--- c.cpp +export module c; + +import a; +import b; + +struct noncopyable { + noncopyable(noncopyable const &) = delete; +}; + +struct c { + noncopyable c0; + a c1; +}; \ No newline at end of file diff --git a/clang/test/Modules/GH61065.cppm b/clang/test/Modules/GH61065.cppm new file mode 100644 index 0000000000000..8370ad93b55ed --- /dev/null +++ b/clang/test/Modules/GH61065.cppm @@ -0,0 +1,15 @@ +// RUN: rm -rf %t +// RUN: mkdir %t +// RUN: split-file %s %t +// +// RUN: %clang_cc1 -fallow-pcm-with-compiler-errors -fmodule-name=c -xc++ -emit-module -fmodules -std=gnu++20 %t/a.cppmap -o %t/c.pcm +//--- a.cppmap +module "c" { + header "a.h" +} + +//--- a.h +template <class> +class C {}; +template <class T> +C<T>::operator C() {} \ No newline at end of file >From 47468e0b50270a2778b73d72352d4c6cec00dcbc Mon Sep 17 00:00:00 2001 From: Viktoriia Bakalova <bakal...@google.com> Date: Mon, 12 May 2025 15:21:53 +0200 Subject: [PATCH 3/6] Minor adjustments. --- clang/test/Modules/GH61065-3.cppm | 42 ------------------- .../Modules/{GH61065.cppm => pr61065-2.cppm} | 0 .../{GH61065-2.cppm => pr61065-3.cppm} | 0 clang/test/Modules/pr61065.cppm | 6 +-- 4 files changed, 3 insertions(+), 45 deletions(-) delete mode 100644 clang/test/Modules/GH61065-3.cppm rename clang/test/Modules/{GH61065.cppm => pr61065-2.cppm} (100%) rename clang/test/Modules/{GH61065-2.cppm => pr61065-3.cppm} (100%) diff --git a/clang/test/Modules/GH61065-3.cppm b/clang/test/Modules/GH61065-3.cppm deleted file mode 100644 index e19a7a9616d71..0000000000000 --- a/clang/test/Modules/GH61065-3.cppm +++ /dev/null @@ -1,42 +0,0 @@ -// RUN: rm -rf %t -// RUN: mkdir %t -// RUN: split-file %s %t -// -// RUN: %clang -std=c++20 -x c++-module --precompile -c %t/a.cpp -o %t/a.pcm -// RUN: %clang -std=c++20 -fmodule-file=a=%t/a.pcm --precompile -x c++-module -c %t/b.cpp -o %t/b.pcm -// RUN: %clang -std=c++20 -fmodule-file=a=%t/a.pcm -fmodule-file=b=%t/b.pcm -x c++-module --precompile -c %t/c.cpp -o /dev/null - -//--- a.cpp -export module a; - -struct base { - base(int) {} -}; - -export struct a : base { - using base::base; -}; - -//--- b.cpp -export module b; - -import a; - -a b() { - return a(1); -} - -//--- c.cpp -export module c; - -import a; -import b; - -struct noncopyable { - noncopyable(noncopyable const &) = delete; -}; - -struct c { - noncopyable c0; - a c1; -}; \ No newline at end of file diff --git a/clang/test/Modules/GH61065.cppm b/clang/test/Modules/pr61065-2.cppm similarity index 100% rename from clang/test/Modules/GH61065.cppm rename to clang/test/Modules/pr61065-2.cppm diff --git a/clang/test/Modules/GH61065-2.cppm b/clang/test/Modules/pr61065-3.cppm similarity index 100% rename from clang/test/Modules/GH61065-2.cppm rename to clang/test/Modules/pr61065-3.cppm diff --git a/clang/test/Modules/pr61065.cppm b/clang/test/Modules/pr61065.cppm index f6a0d149c93f8..98e4988cd7174 100644 --- a/clang/test/Modules/pr61065.cppm +++ b/clang/test/Modules/pr61065.cppm @@ -6,9 +6,9 @@ // RUN: %clang_cc1 -std=c++20 %t/a.cppm -emit-module-interface -o %t/a.pcm // RUN: %clang_cc1 -std=c++20 %t/b.cppm -emit-module-interface -o %t/b.pcm \ // RUN: -fprebuilt-module-path=%t -// DISABLED: %clang_cc1 -std=c++20 %t/c.cppm -emit-module-interface -o %t/c.pcm \ -// DISABLED: -fprebuilt-module-path=%t -// DISABLED: %clang_cc1 -std=c++20 %t/d.cpp -fsyntax-only -verify -fprebuilt-module-path=%t +// RUN: %clang_cc1 -std=c++20 %t/c.cppm -emit-module-interface -o %t/c.pcm \ +// RUN: -fprebuilt-module-path=%t +// RUN: %clang_cc1 -std=c++20 %t/d.cpp -fsyntax-only -verify -fprebuilt-module-path=%t // Test again with reduced BMI // RUN: rm -rf %t >From 49c38568161a245a251e4c7fa7ebb259f8d4e08f Mon Sep 17 00:00:00 2001 From: Viktoriia Bakalova <bakal...@google.com> Date: Mon, 12 May 2025 15:22:35 +0200 Subject: [PATCH 4/6] Minor adjustments. --- clang/test/Modules/pr61065-2.cppm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/test/Modules/pr61065-2.cppm b/clang/test/Modules/pr61065-2.cppm index 8370ad93b55ed..f46a0f73aa203 100644 --- a/clang/test/Modules/pr61065-2.cppm +++ b/clang/test/Modules/pr61065-2.cppm @@ -12,4 +12,4 @@ module "c" { template <class> class C {}; template <class T> -C<T>::operator C() {} \ No newline at end of file +C<T>::operator C() {} >From a55894b454acdb93638ae7393f7010da440342e3 Mon Sep 17 00:00:00 2001 From: Haojian Wu <hokein...@gmail.com> Date: Mon, 12 May 2025 15:56:10 +0200 Subject: [PATCH 5/6] Cleanup on ASTWriter.cpp --- clang/lib/Serialization/ASTWriter.cpp | 7 ------- 1 file changed, 7 deletions(-) diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index 4211a4a7275e5..c5ad05b525bf9 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -4594,16 +4594,10 @@ void ASTWriter::GenerateNameLookupTable( break; case DeclarationName::CXXConstructorName: - // assert(isa<CXXRecordDecl>(DC) && - // "Cannot have a constructor name outside of a class!"); - // ConstructorNameSet.insert(Name); IncludeConstructorNames = true; break; case DeclarationName::CXXConversionFunctionName: - // assert(isa<CXXRecordDecl>(DC) && - // "Cannot have a conversion function name outside of a class!"); - // ConversionNameSet.insert(Name); IncludeConversionNames = true; break; } @@ -4612,7 +4606,6 @@ void ASTWriter::GenerateNameLookupTable( // Sort the names into a stable order. llvm::sort(Names); - // if (auto *D = dyn_cast<CXXRecordDecl>(DC)) { if (IncludeConstructorNames || IncludeConversionNames) { // We need to establish an ordering of constructor and conversion function // names, and they don't have an intrinsic ordering. We also need to write >From 6369313a057ce3b8833ddd44509dd5819b3ba834 Mon Sep 17 00:00:00 2001 From: Haojian Wu <hokein...@gmail.com> Date: Mon, 12 May 2025 16:01:00 +0200 Subject: [PATCH 6/6] Add release notes --- clang/docs/ReleaseNotes.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 918ff952bb2c3..80c2b37a869db 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -551,6 +551,7 @@ Bug Fixes in This Version - Fixed a bug where an attribute before a ``pragma clang attribute`` or ``pragma clang __debug`` would cause an assertion. Instead, this now diagnoses the invalid attribute location appropriately. (#GH137861) +- Fixed assertion failures when generating name lookup table in modules. (#GH61065, #GH134739) Bug Fixes to Compiler Builtins ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
