Reland "Reland [Modules] Remove unnecessary check when generating name lookup table in ASTWriter" (#139253)

This relands the patch
67b298f,
with some more testcases.

The `undefined symbol` error mentioned in
#61065 (comment)
doesn't exist anymore from our internal tests.

Fixes #61065, #134739

---------

Co-authored-by: Viktoriia Bakalova <[email protected]>

Author: hokein

clang/docs/ReleaseNotes.rst

@@ -582,6 +582,7 @@ Bug Fixes in This Version
   ``#include`` directive. (#GH138094)
 - Fixed a crash during constant evaluation involving invalid lambda captures
   (#GH138832)
+- Fixed assertion failures when generating name lookup table in modules. (#GH61065, #GH134739)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

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);

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,82 +4585,55 @@ 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);
+      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;
     }
   }
 
   // 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.
-
-    // 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

clang/test/Modules/pr61065-2.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() {}

clang/test/Modules/pr61065-3.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;
+};

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
@@ -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