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_; +}; +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
