-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[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
Changes from all commits
b78a951
df987d5
5800242
35489bf
635d348
12b28ca
d684746
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,6 +80,7 @@ | |
#include "SymbolTable.h" | ||
#include "Symbols.h" | ||
#include "SyntheticSections.h" | ||
#include "llvm/ADT/EquivalenceClasses.h" | ||
#include "llvm/BinaryFormat/ELF.h" | ||
#include "llvm/Object/ELF.h" | ||
#include "llvm/Support/Parallel.h" | ||
|
@@ -333,6 +334,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> | ||
|
@@ -535,14 +558,28 @@ template <class ELFT> void ICF<ELFT>::run() { | |
auto print = [&ctx = ctx]() -> ELFSyncStream { | ||
return {ctx, ctx.arg.printIcfSections ? DiagLevel::Msg : DiagLevel::None}; | ||
}; | ||
|
||
EquivalenceClasses<Symbol *> symbolEquivalence; | ||
// 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() || | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 So I think we need to add a check in variableEq, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I followed-up here in this PR: #136641 |
||
!replacedSyms[i]->isGlobal()) | ||
continue; | ||
symbolEquivalence.unionSets(syms[i], replacedSyms[i]); | ||
} | ||
|
||
// At this point we know sections merged are fully identical and hence | ||
// we want to remove duplicate implicit dependencies such as link order | ||
|
@@ -561,11 +598,26 @@ template <class ELFT> void ICF<ELFT>::run() { | |
d->folded = true; | ||
} | ||
}; | ||
for (Symbol *sym : ctx.symtab->getSymbols()) | ||
for (Symbol *sym : ctx.symtab->getSymbols()) { | ||
fold(sym); | ||
auto it = symbolEquivalence.findLeader(sym); | ||
if (it != symbolEquivalence.member_end() && *it != sym) { | ||
print() << "redirecting '" << sym->getName() << "' in symtab to '" | ||
<< (*it)->getName() << "'"; | ||
ctx.symtab->redirect(sym, *it); | ||
} | ||
} | ||
parallelForEach(ctx.objectFiles, [&](ELFFileBase *file) { | ||
for (Symbol *sym : file->getLocalSymbols()) | ||
fold(sym); | ||
for (Symbol *&sym : file->getMutableGlobalSymbols()) { | ||
auto it = symbolEquivalence.findLeader(sym); | ||
if (it != symbolEquivalence.member_end() && *it != sym) { | ||
print() << "redirecting '" << sym->getName() << "' to '" | ||
<< (*it)->getName() << "'"; | ||
sym = *it; | ||
} | ||
} | ||
}); | ||
|
||
// InputSectionDescription::sections is populated by processSectionCommands(). | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
## REQUIRES: aarch64 | ||
## Check that symbols that ICF assumes to be the same get a single GOT entry | ||
|
||
# RUN: llvm-mc -filetype=obj -triple=aarch64 %s -o %t | ||
# RUN: llvm-mc -filetype=obj -crel -triple=aarch64 %s -o %tcrel | ||
# RUN: ld.lld %t -o %t2 --icf=all | ||
# RUN: ld.lld %tcrel -o %tcrel2 --icf=all | ||
|
||
# RUN: llvm-objdump --section-headers %t2 | FileCheck %s --check-prefix=EXE | ||
# RUN: llvm-objdump --section-headers %tcrel2 | FileCheck %s --check-prefix=EXE | ||
|
||
# RUN: ld.lld -shared %t -o %t3 --icf=all | ||
pranavk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# RUN: ld.lld -shared %tcrel -o %tcrel3 --icf=all | ||
|
||
# RUN: llvm-objdump --section-headers %t3 | FileCheck %s --check-prefix=DSO | ||
# RUN: llvm-objdump --section-headers %tcrel3 | FileCheck %s --check-prefix=DSO | ||
|
||
## All global g* symbols should merge into a single GOT entry while non-global | ||
## gets its own GOT entry. | ||
# EXE: {{.*}}.got 00000010{{.*}} | ||
|
||
## When symbols are preemptible in DSO mode, GOT entries wouldn't be merged | ||
# DSO: {{.*}}.got 00000028{{.*}} | ||
|
||
.addrsig | ||
|
||
callee: | ||
ret | ||
|
||
.macro f, index, isglobal | ||
|
||
# (Kept unique) first instruction of the GOT code sequence | ||
.section .text.f1_\index,"ax",@progbits | ||
pranavk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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: | ||
pranavk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.byte 123 | ||
|
||
.section .text._start,"ax",@progbits | ||
bl f1_\index | ||
|
||
.endm | ||
|
||
## Another set of sections merging: g1 <- g2. Linker should be able to | ||
## resolve both g1 and g2 to g0 based on ICF on previous sections. | ||
|
||
.section .text.t1_0,"ax",@progbits | ||
t1_0: | ||
adrp x2, :got:g1 | ||
mov x3, #1 | ||
b t2_0 | ||
|
||
.section .text.t2_0,"ax",@progbits | ||
t2_0: | ||
ldr x2, [x2, :got_lo12:g1] | ||
b callee | ||
|
||
.section .text.t1_1,"ax",@progbits | ||
t1_1: | ||
adrp x2, :got:g2 | ||
mov x3, #2 | ||
b t2_1 | ||
|
||
.section .text.t2_1,"ax",@progbits | ||
t2_1: | ||
ldr x2, [x2, :got_lo12:g2] | ||
b callee | ||
|
||
.section .text._start,"ax",@progbits | ||
.globl _start | ||
_start: | ||
|
||
f 0 1 | ||
f 1 1 | ||
f 2 1 | ||
f 3 1 | ||
f 4 |
Uh oh!
There was an error while loading. Please reload this page.