From 7a5c4ccd37b263a4d3d01df16591b576a64e839f Mon Sep 17 00:00:00 2001 From: Chuanqi Xu Date: Tue, 19 Dec 2023 17:00:59 +0800 Subject: [PATCH] [C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes Close https://github.com/llvm/llvm-project/issues/70585 and reflect https://github.com/itanium-cxx-abi/cxx-abi/issues/170. The significant change of the patch is: for dynamic classes attached to module units, we generate the vtable to the attached module units directly and the key functions for such classes is meaningless. --- clang/lib/CodeGen/CGVTables.cpp | 28 ++++++++++---- clang/lib/CodeGen/CodeGenModule.cpp | 9 +++++ clang/lib/CodeGen/ItaniumCXXABI.cpp | 6 +++ clang/lib/Sema/SemaDecl.cpp | 9 +++++ clang/lib/Sema/SemaDeclCXX.cpp | 13 +++++-- clang/lib/Serialization/ASTReaderDecl.cpp | 13 ++++++- clang/lib/Serialization/ASTWriterDecl.cpp | 13 +++++-- clang/test/CodeGenCXX/modules-vtable.cppm | 27 ++++++++----- clang/test/CodeGenCXX/pr70585.cppm | 47 +++++++++++++++++++++++ 9 files changed, 139 insertions(+), 26 deletions(-) create mode 100644 clang/test/CodeGenCXX/pr70585.cppm diff --git a/clang/lib/CodeGen/CGVTables.cpp b/clang/lib/CodeGen/CGVTables.cpp index 27a2cab4f75319..5c8499d2594a1d 100644 --- a/clang/lib/CodeGen/CGVTables.cpp +++ b/clang/lib/CodeGen/CGVTables.cpp @@ -1046,6 +1046,11 @@ CodeGenModule::getVTableLinkage(const CXXRecordDecl *RD) { if (!RD->isExternallyVisible()) return llvm::GlobalVariable::InternalLinkage; + // V-tables for non-template classes with an owning module are always + // uniquely emitted in that module. + if (Module *M = RD->getOwningModule(); M && M->isNamedModule()) + return llvm::GlobalVariable::ExternalLinkage; + // We're at the end of the translation unit, so the current key // function is fully correct. const CXXMethodDecl *keyFunction = Context.getCurrentKeyFunction(RD); @@ -1180,6 +1185,21 @@ bool CodeGenVTables::isVTableExternal(const CXXRecordDecl *RD) { TSK == TSK_ExplicitInstantiationDefinition) return false; + // Itanium C++ ABI [5.2.3]: + // Virtual tables for dynamic classes are emitted as follows: + // + // - If the class is templated, the tables are emitted in every object that + // references any of them. + // - Otherwise, if the class is attached to a module, the tables are uniquely + // emitted in the object for the module unit in which it is defined. + // - Otherwise, if the class has a key function (see below), the tables are + // emitted in the object for the translation unit containing the definition of + // the key function. This is unique if the key function is not inline. + // - Otherwise, the tables are emitted in every object that references any of + // them. + if (Module *M = RD->getOwningModule(); M && M->isNamedModule()) + return M != CGM.getContext().getCurrentNamedModule(); + // Otherwise, if the class doesn't have a key function (possibly // anymore), the vtable must be defined here. const CXXMethodDecl *keyFunction = CGM.getContext().getCurrentKeyFunction(RD); @@ -1189,13 +1209,7 @@ bool CodeGenVTables::isVTableExternal(const CXXRecordDecl *RD) { const FunctionDecl *Def; // Otherwise, if we don't have a definition of the key function, the // vtable must be defined somewhere else. - if (!keyFunction->hasBody(Def)) - return true; - - assert(Def && "The body of the key function is not assigned to Def?"); - // If the non-inline key function comes from another module unit, the vtable - // must be defined there. - return Def->isInAnotherModuleUnit() && !Def->isInlineSpecified(); + return !keyFunction->hasBody(Def); } /// Given that we're currently at the end of the translation unit, and diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp index d78f2594a23764..4863f7348ed99a 100644 --- a/clang/lib/CodeGen/CodeGenModule.cpp +++ b/clang/lib/CodeGen/CodeGenModule.cpp @@ -6738,6 +6738,15 @@ void CodeGenModule::EmitTopLevelDecl(Decl *D) { if (ES->hasExternalDefinitions(D) == ExternalASTSource::EK_Never) DI->completeUnusedClass(*CRD); } + // If we're emitting a dynamic class from the importable module we're + // emitting, we always need to emit the virtual table according to the ABI + // requirement. + if (CRD->getOwningModule() && + CRD->getOwningModule()->isInterfaceOrPartition() && + CRD->getDefinition() && CRD->isDynamicClass() && + CRD->getOwningModule() == getContext().getCurrentNamedModule()) + EmitVTable(CRD); + // Emit any static data members, they may be definitions. for (auto *I : CRD->decls()) if (isa(I) || isa(I)) diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp index d173806ec8ce53..1acf8b3e0659ff 100644 --- a/clang/lib/CodeGen/ItaniumCXXABI.cpp +++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp @@ -1801,6 +1801,12 @@ void ItaniumCXXABI::emitVTableDefinitions(CodeGenVTables &CGVT, if (VTable->hasInitializer()) return; + // If the class is attached to a C++ named module other than the one + // we're currently compiling, the vtable should be defined there. + if (Module *M = RD->getOwningModule(); + M && M->isNamedModule() && M != CGM.getContext().getCurrentNamedModule()) + return; + ItaniumVTableContext &VTContext = CGM.getItaniumVTableContext(); const VTableLayout &VTLayout = VTContext.getVTableLayout(RD); llvm::GlobalVariable::LinkageTypes Linkage = CGM.getVTableLinkage(RD); diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index ffbe317d559995..646b94f67023c5 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -18127,6 +18127,15 @@ void Sema::ActOnTagFinishDefinition(Scope *S, Decl *TagD, if (NumInitMethods > 1 || !Def->hasInitMethod()) Diag(RD->getLocation(), diag::err_sycl_special_type_num_init_method); } + + // If we're defining a dynamic class in a module interface unit, we always + // need to produce the vtable for it even if the vtable is not used in the + // current TU. + // + // The case that the current class is not dynamic is handled in + // MarkVTableUsed. + if (getCurrentModule() && getCurrentModule()->isInterfaceOrPartition()) + MarkVTableUsed(RD->getLocation(), RD, /*DefinitionRequired=*/true); } // Exit this scope of this tag's definition. diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp index 36e53c684ac4dc..5d1524d7db1385 100644 --- a/clang/lib/Sema/SemaDeclCXX.cpp +++ b/clang/lib/Sema/SemaDeclCXX.cpp @@ -18697,11 +18697,16 @@ bool Sema::DefineUsedVTables() { bool DefineVTable = true; - // If this class has a key function, but that key function is - // defined in another translation unit, we don't need to emit the - // vtable even though we're using it. const CXXMethodDecl *KeyFunction = Context.getCurrentKeyFunction(Class); - if (KeyFunction && !KeyFunction->hasBody()) { + // V-tables for non-template classes with an owning module are always + // uniquely emitted in that module. + if (Class->getOwningModule() && + Class->getOwningModule()->isInterfaceOrPartition()) + DefineVTable = true; + else if (KeyFunction && !KeyFunction->hasBody()) { + // If this class has a key function, but that key function is + // defined in another translation unit, we don't need to emit the + // vtable even though we're using it. // The key function is in another translation unit. DefineVTable = false; TemplateSpecializationKind TSK = diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp index 547eb77930b4ee..e6b3f4a1b42342 100644 --- a/clang/lib/Serialization/ASTReaderDecl.cpp +++ b/clang/lib/Serialization/ASTReaderDecl.cpp @@ -2253,7 +2253,10 @@ ASTDeclReader::VisitCXXRecordDeclImpl(CXXRecordDecl *D) { // Lazily load the key function to avoid deserializing every method so we can // compute it. - if (WasDefinition) { + // + // The key function in named module is meaningless. + if (WasDefinition && (!D->getOwningModule() || + !D->getOwningModule()->isInterfaceOrPartition())) { DeclID KeyFn = readDeclID(); if (KeyFn && D->isCompleteDefinition()) // FIXME: This is wrong for the ARM ABI, where some other module may have @@ -3212,6 +3215,14 @@ static bool isConsumerInterestedIn(ASTContext &Ctx, Decl *D, bool HasBody) { if (ES->hasExternalDefinitions(D) == ExternalASTSource::EK_Never) return true; + // The dynamic class defined in a named module is interesting. + // The code generator needs to emit its vtable there. + if (const auto *Class = dyn_cast(D)) + return Class->getOwningModule() && + Class->getOwningModule()->isInterfaceOrPartition() && + Class->getOwningModule() == Ctx.getCurrentNamedModule() && + Class->getDefinition() && Class->isDynamicClass(); + return false; } diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp index 9e3299f0491848..b971dd02513d86 100644 --- a/clang/lib/Serialization/ASTWriterDecl.cpp +++ b/clang/lib/Serialization/ASTWriterDecl.cpp @@ -1483,10 +1483,15 @@ void ASTDeclWriter::VisitCXXRecordDecl(CXXRecordDecl *D) { if (D->isThisDeclarationADefinition()) Record.AddCXXDefinitionData(D); - // Store (what we currently believe to be) the key function to avoid - // deserializing every method so we can compute it. - if (D->isCompleteDefinition()) - Record.AddDeclRef(Context.getCurrentKeyFunction(D)); + if (D->isCompleteDefinition()) { + if (D->getOwningModule() && D->getOwningModule()->isInterfaceOrPartition()) + Writer.ModularCodegenDecls.push_back(Writer.GetDeclRef(D)); + else { + // Store (what we currently believe to be) the key function to avoid + // deserializing every method so we can compute it. + Record.AddDeclRef(Context.getCurrentKeyFunction(D)); + } + } Code = serialization::DECL_CXX_RECORD; } diff --git a/clang/test/CodeGenCXX/modules-vtable.cppm b/clang/test/CodeGenCXX/modules-vtable.cppm index fb179b1de4880b..abbbc403ace243 100644 --- a/clang/test/CodeGenCXX/modules-vtable.cppm +++ b/clang/test/CodeGenCXX/modules-vtable.cppm @@ -24,6 +24,8 @@ // RUN: %t/M-A.cppm -o %t/M-A.pcm // RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 -fmodule-file=M:A=%t/M-A.pcm \ // RUN: %t/M-B.cppm -emit-llvm -o - | FileCheck %t/M-B.cppm +// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 \ +// RUN: %t/M-A.pcm -emit-llvm -o - | FileCheck %t/M-A.cppm //--- Mod.cppm export module Mod; @@ -41,9 +43,10 @@ Base::~Base() {} // CHECK: @_ZTSW3Mod4Base = constant // CHECK: @_ZTIW3Mod4Base = constant -// CHECK-INLINE: @_ZTVW3Mod4Base = linkonce_odr {{.*}}unnamed_addr constant -// CHECK-INLINE: @_ZTSW3Mod4Base = linkonce_odr {{.*}}constant -// CHECK-INLINE: @_ZTIW3Mod4Base = linkonce_odr {{.*}}constant +// With the new Itanium C++ ABI, the linkage of vtables in modules don't need to be linkonce ODR. +// CHECK-INLINE: @_ZTVW3Mod4Base = {{.*}}unnamed_addr constant +// CHECK-INLINE: @_ZTSW3Mod4Base = {{.*}}constant +// CHECK-INLINE: @_ZTIW3Mod4Base = {{.*}}constant module :private; int private_use() { @@ -60,11 +63,11 @@ int use() { // CHECK-NOT: @_ZTSW3Mod4Base = constant // CHECK-NOT: @_ZTIW3Mod4Base = constant -// CHECK: @_ZTVW3Mod4Base = external unnamed_addr +// CHECK: @_ZTVW3Mod4Base = external -// CHECK-INLINE: @_ZTVW3Mod4Base = linkonce_odr {{.*}}unnamed_addr constant -// CHECK-INLINE: @_ZTSW3Mod4Base = linkonce_odr {{.*}}constant -// CHECK-INLINE: @_ZTIW3Mod4Base = linkonce_odr {{.*}}constant +// CHECK-INLINE-NOT: @_ZTSW3Mod4Base = constant +// CHECK-INLINE-NOT: @_ZTIW3Mod4Base = constant +// CHECK-INLINE: @_ZTVW3Mod4Base = external // Check the case that the declaration of the key function comes from another // module unit but the definition of the key function comes from the current @@ -82,6 +85,10 @@ int a_use() { return 43; } +// CHECK: @_ZTVW1M1C = unnamed_addr constant +// CHECK: @_ZTSW1M1C = constant +// CHECK: @_ZTIW1M1C = constant + //--- M-B.cppm export module M:B; import :A; @@ -93,6 +100,6 @@ int b_use() { return 43; } -// CHECK: @_ZTVW1M1C = unnamed_addr constant -// CHECK: @_ZTSW1M1C = constant -// CHECK: @_ZTIW1M1C = constant +// CHECK: @_ZTVW1M1C = external +// CHECK-NOT: @_ZTSW1M1C = constant +// CHECK-NOT: @_ZTIW1M1C = constant diff --git a/clang/test/CodeGenCXX/pr70585.cppm b/clang/test/CodeGenCXX/pr70585.cppm new file mode 100644 index 00000000000000..a2d6e02b5833ee --- /dev/null +++ b/clang/test/CodeGenCXX/pr70585.cppm @@ -0,0 +1,47 @@ +// REQUIRES: !system-windows + +// RUN: rm -rf %t +// RUN: split-file %s %t +// RUN: cd %t +// +// RUN: %clang_cc1 -std=c++20 %t/layer1.cppm -triple %itanium_abi_triple \ +// RUN: -emit-module-interface -o %t/foo-layer1.pcm +// RUN: %clang_cc1 -std=c++20 %t/layer2.cppm -triple %itanium_abi_triple \ +// RUN: -emit-module-interface -fmodule-file=foo:layer1=%t/foo-layer1.pcm \ +// RUN: -o %t/foo-layer2.pcm +// RUN: %clang_cc1 -std=c++20 %t/foo-layer1.pcm -S -emit-llvm -o - | FileCheck %t/layer1.cppm +// RUN: %clang_cc1 -std=c++20 %t/foo-layer2.pcm -S -emit-llvm -o - \ +// RUN: -fmodule-file=foo:layer1=%t/foo-layer1.pcm | FileCheck %t/layer2.cppm +// +// Check the case about emitting object files from sources directly. +// RUN: %clang_cc1 -std=c++20 %t/layer1.cppm -triple %itanium_abi_triple \ +// RUN: -S -emit-llvm -o - | FileCheck %t/layer1.cppm +// RUN: %clang_cc1 -std=c++20 %t/layer2.cppm -triple %itanium_abi_triple -S -emit-llvm \ +// RUN: -fmodule-file=foo:layer1=%t/foo-layer1.pcm -o - | FileCheck %t/layer2.cppm + +//--- layer1.cppm +export module foo:layer1; +struct Fruit { + virtual ~Fruit() = default; + virtual void eval(); +}; + +// CHECK-DAG: @_ZTVW3foo5Fruit = unnamed_addr constant +// CHECK-DAG: @_ZTSW3foo5Fruit = constant +// CHECK-DAG: @_ZTIW3foo5Fruit = constant + +// Testing that: +// (1) The use of virtual functions won't produce the vtable. +// (2) The definition of key functions won't produce the vtable. +// +//--- layer2.cppm +export module foo:layer2; +import :layer1; +export void layer2_fun() { + Fruit *b = new Fruit(); + b->eval(); +} +void Fruit::eval() {} +// CHECK: @_ZTVW3foo5Fruit = external unnamed_addr constant +// CHECK-NOT: @_ZTSW3foo5Fruit +// CHECK-NOT: @_ZTIW3foo5Fruit