[ELF] Merge copyLocalSymbols and demoteLocalSymbolsInDiscardedSections #69425
Conversation
In `Writer<ELFT>::run`, the symbol passes are flexible: they can be placed almost everywhere before `scanRelocations`, with a constraint that the `computeIsPreemptible` pass must be invoked for linker-defined non-local symbols. Merge copyLocalSymbols and demoteLocalSymbolsInDiscardedSections to simplify code: * Demoting local symbols can be made unconditional, not constrainted to /DISCARD/ uses due to performance concerns * `includeInSymtab` can be made faster * Make symbol passes close to each other * Decrease data cache misses due to saving an iteration over local symbols There is no speedup, likely due to the unconditional `dr->section` access in `demoteAndCopyLocalSymbols`. `gc-sections-tls.s` no longer reports an error because the TLS symbol is converted to an Undefined.
|
@llvm/pr-subscribers-lld-elf @llvm/pr-subscribers-lld Author: Fangrui Song (MaskRay) ChangesIn Merge copyLocalSymbols and demoteLocalSymbolsInDiscardedSections to simplify
There is no speedup, likely due to the unconditional
Full diff: https://github.com/llvm/llvm-project/pull/69425.diff 4 Files Affected:
diff --git a/lld/ELF/LinkerScript.cpp b/lld/ELF/LinkerScript.cpp
index 00e583903f1b455..df091613dc0a144 100644
--- a/lld/ELF/LinkerScript.cpp
+++ b/lld/ELF/LinkerScript.cpp
@@ -613,7 +613,6 @@ void LinkerScript::processSectionCommands() {
discard(*s);
discardSynthetic(*osec);
osec->commands.clear();
- seenDiscard = true;
return false;
}
diff --git a/lld/ELF/LinkerScript.h b/lld/ELF/LinkerScript.h
index c97fdfab1d2f21c..18eaf58b785e370 100644
--- a/lld/ELF/LinkerScript.h
+++ b/lld/ELF/LinkerScript.h
@@ -356,7 +356,6 @@ class LinkerScript final {
bool hasSectionsCommand = false;
bool seenDataAlign = false;
- bool seenDiscard = false;
bool seenRelroEnd = false;
bool errorOnMissingSection = false;
std::string backwardDotErr;
diff --git a/lld/ELF/Writer.cpp b/lld/ELF/Writer.cpp
index 6f00c7ff8c0d1a8..57e1aa06c6aa873 100644
--- a/lld/ELF/Writer.cpp
+++ b/lld/ELF/Writer.cpp
@@ -53,7 +53,6 @@ template <class ELFT> class Writer {
void run();
private:
- void copyLocalSymbols();
void addSectionSymbols();
void sortSections();
void resolveShfLinkOrder();
@@ -292,18 +291,6 @@ static void demoteSymbolsAndComputeIsPreemptible() {
}
}
-static void demoteLocalSymbolsInDiscardedSections() {
- llvm::TimeTraceScope timeScope("Demote local symbols");
- parallelForEach(ctx.objectFiles, [&](ELFFileBase *file) {
- DenseMap<SectionBase *, size_t> sectionIndexMap;
- for (Symbol *sym : file->getLocalSymbols()) {
- Defined *d = dyn_cast<Defined>(sym);
- if (d && d->section && !d->section->isLive())
- demoteDefined(*d, sectionIndexMap);
- }
- });
-}
-
// Fully static executables don't support MTE globals at this point in time, as
// we currently rely on:
// - A dynamic loader to process relocations, and
@@ -598,11 +585,6 @@ template <class ELFT> void elf::createSyntheticSections() {
// The main function of the writer.
template <class ELFT> void Writer<ELFT>::run() {
- copyLocalSymbols();
-
- if (config->copyRelocs)
- addSectionSymbols();
-
// Now that we have a complete set of output sections. This function
// completes section contents. For example, we need to add strings
// to the string table, and add entries to .got and .plt.
@@ -751,31 +733,33 @@ bool lld::elf::includeInSymtab(const Symbol &b) {
SectionBase *sec = d->section;
if (!sec)
return true;
+ assert(sec->isLive());
if (auto *s = dyn_cast<MergeInputSection>(sec))
return s->getSectionPiece(d->value).live;
- return sec->isLive();
+ return true;
}
return b.used || !config->gcSections;
}
-// Local symbols are not in the linker's symbol table. This function scans
-// each object file's symbol table to copy local symbols to the output.
-template <class ELFT> void Writer<ELFT>::copyLocalSymbols() {
- if (!in.symTab)
- return;
+// Scan local symbols to:
+//
+// - demote symbols defined relative to /DISCARD/ discarded input sections so
+// that relocations referencing them will lead to errors.
+// - copy eligible symbols to .symTab
+static void demoteAndCopyLocalSymbols() {
llvm::TimeTraceScope timeScope("Add local symbols");
- if (config->copyRelocs && config->discard != DiscardPolicy::None)
- markUsedLocalSymbols<ELFT>();
for (ELFFileBase *file : ctx.objectFiles) {
+ DenseMap<SectionBase *, size_t> sectionIndexMap;
for (Symbol *b : file->getLocalSymbols()) {
assert(b->isLocal() && "should have been caught in initializeSymbols()");
auto *dr = dyn_cast<Defined>(b);
-
- // No reason to keep local undefined symbol in symtab.
if (!dr)
continue;
- if (includeInSymtab(*b) && shouldKeepInSymtab(*dr))
+
+ if (dr->section && !dr->section->isLive())
+ demoteDefined(*dr, sectionIndexMap);
+ else if (in.symTab && includeInSymtab(*b) && shouldKeepInSymtab(*dr))
in.symTab->addSymbol(b);
}
}
@@ -1991,12 +1975,13 @@ template <class ELFT> void Writer<ELFT>::finalizeSections() {
}
demoteSymbolsAndComputeIsPreemptible();
- // Also demote local symbols defined relative to discarded input sections so
- // that relocations referencing them will lead to errors. To avoid unneeded
- // work, we only do this when /DISCARD/ is seen, but this demotation also
- // applies to --gc-sections discarded sections.
- if (script->seenDiscard)
- demoteLocalSymbolsInDiscardedSections();
+
+ if (config->copyRelocs && config->discard != DiscardPolicy::None)
+ markUsedLocalSymbols<ELFT>();
+ demoteAndCopyLocalSymbols();
+
+ if (config->copyRelocs)
+ addSectionSymbols();
// Change values of linker-script-defined symbols from placeholders (assigned
// by declareSymbols) to actual definitions.
diff --git a/lld/test/ELF/gc-sections-tls.s b/lld/test/ELF/gc-sections-tls.s
index edcf30e264909e0..3036a676dde1235 100644
--- a/lld/test/ELF/gc-sections-tls.s
+++ b/lld/test/ELF/gc-sections-tls.s
@@ -1,31 +1,25 @@
# REQUIRES: x86
# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
-## Relocation in a non .debug_* referencing a discarded TLS symbol is invalid.
-## If we happen to have no PT_TLS, we will emit an error.
-# RUN: not ld.lld %t.o --gc-sections -o /dev/null 2>&1 | FileCheck %s --check-prefix=ERR
-
-# ERR: error: {{.*}}.o has an STT_TLS symbol but doesn't have an SHF_TLS section
-
-## TODO As a corner case, when /DISCARD/ is present, demoteLocalSymbolsInDiscardedSections
-## demotes tls and the error is not triggered.
-# RUN: echo 'SECTIONS { /DISCARD/ : {} }' > %t.lds
-# RUN: ld.lld %t.o --gc-sections -T %t.lds -o /dev/null
+## When a TLS section is discarded, we will resolve the relocation in a non-SHF_ALLOC
+## section to the addend. Technically, we can emit an error in this case as the
+## relocation type is not TLS.
+# RUN: ld.lld %t.o --gc-sections -o %t
+# RUN: llvm-readelf -x .noalloc %t | FileCheck %s
-## If we happen to have a PT_TLS, we will resolve the relocation to
-## an arbitrary value (current implementation uses a negative value).
# RUN: echo '.section .tbss,"awT"; .globl root; root: .long 0' | \
# RUN: llvm-mc -filetype=obj -triple=x86_64 - -o %t1.o
# RUN: ld.lld --gc-sections -u root %t.o %t1.o -o %t
# RUN: llvm-readelf -x .noalloc %t | FileCheck %s
# CHECK: Hex dump of section '.noalloc':
-# CHECK-NEXT: 0x00000000 {{[0-9a-f]+}} ffffffff
+# CHECK-NEXT: 0x00000000 00800000 00000000
.globl _start
_start:
.section .tbss,"awT",@nobits
+ .long 0
tls:
.long 0
|
|
I think we're seeing an issue with this patch when building Fuchsia. I haven't bisected precisely yet, but given that this is the only ELF/LLD change in the blame it seems highly likely. Can you take a look? I had to split the reproducer w/ zip. You should be able to recombine them in the following way: The following page has more details if needed: https://superuser.com/questions/336219/how-do-i-split-a-zip-file-into-multiple-segments |
|
hmm, seems like I copied a truncated log message from the bot diagnostic. This should be the full output. |
|
After bisecting it seems like bbf7b9d is when the issue crops up. Sorry for pinging the wrong PR. Our CI infra hasn't adapted to linkify github PR numbers yet, and I must have copied the wrong PR number. At least I pinged the right person, but still not great. I'll file a separate issue and CC you directly. |
I saw your internal message as well. Do you mean that bbf7b9d ("[ELF] Remove unused setSymbolAndType after #69295. NFC") introduced a regression which is fixed by this patch? This ensures for every Thank you for the zipped reproduce files. I think we miss some coverage for --emit-relocs or -r. |
I have confirmed that this patch has actually fixed this regression ( Adding a test ab301d8 to prevent regression. |
Sorry, I missed this reply. Yes, that was what I meant.
NP. It's what I would want, so I'm happy to do the minimum :) |
Follow-up to #69295: In
Writer<ELFT>::run, the symbol passes are flexible:they can be placed almost everywhere before
scanRelocations, with a constraintthat the
computeIsPreemptiblepass must be invoked for linker-definednon-local symbols.
Merge copyLocalSymbols and demoteLocalSymbolsInDiscardedSections to simplify
code:
includeInSymtabcan be made fasterThere is no speedup, likely due to the unconditional
dr->sectionaccess indemoteAndCopyLocalSymbols.gc-sections-tls.sno longer reports an error because the TLS symbol isconverted to an Undefined.