[clang] Fix incomplete decl chains (PR #129982)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-modules

Author: Michael Park (mpark)

<details>
<summary>Changes</summary>



---
Full diff: https://github.com/llvm/llvm-project/pull/129982.diff


4 Files Affected:

- (modified) clang/lib/Sema/SemaType.cpp (+6) 
- (modified) clang/lib/Serialization/ASTReader.cpp (+12-13) 
- (added) clang/test/Modules/mpark.cpp (+107) 
- (added) clang/test/Modules/pr121245.cpp (+93) 


``````````diff
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 11943c0b53591..e2002bb410110 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -9180,6 +9180,12 @@ bool Sema::hasAcceptableDefinition(NamedDecl *D, NamedDecl **Suggested,
   if (!getLangOpts().Modules && !getLangOpts().ModulesLocalVisibility)
     return true;
 
+  // The external source may have additional definitions of this entity that are
+  // visible, so complete the redeclaration chain now.
+  if (auto *Source = Context.getExternalSource()) {
+    Source->CompleteRedeclChain(D);
+  }
+
   // If this definition was instantiated from a template, map back to the
   // pattern from which it was instantiated.
   if (isa<TagDecl>(D) && cast<TagDecl>(D)->isBeingDefined()) {
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index ca09c3d79d941..17d07f8535346 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -10178,12 +10178,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 =
@@ -10231,13 +10231,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,
@@ -10475,6 +10468,12 @@ 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();
 }
 
 void ASTReader::diagnoseOdrViolations() {
diff --git a/clang/test/Modules/mpark.cpp b/clang/test/Modules/mpark.cpp
new file mode 100644
index 0000000000000..d3b45767526f2
--- /dev/null
+++ b/clang/test/Modules/mpark.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++17 -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
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();
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/129982