Skip to content

Commit fd3fecf

Browse files
committed
Revert "[lld] Merge equivalent symbols found during ICF (#134342)"
The change would also merge *non-equivalent* symbols under some circumstances, see comment with a reproducer on the PR. > Fixes a correctness issue for AArch64 when ADRP and LDR instructions are > outlined in separate sections and sections are fed to ICF for > deduplication. > > See test case (based on > #129122) for details. All > rodata.* sections are folded into a single section with ICF. This leads > to all f2_* function sections getting folded into one (as their > relocation target symbols g* belong to .rodata.g* sections that have > already been folded into one). Since relocations still refer original g* > symbols, we end up creating duplicate GOT entry for all such symbols. > This PR addresses that by tracking such folded symbols and create one > GOT entry for all such symbols. > > Fixes #129122 > > Co-authored by: @jyknight This reverts commit 8389d6f.
1 parent 9876343 commit fd3fecf

File tree

5 files changed

+1
-161
lines changed

5 files changed

+1
-161
lines changed

lld/ELF/ICF.cpp

Lines changed: 1 addition & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,6 @@
8080
#include "SymbolTable.h"
8181
#include "Symbols.h"
8282
#include "SyntheticSections.h"
83-
#include "llvm/ADT/EquivalenceClasses.h"
8483
#include "llvm/BinaryFormat/ELF.h"
8584
#include "llvm/Object/ELF.h"
8685
#include "llvm/Support/Parallel.h"
@@ -334,28 +333,6 @@ bool ICF<ELFT>::equalsConstant(const InputSection *a, const InputSection *b) {
334333
: constantEq(a, ra.relas, b, rb.relas);
335334
}
336335

337-
template <class RelTy>
338-
static SmallVector<Symbol *> getReloc(const InputSection *sec,
339-
Relocs<RelTy> relocs) {
340-
SmallVector<Symbol *> syms;
341-
for (auto ri = relocs.begin(), re = relocs.end(); ri != re; ++ri) {
342-
Symbol &sym = sec->file->getRelocTargetSym(*ri);
343-
syms.push_back(&sym);
344-
}
345-
return syms;
346-
}
347-
348-
template <class ELFT>
349-
static SmallVector<Symbol *> getRelocTargetSyms(const InputSection *sec) {
350-
const RelsOrRelas<ELFT> rel = sec->template relsOrRelas<ELFT>();
351-
if (rel.areRelocsCrel())
352-
return getReloc(sec, rel.crels);
353-
if (rel.areRelocsRel())
354-
return getReloc(sec, rel.rels);
355-
356-
return getReloc(sec, rel.relas);
357-
}
358-
359336
// Compare two lists of relocations. Returns true if all pairs of
360337
// relocations point to the same section in terms of ICF.
361338
template <class ELFT>
@@ -560,28 +537,14 @@ template <class ELFT> void ICF<ELFT>::run() {
560537
auto print = [&ctx = ctx]() -> ELFSyncStream {
561538
return {ctx, ctx.arg.printIcfSections ? DiagLevel::Msg : DiagLevel::None};
562539
};
563-
564-
EquivalenceClasses<Symbol *> symbolEquivalence;
565540
// Merge sections by the equivalence class.
566-
// Merge symbols identified as equivalent during ICF.
567541
forEachClassRange(0, sections.size(), [&](size_t begin, size_t end) {
568542
if (end - begin == 1)
569543
return;
570544
print() << "selected section " << sections[begin];
571-
SmallVector<Symbol *> syms = getRelocTargetSyms<ELFT>(sections[begin]);
572545
for (size_t i = begin + 1; i < end; ++i) {
573546
print() << " removing identical section " << sections[i];
574547
sections[begin]->replace(sections[i]);
575-
SmallVector<Symbol *> replacedSyms =
576-
getRelocTargetSyms<ELFT>(sections[i]);
577-
assert(syms.size() == replacedSyms.size() &&
578-
"Should have same number of syms!");
579-
for (size_t i = 0; i < syms.size(); i++) {
580-
if (syms[i] == replacedSyms[i] || !syms[i]->isGlobal() ||
581-
!replacedSyms[i]->isGlobal())
582-
continue;
583-
symbolEquivalence.unionSets(syms[i], replacedSyms[i]);
584-
}
585548

586549
// At this point we know sections merged are fully identical and hence
587550
// we want to remove duplicate implicit dependencies such as link order
@@ -600,26 +563,11 @@ template <class ELFT> void ICF<ELFT>::run() {
600563
d->folded = true;
601564
}
602565
};
603-
for (Symbol *sym : ctx.symtab->getSymbols()) {
566+
for (Symbol *sym : ctx.symtab->getSymbols())
604567
fold(sym);
605-
auto it = symbolEquivalence.findLeader(sym);
606-
if (it != symbolEquivalence.member_end() && *it != sym) {
607-
print() << "redirecting '" << sym->getName() << "' in symtab to '"
608-
<< (*it)->getName() << "'";
609-
ctx.symtab->redirect(sym, *it);
610-
}
611-
}
612568
parallelForEach(ctx.objectFiles, [&](ELFFileBase *file) {
613569
for (Symbol *sym : file->getLocalSymbols())
614570
fold(sym);
615-
for (Symbol *&sym : file->getMutableGlobalSymbols()) {
616-
auto it = symbolEquivalence.findLeader(sym);
617-
if (it != symbolEquivalence.member_end() && *it != sym) {
618-
print() << "redirecting '" << sym->getName() << "' to '"
619-
<< (*it)->getName() << "'";
620-
sym = *it;
621-
}
622-
}
623571
});
624572

625573
// InputSectionDescription::sections is populated by processSectionCommands().

lld/ELF/SymbolTable.cpp

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,12 +29,6 @@ using namespace llvm::ELF;
2929
using namespace lld;
3030
using namespace lld::elf;
3131

32-
void SymbolTable::redirect(Symbol *from, Symbol *to) {
33-
int &fromIdx = symMap[CachedHashStringRef(from->getName())];
34-
const int toIdx = symMap[CachedHashStringRef(to->getName())];
35-
fromIdx = toIdx;
36-
}
37-
3832
void SymbolTable::wrap(Symbol *sym, Symbol *real, Symbol *wrap) {
3933
// Redirect __real_foo to the original foo and foo to the original __wrap_foo.
4034
int &idx1 = symMap[CachedHashStringRef(sym->getName())];

lld/ELF/SymbolTable.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@ class SymbolTable {
4141
SymbolTable(Ctx &ctx) : ctx(ctx) {}
4242
ArrayRef<Symbol *> getSymbols() const { return symVector; }
4343

44-
void redirect(Symbol *from, Symbol *to);
4544
void wrap(Symbol *sym, Symbol *real, Symbol *wrap);
4645

4746
Symbol *insert(StringRef name);

lld/test/ELF/aarch64-got-merging-icf.s

Lines changed: 0 additions & 95 deletions
This file was deleted.

lld/test/ELF/icf-preemptible.s

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,6 @@
1717
# EXE-NEXT: selected section {{.*}}:(.text.h1)
1818
# EXE-NEXT: removing identical section {{.*}}:(.text.h2)
1919
# EXE-NEXT: removing identical section {{.*}}:(.text.h3)
20-
# EXE-NEXT: redirecting 'f2' in symtab to 'f1'
21-
# EXE-NEXT: redirecting 'g2' in symtab to 'g1'
22-
# EXE-NEXT: redirecting 'g3' in symtab to 'g1'
23-
# EXE-NEXT: redirecting 'f2' to 'f1'
24-
# EXE-NEXT: redirecting 'g2' to 'g1'
25-
# EXE-NEXT: redirecting 'g3' to 'g1'
2620
# EXE-NOT: {{.}}
2721

2822
## Definitions are preemptible in a DSO. Only leaf functions can be folded.

0 commit comments

Comments
 (0)