Skip to content

[Linker] Do not keep a private member of a non-prevailing comdat group #69143

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 1 commit into from
Oct 28, 2023
Merged

[Linker] Do not keep a private member of a non-prevailing comdat group #69143

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
21 changes: 21 additions & 0 deletions llvm/lib/Linker/LinkModules.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,7 @@ void ModuleLinker::dropReplacedComdat(
bool ModuleLinker::run() {
Module &DstM = Mover.getModule();
DenseSet<const Comdat *> ReplacedDstComdats;
DenseSet<const Comdat *> NonPrevailingComdats;

for (const auto &SMEC : SrcM->getComdatSymbolTable()) {
const Comdat &C = SMEC.getValue();
Expand All @@ -473,6 +474,9 @@ bool ModuleLinker::run() {
return true;
ComdatsChosen[&C] = std::make_pair(SK, From);

if (From == LinkFrom::Dst)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think for COFF (not for ELF) in the comdat-refer-to-discarded.ll test case we should get a result of LinkFrom::Both here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I remember it right, LinkFrom::Both corresponds to comdat nodeduplicate, while comdat-refer-to-discarded.ll uses comdat any. Do you think the interpretation of a comdat selection type should depend on the target platform (COFF vs ELF)?

NonPrevailingComdats.insert(&C);

if (From != LinkFrom::Src)
continue;

Expand All @@ -497,6 +501,23 @@ bool ModuleLinker::run() {
for (Function &GV : llvm::make_early_inc_range(DstM))
dropReplacedComdat(GV, ReplacedDstComdats);

if (!NonPrevailingComdats.empty()) {
DenseSet<GlobalObject *> AliasedGlobals;
for (auto &GA : SrcM->aliases())
if (GlobalObject *GO = GA.getAliaseeObject(); GO && GO->getComdat())
AliasedGlobals.insert(GO);
for (const Comdat *C : NonPrevailingComdats) {
SmallVector<GlobalObject *> ToUpdate;
for (GlobalObject *GO : C->getUsers())
if (GO->hasPrivateLinkage() && !AliasedGlobals.contains(GO))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's more correct to use hasLocalLinkage instead of hasPrivateLinkage. Looking at the IRMover call sites, that's what's used there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change would break the Linker/comdat-refer-to-discarded.ll test. Originally, it was Linker/comdat13.ll in baa3bf8, which stated that the duplication of internal globals "matches the expectations of COFF linkers`. Since no high-level example was provided, it is now quite difficult, at least for me, to judge whether that behavior is correct or not, so I would prefer not to change it in my patch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at the test and the history, it is still valid. I think the bug here is that these comdats referring to COFF internal linkage entities are considered NonPrevailingComdats. The proper fix for that is probably in some other part of the code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should go ahead and land this as is. I'll file an issue about the logic bug I think I see.

Copy link
Member

Choose a reason for hiding this comment

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

for (auto &GA : SrcM->aliases()) is not tested. Add some tests?

Copy link
Collaborator Author

@igorkudrin igorkudrin Oct 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

for (auto &GA : SrcM->aliases()) is not tested. Add some tests?

This is covered by Linker/comdat-any.ll, case 3.

ToUpdate.push_back(GO);
for (GlobalObject *GO : ToUpdate) {
GO->setLinkage(GlobalValue::AvailableExternallyLinkage);
GO->setComdat(nullptr);
}
}
}

for (GlobalVariable &GV : SrcM->globals())
if (GV.hasLinkOnceLinkage())
if (const Comdat *SC = GV.getComdat())
Expand Down
29 changes: 29 additions & 0 deletions llvm/test/Linker/comdat-nonprevailing-decl.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
; RUN: rm -rf %t && split-file %s %t
; RUN: llvm-link -S %t/1.ll %t/1-aux.ll -o - | FileCheck %s
; RUN: llvm-link -S %t/2.ll %t/2-aux.ll -o - | FileCheck %s --check-prefix=CHECK2

;--- 1.ll
$c = comdat any
Expand All @@ -23,3 +24,31 @@ define ptr @f3() {
ret ptr @v3
}

;--- 2.ll
;; Check that a private global variable from a non-prevailing comdat group is
;; converted into 'available_externally' and excluded from the comdat group.

; CHECK2: $__profc_foo = comdat any
; CHECK2: @llvm.compiler.used = appending global [2 x ptr] [ptr @__profd_foo.[[SUFFIX:[0-9]+]], ptr @__profd_foo]
; CHECK2: @__profd_foo.[[SUFFIX]] = private global ptr @__profc_foo, comdat($__profc_foo)
; CHECK2: @__profc_foo = linkonce_odr global i64 1, comdat
; CHECK2: @__profd_foo = available_externally dso_local global ptr @__profc_foo{{$}}

$__profc_foo = comdat any
@__profc_foo = linkonce_odr global i64 1, comdat
@__profd_foo = private global ptr @__profc_foo, comdat($__profc_foo)
@llvm.compiler.used = appending global [1 x ptr] [ ptr @__profd_foo ]

define ptr @bar() {
ret ptr @__profc_foo
}

;--- 2-aux.ll
$__profc_foo = comdat any
@__profc_foo = linkonce_odr global i64 1, comdat
@__profd_foo = private global ptr @__profc_foo, comdat($__profc_foo)
@llvm.compiler.used = appending global [1 x ptr] [ ptr @__profd_foo ]

define ptr @baz() {
ret ptr @__profc_foo
}