Skip to content

Fixes #3589: ELF COMDAT needed on templates to enable linker data culling #4747

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

Closed
ljmf00-wekaio wants to merge 1 commit into from
Closed

Fixes #3589: ELF COMDAT needed on templates to enable linker data culling #4747

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 21 additions & 1 deletion gen/tollvm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,15 @@ LLGlobalValue::LinkageTypes DtoLinkageOnly(Dsymbol *sym) {
}

LinkageWithCOMDAT DtoLinkage(Dsymbol *sym) {
return {DtoLinkageOnly(sym), needsCOMDAT()};
bool hasCOMDAT = needsCOMDAT();
auto appliedLinkage = DtoLinkageOnly(sym);

// generate COMDAT for templates to enable linking data culling
// See https://github.com/ldc-developers/ldc/issues/3589 .
if (sym->isInstantiated())
hasCOMDAT |= appliedLinkage != LLGlobalValue::InternalLinkage;

return {appliedLinkage, hasCOMDAT && supportsCOMDAT()};
}

bool needsCOMDAT() {
Expand All @@ -274,6 +282,18 @@ bool needsCOMDAT() {
return global.params.targetTriple->isOSBinFormatCOFF();
}

bool supportsCOMDAT() {
const auto &triple = *global.params.targetTriple;
return !(triple.isOSBinFormatMachO() ||
#if LDC_LLVM_VER >= 500
triple.isOSBinFormatWasm()
#else
triple.getArch() == llvm::Triple::wasm32 ||
triple.getArch() == llvm::Triple::wasm64
#endif
);
Comment on lines +287 to +294
Copy link
Contributor

@the-horo the-horo Sep 7, 2024

Choose a reason for hiding this comment

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

Suggested change
return !(triple.isOSBinFormatMachO() ||
#if LDC_LLVM_VER >= 500
triple.isOSBinFormatWasm()
#else
triple.getArch() == llvm::Triple::wasm32 ||
triple.getArch() == llvm::Triple::wasm64
#endif
);
return !(triple.isOSBinFormatMachO() || triple.isOSBinFormatWasm());

Even if isOSBinFormatWasm was added in llvm 5, ldc only supports >=llvm-15 so LDC_LLVM_VER will always be >= 1500

Copy link
Author

@ljmf00-wekaio ljmf00-wekaio Sep 8, 2024

Choose a reason for hiding this comment

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

This was picked from older code, before it got removed. I'll remove this, since llvm-5 is no longer supported by LDC.

}

void setLinkage(LinkageWithCOMDAT lwc, llvm::GlobalObject *obj) {
obj->setLinkage(lwc.first);
obj->setComdat(lwc.second ? gIR->module.getOrInsertComdat(obj->getName())
Expand Down
1 change: 1 addition & 0 deletions gen/tollvm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ typedef std::pair<llvm::GlobalValue::LinkageTypes, bool> LinkageWithCOMDAT;
LinkageWithCOMDAT DtoLinkage(Dsymbol *sym);

bool needsCOMDAT();
bool supportsCOMDAT();
void setLinkage(LinkageWithCOMDAT lwc, llvm::GlobalObject *obj);
// Sets linkage and visibility of the specified IR symbol based on the specified
// D symbol.
Expand Down