Skip to content

[lld] Merge equivalent symbols found during ICF #134342

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.

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 7 commits into from
Apr 21, 2025
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
51 changes: 50 additions & 1 deletion lld/ELF/ICF.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,28 @@ bool ICF<ELFT>::equalsConstant(const InputSection *a, const InputSection *b) {
: constantEq(a, ra.relas, b, rb.relas);
}

template <class RelTy>
static SmallVector<Symbol *> getReloc(const InputSection *sec,
Relocs<RelTy> relocs) {
SmallVector<Symbol *> syms;
for (auto ri = relocs.begin(), re = relocs.end(); ri != re; ++ri) {
Symbol &sym = sec->file->getRelocTargetSym(*ri);
syms.push_back(&sym);
}
return syms;
}

template <class ELFT>
static SmallVector<Symbol *> getRelocTargetSyms(const InputSection *sec) {
const RelsOrRelas<ELFT> rel = sec->template relsOrRelas<ELFT>();
if (rel.areRelocsCrel())
return getReloc(sec, rel.crels);
if (rel.areRelocsRel())
return getReloc(sec, rel.rels);

return getReloc(sec, rel.relas);
}

// Compare two lists of relocations. Returns true if all pairs of
// relocations point to the same section in terms of ICF.
template <class ELFT>
Expand Down Expand Up @@ -535,14 +557,35 @@ template <class ELFT> void ICF<ELFT>::run() {
auto print = [&ctx = ctx]() -> ELFSyncStream {
return {ctx, ctx.arg.printIcfSections ? DiagLevel::Msg : DiagLevel::None};
};

DenseMap<Symbol *, Symbol *> symbolMap;
// Merge sections by the equivalence class.
// Merge symbols identified as equivalent during ICF
forEachClassRange(0, sections.size(), [&](size_t begin, size_t end) {
if (end - begin == 1)
return;
print() << "selected section " << sections[begin];
SmallVector<Symbol *> syms = getRelocTargetSyms<ELFT>(sections[begin]);
for (size_t i = begin + 1; i < end; ++i) {
print() << " removing identical section " << sections[i];
sections[begin]->replace(sections[i]);
SmallVector<Symbol *> replacedSyms =
getRelocTargetSyms<ELFT>(sections[i]);
assert(syms.size() == replacedSyms.size() &&
"Should have same number of syms!");
for (size_t i = 0; i < syms.size(); i++) {
if (syms[i] == replacedSyms[i] || !syms[i]->isGlobal() ||
Copy link
Member

Choose a reason for hiding this comment

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

Since you're checking isGlobal only here, and not in variableEq, doesn't that mean that any non-trivial relocation (such as GOT) which references a local symbol will remain broken? Maybe ELF producers don't need to create a GOT reloc pointing to a local symbol, but it's not invalid to do so.

We can't (at least not right now) get rid of the isGlobal check, because we cannot actually canonicalize local symbols across TUs, since code doing loops over getLocalSymbols() for each file in ctx.objectFiles expects to see no duplicates. (which was a bug in my original version).

So I think we need to add a check in variableEq, if((!sa.isGlobal() || !sb.isGlobal()) && !ra->"isTrivialRelocType"()) return false;. (I made up "isTrivialRelocType"; do we have a relevant predicate already that would determine if a relocation has semantics beyond just the offset?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, as I mentioned in my comment earlier, I plan to do that in a separate PR as it's going to change the behavior of two sections getting ICF-merged (which also changes a icf-ineligible.s test as you noticed earlier).

Copy link
Member

Choose a reason for hiding this comment

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

OK with me to do in another PR, as long as we're agreed the issue hasn't fully been fixed until that's also done.

The change doesn't need to affect icf-ineligible, though, since that test uses only trivial relocations, where we don't need to replace the symbols.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed-up here in this PR: #136641

!replacedSyms[i]->isGlobal())
continue;
auto [it, inserted] =
symbolMap.insert(std::make_pair(replacedSyms[i], syms[i]));
print() << " selected symbol: " << syms[i]->getName().data()
<< "; replaced symbol: " << replacedSyms[i]->getName().data();
if (!inserted) {
print() << " replacement already exists: "
<< it->getSecond()->getName().data();
}
}

// At this point we know sections merged are fully identical and hence
// we want to remove duplicate implicit dependencies such as link order
Expand All @@ -561,11 +604,17 @@ template <class ELFT> void ICF<ELFT>::run() {
d->folded = true;
}
};
for (Symbol *sym : ctx.symtab->getSymbols())
for (Symbol *sym : ctx.symtab->getSymbols()) {
fold(sym);
if (Symbol *s = symbolMap.lookup(sym))
ctx.symtab->redirect(sym, s);
}
parallelForEach(ctx.objectFiles, [&](ELFFileBase *file) {
for (Symbol *sym : file->getLocalSymbols())
fold(sym);
for (Symbol *&sym : file->getMutableGlobalSymbols())
if (Symbol *s = symbolMap.lookup(sym))
sym = s;
});

// InputSectionDescription::sections is populated by processSectionCommands().
Expand Down
7 changes: 7 additions & 0 deletions lld/ELF/SymbolTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,13 @@ using namespace llvm::ELF;
using namespace lld;
using namespace lld::elf;

void SymbolTable::redirect(Symbol *from, Symbol *to) {
int &fromIdx = symMap[CachedHashStringRef(from->getName())];
const int toIdx = symMap[CachedHashStringRef(to->getName())];

fromIdx = toIdx;
}

void SymbolTable::wrap(Symbol *sym, Symbol *real, Symbol *wrap) {
// Redirect __real_foo to the original foo and foo to the original __wrap_foo.
int &idx1 = symMap[CachedHashStringRef(sym->getName())];
Expand Down
1 change: 1 addition & 0 deletions lld/ELF/SymbolTable.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ class SymbolTable {
SymbolTable(Ctx &ctx) : ctx(ctx) {}
ArrayRef<Symbol *> getSymbols() const { return symVector; }

void redirect(Symbol *from, Symbol *to);
void wrap(Symbol *sym, Symbol *real, Symbol *wrap);

Symbol *insert(StringRef name);
Expand Down
60 changes: 60 additions & 0 deletions lld/test/ELF/aarch64-got-merging-icf.s
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
// REQUIRES: aarch64

# RUN: llvm-mc -filetype=obj -triple=aarch64 %s -o %t
# RUN: ld.lld %t -o %t2 --icf=all
# RUN: llvm-objdump --section-headers %t2 | FileCheck %s --check-prefix=EXE

# RUN: ld.lld -shared %t -o %t3 --icf=all
# RUN: llvm-objdump --section-headers %t3 | FileCheck %s --check-prefix=DSO

## All .rodata.* sections should merge into a single GOT entry
# EXE: {{.*}}.got 00000010{{.*}}

## When symbols are preemptible in DSO mode, GOT entries wouldn't be merged
# DSO: {{.*}}.got 00000020{{.*}}

.addrsig

callee:
ret

.macro f, index, isglobal

# (Kept unique) first instruction of the GOT code sequence
.section .text.f1_\index,"ax",@progbits
f1_\index:
adrp x0, :got:g\index
mov x1, #\index
b f2_\index

# Folded, second instruction of the GOT code sequence
.section .text.f2_\index,"ax",@progbits
f2_\index:
ldr x0, [x0, :got_lo12:g\index]
b callee

# Folded
.ifnb \isglobal
.globl g\index
.endif
.section .rodata.g\index,"a",@progbits
g_\index:
.long 111
.long 122

g\index:
.byte 123

.section .text._start,"ax",@progbits
bl f1_\index

.endm

.section .text._start,"ax",@progbits
.globl _start
_start:

f 0 1
f 1 1
f 2 1
f 3
3 changes: 3 additions & 0 deletions lld/test/ELF/icf-preemptible.s
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,15 @@
# EXE-NOT: {{.}}
# EXE: selected section {{.*}}:(.text.g1)
# EXE-NEXT: removing identical section {{.*}}:(.text.g2)
# EXE-NEXT: selected symbol: f1; replaced symbol: f2
# EXE-NEXT: removing identical section {{.*}}:(.text.g3)
# EXE-NEXT: selected section {{.*}}:(.text.f1)
# EXE-NEXT: removing identical section {{.*}}:(.text.f2)
# EXE-NEXT: selected section {{.*}}:(.text.h1)
# EXE-NEXT: removing identical section {{.*}}:(.text.h2)
# EXE-NEXT: selected symbol: g1; replaced symbol: g2
# EXE-NEXT: removing identical section {{.*}}:(.text.h3)
# EXE-NEXT: selected symbol: g1; replaced symbol: g3
# EXE-NOT: {{.}}

## Definitions are preemptible in a DSO. Only leaf functions can be folded.
Expand Down
Loading