llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-modules Author: None (llvmbot) <details> <summary>Changes</summary> Backport 847f9cb0e868c8ec34f9aa86fdf846f8c4e0388b Requested by: @<!-- -->ChuanqiXu9 --- Patch is 27.70 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/102561.diff 19 Files Affected: - (modified) clang/include/clang/AST/DeclBase.h (+7) - (modified) clang/include/clang/Serialization/ASTBitCodes.h (+3) - (modified) clang/include/clang/Serialization/ASTReader.h (+6) - (modified) clang/include/clang/Serialization/ASTWriter.h (+7) - (modified) clang/lib/AST/ASTContext.cpp (+1-2) - (modified) clang/lib/AST/DeclBase.cpp (+25-9) - (modified) clang/lib/CodeGen/CGVTables.cpp (+33-23) - (modified) clang/lib/CodeGen/ItaniumCXXABI.cpp (+3) - (modified) clang/lib/Sema/SemaDecl.cpp (+9) - (modified) clang/lib/Sema/SemaDeclCXX.cpp (+9-5) - (modified) clang/lib/Serialization/ASTReader.cpp (+11) - (modified) clang/lib/Serialization/ASTReaderDecl.cpp (+7) - (modified) clang/lib/Serialization/ASTWriter.cpp (+29-4) - (modified) clang/lib/Serialization/ASTWriterDecl.cpp (+6) - (modified) clang/test/CodeGenCXX/modules-vtable.cppm (+19-12) - (added) clang/test/CodeGenCXX/pr70585.cppm (+47) - (added) clang/test/Modules/pr97313.cppm (+118) - (added) clang/test/Modules/static-func-in-private.cppm (+8) - (added) clang/test/Modules/vtable-windows.cppm (+26) ``````````diff diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h index 40f01abf384e92..2a4bd0f9c2fda3 100644 --- a/clang/include/clang/AST/DeclBase.h +++ b/clang/include/clang/AST/DeclBase.h @@ -670,6 +670,13 @@ class alignas(8) Decl { /// Whether this declaration comes from another module unit. bool isInAnotherModuleUnit() const; + /// Whether this declaration comes from the same module unit being compiled. + bool isInCurrentModuleUnit() const; + + /// Whether the definition of the declaration should be emitted in external + /// sources. + bool shouldEmitInExternalSource() const; + /// Whether this declaration comes from explicit global module. bool isFromExplicitGlobalModule() const; diff --git a/clang/include/clang/Serialization/ASTBitCodes.h b/clang/include/clang/Serialization/ASTBitCodes.h index 5dd0ba33f8a9c2..9b7e3af0e449b5 100644 --- a/clang/include/clang/Serialization/ASTBitCodes.h +++ b/clang/include/clang/Serialization/ASTBitCodes.h @@ -721,6 +721,9 @@ enum ASTRecordTypes { /// Record code for \#pragma clang unsafe_buffer_usage begin/end PP_UNSAFE_BUFFER_USAGE = 69, + + /// Record code for vtables to emit. + VTABLES_TO_EMIT = 70, }; /// Record types used within a source manager block. diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h index 76e51ac7ab9792..671520a3602b3b 100644 --- a/clang/include/clang/Serialization/ASTReader.h +++ b/clang/include/clang/Serialization/ASTReader.h @@ -790,6 +790,11 @@ class ASTReader /// the consumer eagerly. SmallVector<GlobalDeclID, 16> EagerlyDeserializedDecls; + /// The IDs of all vtables to emit. The referenced declarations are passed + /// to the consumers' HandleVTable eagerly after passing + /// EagerlyDeserializedDecls. + SmallVector<GlobalDeclID, 16> VTablesToEmit; + /// The IDs of all tentative definitions stored in the chain. /// /// Sema keeps track of all tentative definitions in a TU because it has to @@ -1500,6 +1505,7 @@ class ASTReader bool isConsumerInterestedIn(Decl *D); void PassInterestingDeclsToConsumer(); void PassInterestingDeclToConsumer(Decl *D); + void PassVTableToConsumer(CXXRecordDecl *RD); void finishPendingActions(); void diagnoseOdrViolations(); diff --git a/clang/include/clang/Serialization/ASTWriter.h b/clang/include/clang/Serialization/ASTWriter.h index a0e475ec9f862c..71a7c28047e318 100644 --- a/clang/include/clang/Serialization/ASTWriter.h +++ b/clang/include/clang/Serialization/ASTWriter.h @@ -500,6 +500,10 @@ class ASTWriter : public ASTDeserializationListener, std::vector<SourceRange> NonAffectingRanges; std::vector<SourceLocation::UIntTy> NonAffectingOffsetAdjustments; + /// A list of classes which need to emit the VTable in the corresponding + /// object file. + llvm::SmallVector<CXXRecordDecl *> PendingEmittingVTables; + /// Computes input files that didn't affect compilation of the current module, /// and initializes data structures necessary for leaving those files out /// during \c SourceManager serialization. @@ -857,6 +861,8 @@ class ASTWriter : public ASTDeserializationListener, return PredefinedDecls.count(D); } + void handleVTable(CXXRecordDecl *RD); + private: // ASTDeserializationListener implementation void ReaderInitialized(ASTReader *Reader) override; @@ -951,6 +957,7 @@ class PCHGenerator : public SemaConsumer { void InitializeSema(Sema &S) override { SemaPtr = &S; } void HandleTranslationUnit(ASTContext &Ctx) override; + void HandleVTable(CXXRecordDecl *RD) override { Writer.handleVTable(RD); } ASTMutationListener *GetASTMutationListener() override; ASTDeserializationListener *GetASTDeserializationListener() override; bool hasEmittedPCH() const { return Buffer->IsComplete; } diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp index 7af9ea7105bb08..3da5e888f25175 100644 --- a/clang/lib/AST/ASTContext.cpp +++ b/clang/lib/AST/ASTContext.cpp @@ -12405,8 +12405,7 @@ bool ASTContext::DeclMustBeEmitted(const Decl *D) { !isMSStaticDataMemberInlineDefinition(VD)) return false; - // Variables in other module units shouldn't be forced to be emitted. - if (VD->isInAnotherModuleUnit()) + if (VD->shouldEmitInExternalSource()) return false; // Variables that can be needed in other TUs are required. diff --git a/clang/lib/AST/DeclBase.cpp b/clang/lib/AST/DeclBase.cpp index bc5a9206c0db28..b59f118380ca4b 100644 --- a/clang/lib/AST/DeclBase.cpp +++ b/clang/lib/AST/DeclBase.cpp @@ -1125,20 +1125,36 @@ bool Decl::isInAnotherModuleUnit() const { if (!M) return false; + // FIXME or NOTE: maybe we need to be clear about the semantics + // of clang header modules. e.g., if this lives in a clang header + // module included by the current unit, should we return false + // here? + // + // This is clear for header units as the specification says the + // header units live in a synthesised translation unit. So we + // can return false here. M = M->getTopLevelModule(); - // FIXME: It is problematic if the header module lives in another module - // unit. Consider to fix this by techniques like - // ExternalASTSource::hasExternalDefinitions. - if (M->isHeaderLikeModule()) + if (!M->isNamedModule()) return false; - // A global module without parent implies that we're parsing the global - // module. So it can't be in another module unit. - if (M->isGlobalModule()) + return M != getASTContext().getCurrentNamedModule(); +} + +bool Decl::isInCurrentModuleUnit() const { + auto *M = getOwningModule(); + + if (!M || !M->isNamedModule()) return false; - assert(M->isNamedModule() && "New module kind?"); - return M != getASTContext().getCurrentNamedModule(); + return M == getASTContext().getCurrentNamedModule(); +} + +bool Decl::shouldEmitInExternalSource() const { + ExternalASTSource *Source = getASTContext().getExternalSource(); + if (!Source) + return false; + + return Source->hasExternalDefinitions(this) == ExternalASTSource::EK_Always; } bool Decl::isFromExplicitGlobalModule() const { diff --git a/clang/lib/CodeGen/CGVTables.cpp b/clang/lib/CodeGen/CGVTables.cpp index 7f729d359b82b3..267bdf09829700 100644 --- a/clang/lib/CodeGen/CGVTables.cpp +++ b/clang/lib/CodeGen/CGVTables.cpp @@ -1078,29 +1078,41 @@ llvm::GlobalVariable::LinkageTypes CodeGenModule::getVTableLinkage(const CXXRecordDecl *RD) { if (!RD->isExternallyVisible()) return llvm::GlobalVariable::InternalLinkage; - - // We're at the end of the translation unit, so the current key - // function is fully correct. - const CXXMethodDecl *keyFunction = Context.getCurrentKeyFunction(RD); - if (keyFunction && !RD->hasAttr<DLLImportAttr>()) { + + // In windows, the linkage of vtable is not related to modules. + bool IsInNamedModule = !getTarget().getCXXABI().isMicrosoft() && + RD->isInNamedModule(); + // If the CXXRecordDecl is not in a module unit, we need to get + // its key function. We're at the end of the translation unit, so the current + // key function is fully correct. + const CXXMethodDecl *keyFunction = + IsInNamedModule ? nullptr : Context.getCurrentKeyFunction(RD); + if (IsInNamedModule || (keyFunction && !RD->hasAttr<DLLImportAttr>())) { // If this class has a key function, use that to determine the // linkage of the vtable. const FunctionDecl *def = nullptr; - if (keyFunction->hasBody(def)) + if (keyFunction && keyFunction->hasBody(def)) keyFunction = cast<CXXMethodDecl>(def); - switch (keyFunction->getTemplateSpecializationKind()) { - case TSK_Undeclared: - case TSK_ExplicitSpecialization: + bool IsExternalDefinition = + IsInNamedModule ? RD->shouldEmitInExternalSource() : !def; + + TemplateSpecializationKind Kind = + IsInNamedModule ? RD->getTemplateSpecializationKind() + : keyFunction->getTemplateSpecializationKind(); + + switch (Kind) { + case TSK_Undeclared: + case TSK_ExplicitSpecialization: assert( - (def || CodeGenOpts.OptimizationLevel > 0 || + (IsInNamedModule || def || CodeGenOpts.OptimizationLevel > 0 || CodeGenOpts.getDebugInfo() != llvm::codegenoptions::NoDebugInfo) && - "Shouldn't query vtable linkage without key function, " - "optimizations, or debug info"); - if (!def && CodeGenOpts.OptimizationLevel > 0) + "Shouldn't query vtable linkage without the class in module units, " + "key function, optimizations, or debug info"); + if (IsExternalDefinition && CodeGenOpts.OptimizationLevel > 0) return llvm::GlobalVariable::AvailableExternallyLinkage; - if (keyFunction->isInlined()) + if (keyFunction && keyFunction->isInlined()) return !Context.getLangOpts().AppleKext ? llvm::GlobalVariable::LinkOnceODRLinkage : llvm::Function::InternalLinkage; @@ -1119,7 +1131,7 @@ CodeGenModule::getVTableLinkage(const CXXRecordDecl *RD) { case TSK_ExplicitInstantiationDeclaration: llvm_unreachable("Should not have been asked to emit this"); - } + } } // -fapple-kext mode does not support weak linkage, so we must use @@ -1213,22 +1225,20 @@ bool CodeGenVTables::isVTableExternal(const CXXRecordDecl *RD) { TSK == TSK_ExplicitInstantiationDefinition) return false; + // 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. + if (RD->isInNamedModule()) + return RD->shouldEmitInExternalSource(); + // 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); if (!keyFunction) return false; - 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(); } /// Given that we're currently at the end of the translation unit, and diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp index cd76f8406e7b72..0be92fb2e27579 100644 --- a/clang/lib/CodeGen/ItaniumCXXABI.cpp +++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp @@ -2315,6 +2315,9 @@ bool ItaniumCXXABI::canSpeculativelyEmitVTable(const CXXRecordDecl *RD) const { if (!canSpeculativelyEmitVTableAsBaseClass(RD)) return false; + if (RD->shouldEmitInExternalSource()) + return false; + // For a complete-object vtable (or more specifically, for the VTT), we need // to be able to speculatively emit the vtables of all dynamic virtual bases. for (const auto &B : RD->vbases()) { diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 01231f8e385ef1..99ee8efea54c03 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -18073,6 +18073,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 where 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 04b8d88cae217b..6f058a62599991 100644 --- a/clang/lib/Sema/SemaDeclCXX.cpp +++ b/clang/lib/Sema/SemaDeclCXX.cpp @@ -18485,11 +18485,15 @@ 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->isInCurrentModuleUnit()) { + 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 = @@ -18534,7 +18538,7 @@ bool Sema::DefineUsedVTables() { DefinedAnything = true; MarkVirtualMembersReferenced(Loc, Class); CXXRecordDecl *Canonical = Class->getCanonicalDecl(); - if (VTablesUsed[Canonical]) + if (VTablesUsed[Canonical] && !Class->shouldEmitInExternalSource()) Consumer.HandleVTable(Class); // Warn if we're emitting a weak vtable. The vtable will be weak if there is diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index 3cb96df12e4da0..29aec144aec1a8 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -3921,6 +3921,13 @@ llvm::Error ASTReader::ReadASTBlock(ModuleFile &F, } break; + case VTABLES_TO_EMIT: + if (F.Kind == MK_MainFile || + getContext().getLangOpts().BuildingPCHWithObjectFile) + for (unsigned I = 0, N = Record.size(); I != N;) + VTablesToEmit.push_back(ReadDeclID(F, Record, I)); + break; + case IMPORTED_MODULES: if (!F.isModule()) { // If we aren't loading a module (which has its own exports), make @@ -8110,6 +8117,10 @@ void ASTReader::PassInterestingDeclToConsumer(Decl *D) { Consumer->HandleInterestingDecl(DeclGroupRef(D)); } +void ASTReader::PassVTableToConsumer(CXXRecordDecl *RD) { + Consumer->HandleVTable(RD); +} + void ASTReader::StartTranslationUnit(ASTConsumer *Consumer) { this->Consumer = Consumer; diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp index 31ab6c651d59f4..f17069fe12e3bb 100644 --- a/clang/lib/Serialization/ASTReaderDecl.cpp +++ b/clang/lib/Serialization/ASTReaderDecl.cpp @@ -4209,6 +4209,13 @@ void ASTReader::PassInterestingDeclsToConsumer() { // If we add any new potential interesting decl in the last call, consume it. ConsumingPotentialInterestingDecls(); + + for (GlobalDeclID ID : VTablesToEmit) { + auto *RD = cast<CXXRecordDecl>(GetDecl(ID)); + assert(!RD->shouldEmitInExternalSource()); + PassVTableToConsumer(RD); + } + VTablesToEmit.clear(); } void ASTReader::loadDeclUpdateRecords(PendingUpdateRecord &Record) { diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index c78d8943d6d92e..7c0636962459e9 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -927,6 +927,7 @@ void ASTWriter::WriteBlockInfoBlock() { RECORD(DECLS_TO_CHECK_FOR_DEFERRED_DIAGS); RECORD(PP_ASSUME_NONNULL_LOC); RECORD(PP_UNSAFE_BUFFER_USAGE); + RECORD(VTABLES_TO_EMIT); // SourceManager Block. BLOCK(SOURCE_MANAGER_BLOCK); @@ -3961,6 +3962,10 @@ void ASTWriter::WriteIdentifierTable(Preprocessor &PP, Stream.EmitRecord(INTERESTING_IDENTIFIERS, InterestingIdents); } +void ASTWriter::handleVTable(CXXRecordDecl *RD) { + PendingEmittingVTables.push_back(RD); +} + //===----------------------------------------------------------------------===// // DeclContext's Name Lookup Table Serialization //===----------------------------------------------------------------------===// @@ -5163,6 +5168,13 @@ void ASTWriter::PrepareWritingSpecialDecls(Sema &SemaRef) { // Write all of the DeclsToCheckForDeferredDiags. for (auto *D : SemaRef.DeclsToCheckForDeferredDiags) GetDeclRef(D); + + // Write all classes that need to emit the vtable definitions if required. + if (isWritingStdCXXNamedModules()) + for (CXXRecordDecl *RD : PendingEmittingVTables) + GetDeclRef(RD); + else + PendingEmittingVTables.clear(); } void ASTWriter::WriteSpecialDeclRecords(Sema &SemaRef) { @@ -5317,6 +5329,17 @@ void ASTWriter::WriteSpecialDeclRecords(Sema &SemaRef) { } if (!DeleteExprsToAnalyze.empty()) Stream.EmitRecord(DELETE_EXPRS_TO_ANALYZE, DeleteExprsToAnalyze); + + RecordData VTablesToEmit; + for (CXXRecordDecl *RD : PendingEmittingVTables) { + if (!wasDeclEmitted(RD)) + continue; + + AddDeclRef(RD, VTablesToEmit); + } + + if (!VTablesToEmit.empty()) + Stream.EmitRecord(VTABLES_TO_EMIT, VTablesToEmit); } ASTFileSignature ASTWriter::WriteASTCore(Sema &SemaRef, StringRef isysroot, @@ -6559,10 +6582,12 @@ void ASTRecordWriter::AddCXXDefinitionData(const CXXRecordDecl *D) { // computed. Record->push_back(D->getODRHash()); - bool ModulesDebugInfo = - Writer->Context->getLangOpts().ModulesDebugInfo && !D->isDependentType(); - Record->push_back(ModulesDebugInfo); - if (ModulesDebugInfo) + bool ModulesCodegen = + !D->isDependentType() && + (Writer->Context->getLangOpts().ModulesDebugInfo || + D->isInNamedModule()); + Record->push_back(ModulesCodegen); + if (ModulesCodegen) Writer->AddDeclRef(D, Writer->ModularCodegenDecls); // IsLambda bit is already saved. diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp index 17c774038571ef..8a4ca54349e38f 100644 --- a/clang/lib/Serialization/ASTWriterDecl.cpp +++ b/clang/lib/Serialization/ASTWriterDecl.cpp @@ -1529,8 +1529,14 @@ void ASTDeclWriter::VisitCXXRecordDecl(CXXRecordDecl *D) { if (D->isThisDeclarationADefinition()) Record.AddCXXDefinitionData(D); + if (D->isCompleteDefinition() && D->isInNamedModule()) + Writer.AddDeclRef(D, Writer.ModularCodegenDecls); + // Store (what we currently believe to be) the key function to avoid // deserializing every method so we can compute it. + // + // FIXME: Avoid adding the key function if the class is defined in + // module purview since in that case the key function is meaningless. if (D->isCompleteDefinition()) Record.AddDeclRef(Context.getCurrentKeyFunction(D)); diff --git a/clang/test/CodeGenCXX/modules-vtable.cppm b/clang/test/CodeGenCXX/modules-vtable.cppm index fb179b1de4880b..5cc3504d72628f 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() { @@ -58,13 +61,13 @@ int use() { return 43; } -// CHECK-NOT: @_ZTSW3Mod4Base = constant -// CHECK-NOT: @_ZTIW3Mod4Base = constant -// CHECK: @_ZTVW3Mod4Base = external unnamed_addr +// CHECK-NOT: @_ZTSW3Mod4Base +// CHECK-NOT: @_ZTIW3Mod4Base +// CHECK: @_ZTVW3Mod4Base = external -// CHECK... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/102561 _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
