[clang] 013b345 - [Serialization] Stop demote var definition as declaration (#172430) (#177117)
via cfe-commits
cfe-commits at lists.llvm.org
Sun Feb 1 23:24:02 PST 2026
Author: Chuanqi Xu
Date: 2026-02-02T15:23:57+08:00
New Revision: 013b345af992f66d5ecfd168844ebfc6956ccae0
URL: https://github.com/llvm/llvm-project/commit/013b345af992f66d5ecfd168844ebfc6956ccae0
DIFF: https://github.com/llvm/llvm-project/commit/013b345af992f66d5ecfd168844ebfc6956ccae0.diff
LOG: [Serialization] Stop demote var definition as declaration (#172430) (#177117)
Close https://github.com/llvm/llvm-project/issues/172241
Close https://github.com/llvm/llvm-project/issues/64034
Close https://github.com/llvm/llvm-project/issues/149404
Close https://github.com/llvm/llvm-project/issues/174858
After this patch, we (the clang dev) no longer assumes there are at most
one definition in a redeclaration chain.
See
https://discourse.llvm.org/t/rfc-clang-not-assuming-there-is-at-most-one-definition-in-a-redeclaration-chain/89360
for details.
---
Update since last commit:
Previously I remove the code to update visibility accidently. This is
the root cause of the failure.
Added:
clang/test/Modules/demote-var-def.cpp
clang/test/Modules/pr149404-02.cppm
clang/test/Modules/pr172241.cppm
clang/test/Modules/var-inst-def.cppm
Modified:
clang/lib/Sema/SemaType.cpp
clang/lib/Serialization/ASTReaderDecl.cpp
Removed:
################################################################################
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index fa4dcdd9e1422..28d1d63ff7acf 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -9282,11 +9282,59 @@ bool Sema::hasAcceptableDefinition(NamedDecl *D, NamedDecl **Suggested,
// 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()) {
+ if (isa<TagDecl>(D) && cast<TagDecl>(D)->isBeingDefined())
// We're in the middle of defining it; this definition should be treated
// as visible.
return true;
- } else if (auto *RD = dyn_cast<CXXRecordDecl>(D)) {
+
+ auto DefinitionIsAcceptable = [&](NamedDecl *D) {
+ // The (primary) definition might be in a visible module.
+ if (isAcceptable(D, Kind))
+ return true;
+
+ // A visible module might have a merged definition instead.
+ if (D->isModulePrivate() ? hasMergedDefinitionInCurrentModule(D)
+ : hasVisibleMergedDefinition(D)) {
+ if (CodeSynthesisContexts.empty() &&
+ !getLangOpts().ModulesLocalVisibility) {
+ // Cache the fact that this definition is implicitly visible because
+ // there is a visible merged definition.
+ D->setVisibleDespiteOwningModule();
+ }
+ return true;
+ }
+
+ return false;
+ };
+ auto IsDefinition = [](NamedDecl *D) {
+ if (auto *RD = dyn_cast<CXXRecordDecl>(D))
+ return RD->isThisDeclarationADefinition();
+ if (auto *ED = dyn_cast<EnumDecl>(D))
+ return ED->isThisDeclarationADefinition();
+ if (auto *FD = dyn_cast<FunctionDecl>(D))
+ return FD->isThisDeclarationADefinition();
+ if (auto *VD = dyn_cast<VarDecl>(D))
+ return VD->isThisDeclarationADefinition() == VarDecl::Definition;
+ llvm_unreachable("unexpected decl type");
+ };
+ auto FoundAcceptableDefinition = [&](NamedDecl *D) {
+ if (!isa<CXXRecordDecl, FunctionDecl, EnumDecl, VarDecl>(D))
+ return DefinitionIsAcceptable(D);
+
+ for (auto *RD : D->redecls()) {
+ auto *ND = cast<NamedDecl>(RD);
+ if (!IsDefinition(ND))
+ continue;
+ if (DefinitionIsAcceptable(ND)) {
+ *Suggested = ND;
+ return true;
+ }
+ }
+
+ return false;
+ };
+
+ if (auto *RD = dyn_cast<CXXRecordDecl>(D)) {
if (auto *Pattern = RD->getTemplateInstantiationPattern())
RD = Pattern;
D = RD->getDefinition();
@@ -9325,34 +9373,14 @@ bool Sema::hasAcceptableDefinition(NamedDecl *D, NamedDecl **Suggested,
*Suggested = D;
- auto DefinitionIsAcceptable = [&] {
- // The (primary) definition might be in a visible module.
- if (isAcceptable(D, Kind))
- return true;
-
- // A visible module might have a merged definition instead.
- if (D->isModulePrivate() ? hasMergedDefinitionInCurrentModule(D)
- : hasVisibleMergedDefinition(D)) {
- if (CodeSynthesisContexts.empty() &&
- !getLangOpts().ModulesLocalVisibility) {
- // Cache the fact that this definition is implicitly visible because
- // there is a visible merged definition.
- D->setVisibleDespiteOwningModule();
- }
- return true;
- }
-
- return false;
- };
-
- if (DefinitionIsAcceptable())
+ if (FoundAcceptableDefinition(D))
return true;
// The external source may have additional definitions of this entity that are
// visible, so complete the redeclaration chain now and ask again.
if (auto *Source = Context.getExternalSource()) {
Source->CompleteRedeclChain(D);
- return DefinitionIsAcceptable();
+ return FoundAcceptableDefinition(D);
}
return false;
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index f8e9caa3f5d1d..f0fb247f1afb9 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -3642,23 +3642,9 @@ template<>
void ASTDeclReader::attachPreviousDeclImpl(ASTReader &Reader,
Redeclarable<VarDecl> *D,
Decl *Previous, Decl *Canon) {
- auto *VD = static_cast<VarDecl *>(D);
auto *PrevVD = cast<VarDecl>(Previous);
D->RedeclLink.setPrevious(PrevVD);
D->First = PrevVD->First;
-
- // We should keep at most one definition on the chain.
- // FIXME: Cache the definition once we've found it. Building a chain with
- // N definitions currently takes O(N^2) time here.
- if (VD->isThisDeclarationADefinition() == VarDecl::Definition) {
- for (VarDecl *CurD = PrevVD; CurD; CurD = CurD->getPreviousDecl()) {
- if (CurD->isThisDeclarationADefinition() == VarDecl::Definition) {
- Reader.mergeDefinitionVisibility(CurD, VD);
- VD->demoteThisDefinitionToDeclaration();
- break;
- }
- }
- }
}
static bool isUndeducedReturnType(QualType T) {
diff --git a/clang/test/Modules/demote-var-def.cpp b/clang/test/Modules/demote-var-def.cpp
new file mode 100644
index 0000000000000..811440dd736f2
--- /dev/null
+++ b/clang/test/Modules/demote-var-def.cpp
@@ -0,0 +1,94 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// DEFINE: %{common-flags}= -I %t -isystem %t -xc++ -std=c++20 -fmodules
+//
+// RUN: mkdir -p %t/b2
+// RUN: mkdir -p %t/b1
+// RUN: %clang_cc1 %{common-flags} -emit-module -fmodule-name=module_d \
+// RUN: d.cppmap -o d.pcm
+// RUN: %clang_cc1 %{common-flags} -emit-module -fmodule-name=module_a \
+// RUN: -fmodule-file=d.pcm a.cppmap -o a.pcm
+// RUN: %clang_cc1 %{common-flags} -emit-module -fmodule-name=module_b2 \
+// RUN: -fmodule-file=a.pcm b2/b.cppmap -o b2/b.pcm
+// RUN: %clang_cc1 %{common-flags} -emit-module -fmodule-name=module_b1 \
+// RUN: -fmodule-file=b2/b.pcm b1/b.cppmap -o b1/b.pcm
+// RUN: %clang_cc1 %{common-flags} -emit-module -fmodule-name=module_f \
+// RUN: -fmodule-file=b1/b.pcm f.cppmap -o f.pcm
+// RUN: %clang_cc1 %{common-flags} -emit-module -fmodule-name=module_c \
+// RUN: -fmodule-file=f.pcm c.cppmap -o c.pcm
+// RUN: %clang_cc1 %{common-flags} -emit-module \
+// RUN: -fmodule-name=module_e e.cppmap -o e.pcm
+//
+// RUN: %clang_cc1 %{common-flags} \
+// RUN: -fmodule-file=c.pcm -fmodule-file=e.pcm \
+// RUN: src.cpp -o src.pic.o
+
+//--- invoke.h
+#ifndef _LIBCPP___TYPE_TRAITS_IS_SAME_H
+#define _LIBCPP___TYPE_TRAITS_IS_SAME_H
+namespace std { inline namespace _LIBCPP_ABI_NAMESPACE {
+template <class _Tp, class _Up>
+constexpr bool is_same_v = __is_same(_Tp, _Up);
+} }
+#endif
+
+//--- memory
+#include <invoke.h>
+namespace std { inline namespace _LIBCPP_ABI_NAMESPACE {
+template <class _Tp>
+using __decay_t = __decay(_Tp);
+template <class _Tp>
+using decay_t = __decay_t<_Tp>;
+} }
+
+//--- other.h
+#include <invoke.h>
+
+//--- a.cppmap
+module "module_a" {
+}
+
+//--- b1/b.cppmap
+module "module_b1" {
+}
+
+//--- b2/b.cppmap
+module "module_b2" {
+}
+
+//--- c.cppmap
+module "module_c" {
+}
+
+//--- d.cppmap
+module "module_d" {
+ header "d.h"
+}
+
+//--- d.h
+#include <other.h>
+
+//--- e.cppmap
+module "module_e" {
+ header "e.h"
+}
+
+//--- e.h
+#include <memory>
+
+//--- f.cppmap
+module "module_f" {
+}
+
+//--- src.cpp
+#include <d.h>
+#include <memory>
+template <typename T>
+concept coroutine_result =
+ std::is_same_v<std::decay_t<T>, T>;
+template <coroutine_result R>
+class Co;
+using T = Co<void>;
diff --git a/clang/test/Modules/pr149404-02.cppm b/clang/test/Modules/pr149404-02.cppm
new file mode 100644
index 0000000000000..291619ea05b8a
--- /dev/null
+++ b/clang/test/Modules/pr149404-02.cppm
@@ -0,0 +1,104 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -o %t/format.pcm %t/format.cppm
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -o %t/includes_in_gmf.pcm %t/includes_in_gmf.cppm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/test.cpp -verify -fsyntax-only
+
+// RUN: %clang_cc1 -std=c++20 -emit-reduced-module-interface -o %t/format.pcm %t/format.cppm
+// RUN: %clang_cc1 -std=c++20 -emit-reduced-module-interface -o %t/includes_in_gmf.pcm %t/includes_in_gmf.cppm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/test.cpp -verify -fsyntax-only
+
+//--- format.h
+#pragma once
+
+namespace test {
+
+template <class _Tp>
+struct type_identity {
+ typedef _Tp type;
+};
+
+template <class _Tp>
+using type_identity_t = typename type_identity<_Tp>::type;
+
+
+template <class _Tp, class _CharT>
+struct formatter
+{
+ formatter() = delete;
+};
+
+template <>
+struct formatter<char, char>
+{};
+
+template <class _CharT, class... _Args>
+struct basic_format_string {
+ static inline const int __handles_{ [] {
+ formatter<char, _CharT> f;
+ (void)f;
+ return 0;
+ }() };
+
+ consteval basic_format_string(const _CharT*) {
+ (void)__handles_;
+ }
+};
+
+template <class... _Args>
+using wformat_string = basic_format_string<wchar_t, type_identity_t<_Args>...>;
+
+template <class... _Args>
+using format_string = basic_format_string<char, type_identity_t<_Args>...>;
+
+template <class... _Args>
+void format(format_string<_Args...> __fmt, _Args&&... __args) {}
+
+template <class... _Args>
+void format(wformat_string<_Args...> __fmt, _Args&&... __args) {}
+
+}
+
+//--- format.cppm
+module;
+#include "format.h"
+export module format;
+
+export namespace test {
+ using test::format;
+ using test::formatter;
+ using test::format_string;
+}
+
+auto something() -> void
+{
+ auto a = 'a';
+ test::format("{}", a);
+}
+
+//--- includes_in_gmf.cppm
+module;
+#include "format.h"
+export module includes_in_gmf;
+
+namespace test {
+ using test::format;
+ using test::formatter;
+ using test::format_string;
+}
+
+//--- test.cpp
+// expected-no-diagnostics
+import format;
+import includes_in_gmf;
+
+auto what() -> void
+{
+ auto a = 'a';
+ test::format("{}", a);
+
+ constexpr auto fs = "{}"; // test::format_string<char>{ "{}" }; // <- same result even passing exact param type
+ test::format(fs, 'r');
+}
diff --git a/clang/test/Modules/pr172241.cppm b/clang/test/Modules/pr172241.cppm
new file mode 100644
index 0000000000000..3eb885e8b2d9f
--- /dev/null
+++ b/clang/test/Modules/pr172241.cppm
@@ -0,0 +1,47 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple %t/m.cppm -emit-module-interface -o %t/m.pcm
+// RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple %t/use.cpp -fmodule-file=m=%t/m.pcm -emit-llvm -o - | FileCheck %t/use.cpp
+//
+// RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple %t/m.cppm -emit-reduced-module-interface -o %t/m.pcm
+// RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple %t/use.cpp -fmodule-file=m=%t/m.pcm -emit-llvm -o - | FileCheck %t/use.cpp
+
+//--- header.h
+#pragma once
+
+template <unsigned T>
+class Templ {
+public:
+ void lock() { __set_locked_bit(); }
+
+private:
+ static constexpr auto __set_locked_bit = [](){};
+};
+
+class JT {
+public:
+ ~JT() {
+ Templ<4> state;
+ state.lock();
+ }
+};
+
+//--- m.cppm
+module;
+#include "header.h"
+export module m;
+export struct M {
+ JT jt;
+};
+//--- use.cpp
+#include "header.h"
+import m;
+
+int main() {
+ M m;
+ return 0;
+}
+
+// CHECK: @_ZN5TemplILj4EE16__set_locked_bitE = {{.*}}linkonce_odr
diff --git a/clang/test/Modules/var-inst-def.cppm b/clang/test/Modules/var-inst-def.cppm
new file mode 100644
index 0000000000000..1414ec76c7be5
--- /dev/null
+++ b/clang/test/Modules/var-inst-def.cppm
@@ -0,0 +1,110 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -fmodule-name=A -xc++ -emit-module -fmodules \
+// RUN: -fno-cxx-modules -fno-implicit-modules \
+// RUN: -fmodule-map-file-home-is-cwd -std=c++20 -I. a.modulemap -o a.pcm
+//
+// RUN: %clang_cc1 -fmodule-name=B -xc++ -emit-module -fmodules \
+// RUN: -fno-cxx-modules -fno-implicit-modules \
+// RUN: -fmodule-map-file-home-is-cwd -std=c++20 -I. b.modulemap -o b.pcm
+//
+// RUN: %clang_cc1 -fmodule-name=C -xc++ -emit-module -fmodules \
+// RUN: -fno-cxx-modules -fno-implicit-modules \
+// RUN: -fmodule-map-file-home-is-cwd -std=c++20 -I. c.modulemap -o c.pcm
+//
+// RUN: %clang_cc1 -fno-cxx-modules -fmodules -fno-implicit-modules \
+// RUN: -fmodule-map-file-home-is-cwd \
+// RUN: -fmodule-file=a.pcm -fmodule-file=b.pcm -fmodule-file=c.pcm \
+// RUN: -std=c++20 -I. main.cpp -o /dev/null
+
+//--- a.modulemap
+module "A" { header "a.h" }
+//--- b.modulemap
+module "B" { header "b.h" }
+//--- c.modulemap
+module "C" { header "c.h" }
+
+//--- common.h
+#pragma once
+#include "stl.h"
+
+//--- a.h
+#pragma once
+#include "common.h"
+#include "repro.h"
+
+//--- b.h
+#pragma once
+#include "common.h"
+#include "repro.h"
+
+//--- c.h
+#pragma once
+#include "common.h"
+#include "repro.h"
+
+//--- repro.h
+#pragma once
+#include "stl.h"
+
+namespace k {
+template <template <typename> class , typename >
+struct is_instantiation : std::integral_constant<bool, false> {};
+template <template <typename> class C, typename T>
+constexpr bool is_instantiation_v = is_instantiation<C, T>::value;
+}
+
+struct ThreadState;
+
+namespace cc::subtle {
+template <typename T>
+class U;
+}
+namespace cc {
+template <typename T> class Co;
+namespace internal {
+template <typename T>
+class Promise {
+ static_assert(!k::is_instantiation_v<subtle::U, T>);
+};
+}
+}
+
+//--- stl.h
+#pragma once
+namespace std {
+inline namespace abi {
+template <class _Tp, _Tp __v>
+struct integral_constant {
+ static const _Tp value = __v;
+};
+template <class _Tp, class _Up>
+constexpr bool is_same_v = __is_same(_Tp, _Up);
+template <class _Tp>
+using decay_t = __decay(_Tp);
+
+template <class>
+struct __invoke_result_impl ;
+template <class... _Args>
+using invoke_result_t = __invoke_result_impl<_Args...>;
+}
+}
+
+//--- main.cpp
+#include "stl.h"
+#include "a.h"
+
+namespace cc {
+template <typename F>
+ requires k::is_instantiation_v<Co, std::invoke_result_t<F>>
+using result_type =
+ std::invoke_result_t<F>;
+}
+namespace cc::internal {
+class final {
+ Promise<ThreadState> outgoing_work_;
+};
+}
More information about the cfe-commits
mailing list