Skip to content

[C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes #75912

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jun 17, 2024
Merged

[C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes #75912

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4b595ee
[C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit
ChuanqiXu9 Dec 19, 2023
4c654ce
update
ChuanqiXu9 Jun 6, 2024
5a65491
Update
ChuanqiXu9 Jun 7, 2024
de4b842
Update
ChuanqiXu9 Jun 11, 2024
aecb473
update
ChuanqiXu9 Jun 12, 2024
c136838
Use isInCurrentModuleUnit
ChuanqiXu9 Jun 13, 2024
65524b6
Update
ChuanqiXu9 Jun 14, 2024
5401fe6
Remove additional blank lines
ChuanqiXu9 Jun 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions clang/include/clang/AST/DeclBase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,6 +670,9 @@ 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;
Expand Down
3 changes: 3 additions & 0 deletions clang/include/clang/Serialization/ASTBitCodes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,6 +697,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.
Expand Down
6 changes: 6 additions & 0 deletions clang/include/clang/Serialization/ASTReader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -805,6 +805,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's 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
Expand Down Expand Up @@ -1514,6 +1519,7 @@ class ASTReader
bool isConsumerInterestedIn(Decl *D);
void PassInterestingDeclsToConsumer();
void PassInterestingDeclToConsumer(Decl *D);
void PassVTableToConsumer(CXXRecordDecl *RD);

void finishPendingActions();
void diagnoseOdrViolations();
Expand Down
7 changes: 7 additions & 0 deletions clang/include/clang/Serialization/ASTWriter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,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.
Expand Down Expand Up @@ -849,6 +853,8 @@ class ASTWriter : public ASTDeserializationListener,

bool getDoneWritingDeclsAndTypes() const { return DoneWritingDeclsAndTypes; }

void handleVTable(CXXRecordDecl *RD);

private:
// ASTDeserializationListener implementation
void ReaderInitialized(ASTReader *Reader) override;
Expand Down Expand Up @@ -943,6 +949,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; }
Expand Down
9 changes: 9 additions & 0 deletions clang/lib/AST/DeclBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1128,6 +1128,15 @@ bool Decl::isInAnotherModuleUnit() const {
return M != getASTContext().getCurrentNamedModule();
}

bool Decl::isInCurrentModuleUnit() const {
auto *M = getOwningModule();

if (!M || !M->isNamedModule())
return false;

return M == getASTContext().getCurrentNamedModule();
}

bool Decl::shouldEmitInExternalSource() const {
ExternalASTSource *Source = getASTContext().getExternalSource();
if (!Source)
Expand Down
28 changes: 21 additions & 7 deletions clang/lib/CodeGen/CGVTables.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1051,6 +1051,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 (RD->isInNamedModule())
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);
Expand Down Expand Up @@ -1185,6 +1190,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 (RD->isInNamedModule())
return RD->shouldEmitInExternalSource();
Comment on lines +1205 to +1206
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't need to test "isInNamedModule" here, though? Would it be adequate to do:

if (RD->shouldEmitInExternalSource())
  return true;

Ah, I geuss you want to return false ASAP too - I guess looking at some of the implementation details of shouldEmitInExternalSource and using those APIs directly here - Always -> true, Never -> false, I think?

Like adjustGVALinkageForExternalDefinitionKind already does? Or even reusing some of that functionality somehow.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried. But it can't work the case we compile the AST directly to the object file. In this case, hasExternalDefinitions returns EK_ReplyHazy, but we need to return false directly here. I think it looks good now. If we want to extend this to header modules with object files, it will be easy.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems unfortunate - having these things work through different codepaths/states when compiling directly rather than via a pcm. It'd be good to make those paths converge (making hasExternalDefinitions provide the same answers/do the right thing when compiling directly without the intermediate pcm).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we would have to maintain both the one phase compilation and two phase compilation path. Maybe we can make it better by refining the API.


// 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);
Expand All @@ -1194,13 +1214,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->shouldEmitInExternalSource() && !Def->isInlineSpecified();
return !keyFunction->hasBody(Def);
}

/// Given that we're currently at the end of the translation unit, and
Expand Down
3 changes: 3 additions & 0 deletions clang/lib/CodeGen/ItaniumCXXABI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2130,6 +2130,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()) {
Expand Down
9 changes: 9 additions & 0 deletions clang/lib/Sema/SemaDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18333,6 +18333,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.
Expand Down
14 changes: 9 additions & 5 deletions clang/lib/Sema/SemaDeclCXX.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18706,11 +18706,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 =
Expand Down Expand Up @@ -18755,7 +18759,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
Expand Down
11 changes: 11 additions & 0 deletions clang/lib/Serialization/ASTReader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3903,6 +3903,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(getGlobalDeclID(F, LocalDeclID(Record[I++])));
break;

case IMPORTED_MODULES:
if (!F.isModule()) {
// If we aren't loading a module (which has its own exports), make
Expand Down Expand Up @@ -8067,6 +8074,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;

Expand Down
7 changes: 7 additions & 0 deletions clang/lib/Serialization/ASTReaderDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4219,6 +4219,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) {
Expand Down
23 changes: 23 additions & 0 deletions clang/lib/Serialization/ASTWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -3957,6 +3958,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
//===----------------------------------------------------------------------===//
Expand Down Expand Up @@ -5141,6 +5146,13 @@ void ASTWriter::PrepareWritingSpecialDecls(Sema &SemaRef) {
// Write all of the DeclsToCheckForDeferredDiags.
for (auto *D : SemaRef.DeclsToCheckForDeferredDiags)
GetDeclRef(D);

// Write all classes need to emit the vtable definitions if required.
if (isWritingStdCXXNamedModules())
for (CXXRecordDecl *RD : PendingEmittingVTables)
GetDeclRef(RD);
else
PendingEmittingVTables.clear();
}

void ASTWriter::WriteSpecialDeclRecords(Sema &SemaRef) {
Expand Down Expand Up @@ -5295,6 +5307,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,
Expand Down
6 changes: 6 additions & 0 deletions clang/lib/Serialization/ASTWriterDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1537,8 +1537,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 the key function is meaningless in module purview.
if (D->isCompleteDefinition())
Record.AddDeclRef(Context.getCurrentKeyFunction(D));

Expand Down
31 changes: 19 additions & 12 deletions clang/test/CodeGenCXX/modules-vtable.cppm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would they benefit from still being linkonce? What if the vtable is never used? Or do we leave that up to --gc-sections linker behavior? (I'm not clear on when we use linkage to let things be dropped, and when we use the linker function-sections/data-sections/gc-sections behavior to drop unused stuff)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is linkonce, as long as I understand correctly, if the vtable is not used in the current TU, the middle end is free to remove the unused linkonce entity. But this is not wanted. Since all the things in the module purview is potentially used. Do I misunderstand anything?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, sorry, was misreading the linkages - I guess weak might be applicable?

“weak” linkage has the same merging semantics as linkonce linkage, except that unreferenced globals with weak linkage may not be discarded.
But maybe just normal external linkage is the right thing & we let the linker GC things if the user asks it to. Fair enough.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree weak may work here, I feel it is odd. I feel the default strong linkage seems better when it is applicable. What's the advance to use weak linkage here?

// CHECK-INLINE: @_ZTVW3Mod4Base = {{.*}}unnamed_addr constant
// CHECK-INLINE: @_ZTSW3Mod4Base = {{.*}}constant
// CHECK-INLINE: @_ZTIW3Mod4Base = {{.*}}constant

module :private;
int private_use() {
Expand All @@ -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-INLINE: @_ZTVW3Mod4Base = linkonce_odr {{.*}}unnamed_addr constant
// CHECK-INLINE: @_ZTSW3Mod4Base = linkonce_odr {{.*}}constant
// CHECK-INLINE: @_ZTIW3Mod4Base = linkonce_odr {{.*}}constant
// CHECK-INLINE-NOT: @_ZTSW3Mod4Base
// CHECK-INLINE-NOT: @_ZTIW3Mod4Base
// 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
Expand All @@ -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;
Expand All @@ -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
// CHECK-NOT: @_ZTIW1M1C
Loading
Loading