[ELF] Demote symbols in /DISCARD/ discarded sections to Undefined #69295
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lld-elf @llvm/pr-subscribers-lld Author: Fangrui Song (MaskRay) ChangesWhen an input section is matched by /DISCARD/ in a linker script, GNU ld Implement the error by demoting eligible symbols to It's arguable whether a weak reference to a discarded symbol should lead to Close #58891 Co-authored-by: Bevin Hansson <bevin.hansson@ericsson.com> This an alternative to https://github.com/MaskRay/llvm-project/tree/lld-symbols-in-discard Full diff: https://github.com/llvm/llvm-project/pull/69295.diff 8 Files Affected:
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index d082463d34e576c..5d3c944f18cf15c 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -2248,17 +2248,38 @@ static void replaceCommonSymbols() {
}
}
+static void demoteDefined(Defined &sym, DenseMap<SectionBase *, size_t> &map) {
+ if (map.empty())
+ for (auto [i, sec] : llvm::enumerate(sym.file->getSections()))
+ map.try_emplace(sec, i);
+ // Change WEAK to GLOBAL so that if a scanned relocation references sym,
+ // maybeReportUndefined will report an error.
+ uint8_t binding = sym.isWeak() ? uint8_t(STB_GLOBAL) : sym.binding;
+ Undefined(sym.file, sym.getName(), binding, sym.stOther, sym.type,
+ /*discardedSecIdx=*/map.lookup(sym.section))
+ .overwrite(sym);
+}
+
// If all references to a DSO happen to be weak, the DSO is not added to
// DT_NEEDED. If that happens, replace ShardSymbol with Undefined to avoid
// dangling references to an unneeded DSO. Use a weak binding to avoid
// --no-allow-shlib-undefined diagnostics. Similarly, demote lazy symbols.
-static void demoteSharedAndLazySymbols() {
- llvm::TimeTraceScope timeScope("Demote shared and lazy symbols");
+//
+// In addition, demote symbols defined in discarded sections, so that
+// references to /DISCARD/ discarded symbols will lead to errors.
+static void demoteSymbols() {
+ llvm::TimeTraceScope timeScope("Demote symbols");
+ DenseMap<InputFile *, DenseMap<SectionBase *, size_t>> sectionIndexMap;
for (Symbol *sym : symtab.getSymbols()) {
+ if (auto *d = dyn_cast<Defined>(sym)) {
+ if (d->section && !d->section->isLive())
+ demoteDefined(*d, sectionIndexMap[d->file]);
+ continue;
+ }
+
auto *s = dyn_cast<SharedSymbol>(sym);
if (!(s && !cast<SharedFile>(s->file)->isNeeded) && !sym->isLazy())
continue;
-
uint8_t binding = sym->isLazy() ? sym->binding : uint8_t(STB_WEAK);
Undefined(nullptr, sym->getName(), binding, sym->stOther, sym->type)
.overwrite(*sym);
@@ -2266,6 +2287,18 @@ static void demoteSharedAndLazySymbols() {
}
}
+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->file == file && d->section && !d->section->isLive())
+ demoteDefined(*d, sectionIndexMap);
+ }
+ });
+}
+
// The section referred to by `s` is considered address-significant. Set the
// keepUnique flag on the section if appropriate.
static void markAddrsig(Symbol *s) {
@@ -3023,7 +3056,6 @@ void LinkerDriver::link(opt::InputArgList &args) {
// Garbage collection and removal of shared symbols from unused shared objects.
invokeELFT(markLive,);
- demoteSharedAndLazySymbols();
// Make copies of any input sections that need to be copied into each
// partition.
@@ -3061,6 +3093,14 @@ void LinkerDriver::link(opt::InputArgList &args) {
script->addOrphanSections();
}
+ demoteSymbols();
+ // 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();
+
{
llvm::TimeTraceScope timeScope("Merge/finalize input sections");
diff --git a/lld/ELF/LinkerScript.cpp b/lld/ELF/LinkerScript.cpp
index df091613dc0a144..00e583903f1b455 100644
--- a/lld/ELF/LinkerScript.cpp
+++ b/lld/ELF/LinkerScript.cpp
@@ -613,6 +613,7 @@ 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 18eaf58b785e370..c97fdfab1d2f21c 100644
--- a/lld/ELF/LinkerScript.h
+++ b/lld/ELF/LinkerScript.h
@@ -356,6 +356,7 @@ 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/MapFile.cpp b/lld/ELF/MapFile.cpp
index 1b6dfcc57176b5f..8b10ae183ae35dd 100644
--- a/lld/ELF/MapFile.cpp
+++ b/lld/ELF/MapFile.cpp
@@ -229,7 +229,7 @@ static void writeCref(raw_fd_ostream &os) {
if (isa<SharedSymbol>(sym))
map[sym].insert(file);
if (auto *d = dyn_cast<Defined>(sym))
- if (!d->isLocal() && (!d->section || d->section->isLive()))
+ if (!d->isLocal())
map[d].insert(file);
}
}
diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp
index ee27cc15e040a49..f3fb0c71a8b3064 100644
--- a/lld/ELF/Relocations.cpp
+++ b/lld/ELF/Relocations.cpp
@@ -507,8 +507,7 @@ int64_t RelocationScanner::computeMipsAddend(const RelTy &rel, RelExpr expr,
template <class ELFT>
static std::string maybeReportDiscarded(Undefined &sym) {
auto *file = dyn_cast_or_null<ObjFile<ELFT>>(sym.file);
- if (!file || !sym.discardedSecIdx ||
- file->getSections()[sym.discardedSecIdx] != &InputSection::discarded)
+ if (!file || !sym.discardedSecIdx)
return "";
ArrayRef<typename ELFT::Shdr> objSections =
file->template getELFShdrs<ELFT>();
@@ -1575,7 +1574,8 @@ template <class ELFT> void elf::scanRelocations() {
scanner.template scanSection<ELFT>(*sec);
if (part.armExidx && part.armExidx->isLive())
for (InputSection *sec : part.armExidx->exidxSections)
- scanner.template scanSection<ELFT>(*sec);
+ if (sec->isLive())
+ scanner.template scanSection<ELFT>(*sec);
}
});
}
diff --git a/lld/ELF/Symbols.cpp b/lld/ELF/Symbols.cpp
index 07061d3a1223e9e..88140904f2f8f8e 100644
--- a/lld/ELF/Symbols.cpp
+++ b/lld/ELF/Symbols.cpp
@@ -316,12 +316,13 @@ void elf::maybeWarnUnorderableSymbol(const Symbol *sym) {
if (!config->warnSymbolOrdering)
return;
- // If UnresolvedPolicy::Ignore is used, no "undefined symbol" error/warning
- // is emitted. It makes sense to not warn on undefined symbols.
+ // If UnresolvedPolicy::Ignore is used, no "undefined symbol" error/warning is
+ // emitted. It makes sense to not warn on undefined symbols (excluding those
+ // demoted by demoteSymbolsInDiscardedSections).
//
// Note, ld.bfd --symbol-ordering-file= does not warn on undefined symbols,
// but we don't have to be compatible here.
- if (sym->isUndefined() &&
+ if (sym->isUndefined() && !cast<Undefined>(sym)->discardedSecIdx &&
config->unresolvedSymbols == UnresolvedPolicy::Ignore)
return;
@@ -330,9 +331,12 @@ void elf::maybeWarnUnorderableSymbol(const Symbol *sym) {
auto report = [&](StringRef s) { warn(toString(file) + s + sym->getName()); };
- if (sym->isUndefined())
- report(": unable to order undefined symbol: ");
- else if (sym->isShared())
+ if (sym->isUndefined()) {
+ if (cast<Undefined>(sym)->discardedSecIdx)
+ report(": unable to order discarded symbol: ");
+ else
+ report(": unable to order undefined symbol: ");
+ } else if (sym->isShared())
report(": unable to order shared symbol: ");
else if (d && !d->section)
report(": unable to order absolute symbol: ");
diff --git a/lld/test/ELF/gc-sections-tls.s b/lld/test/ELF/gc-sections-tls.s
index ccc9ac3c74e5670..e476951213440a1 100644
--- a/lld/test/ELF/gc-sections-tls.s
+++ b/lld/test/ELF/gc-sections-tls.s
@@ -7,6 +7,11 @@
# ERR: error: {{.*}}.o has an STT_TLS symbol but doesn't have an SHF_TLS section
+## 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
+
## 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' | \
@@ -17,6 +22,9 @@
# CHECK: Hex dump of section '.noalloc':
# CHECK-NEXT: 0x00000000 {{[0-9a-f]+}} ffffffff
+.globl _start
+_start:
+
.section .tbss,"awT",@nobits
tls:
.long 0
diff --git a/lld/test/ELF/linkerscript/discard-section.s b/lld/test/ELF/linkerscript/discard-section.s
index 9e021ac83f563a4..eba3e772a21f197 100644
--- a/lld/test/ELF/linkerscript/discard-section.s
+++ b/lld/test/ELF/linkerscript/discard-section.s
@@ -4,9 +4,32 @@
# RUN: rm -rf %t && split-file %s %t && cd %t
# RUN: llvm-mc -filetype=obj -triple=x86_64 a.s -o a.o
# RUN: llvm-mc -filetype=obj -triple=x86_64 b.s -o b.o
-# RUN: ld.lld -T a.lds a.o b.o -z undefs -o /dev/null 2>&1 | count 0
-# RUN: ld.lld -T a.lds a.o b.o -o /dev/null 2>&1 | count 0
-# RUN: ld.lld -r -T a.lds a.o b.o -o /dev/null 2>&1 | count 0
+# RUN: not ld.lld --threads=1 -T a.lds a.o b.o -z undefs -o /dev/null 2>&1 | FileCheck %s --check-prefix=SECTION --implicit-check-not=error:
+# RUN: not ld.lld --threads=1 -T a.lds a.o b.o -o /dev/null 2>&1 | FileCheck %s --check-prefixes=SECTION,SYMBOL --implicit-check-not=error:
+# RUN: ld.lld -r -T a.lds a.o b.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=WARNING --implicit-check-not=warning:
+
+# SECTION: error: relocation refers to a discarded section: .aaa
+# SECTION-NEXT: >>> defined in a.o
+# SECTION-NEXT: >>> referenced by a.o:(.bbb+0x0)
+
+# SYMBOL: error: relocation refers to a symbol in a discarded section: global
+# SYMBOL-NEXT: >>> defined in a.o
+# SYMBOL-NEXT: >>> referenced by b.o:(.data+0x0)
+
+# SYMBOL: error: relocation refers to a symbol in a discarded section: weak
+# SYMBOL-NEXT: >>> defined in a.o
+# SYMBOL-NEXT: >>> referenced by b.o:(.data+0x8)
+
+# SYMBOL: error: relocation refers to a symbol in a discarded section: weakref1
+# SYMBOL-NEXT: >>> defined in a.o
+# SYMBOL-NEXT: >>> referenced by b.o:(.data+0x10)
+
+# SYMBOL: error: relocation refers to a symbol in a discarded section: weakref2
+# SYMBOL-NEXT: >>> defined in a.o
+# SYMBOL-NEXT: >>> referenced by b.o:(.data+0x18)
+
+# WARNING: warning: relocation refers to a discarded section: .aaa
+# WARNING-NEXT: >>> referenced by a.o:(.rela.bbb+0x0)
#--- a.s
.globl _start
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
smithp35
approved these changes
Oct 17, 2023
MaskRay
force-pushed
the
lld-symbols-in-discard2
branch
2 times, most recently
from
b110660
to
0011a4f
Compare
MaskRay
added a commit
that referenced
this pull request
Oct 17, 2023
Check that #69295 will fix symbols referenced by relocations that are defined in discarded sections.
MaskRay
added a commit
that referenced
this pull request
Oct 17, 2023
History of demoteSharedSymbols: * https://reviews.llvm.org/D45536 demotes SharedSymbol * https://reviews.llvm.org/D111365 demotes lazy symbols * The pending #69295 will demote symbols defined in discarded sections The pass is placed after markLive just to be clear that it needs `isNeeded` information computed by markLive. The remaining passes in Driver.cpp do not use symbol information. Move the pass to Writer.cpp to be closer to other symbol-related passes.
MaskRay
force-pushed
the
lld-symbols-in-discard2
branch
from
0011a4f
to
df4188e
Compare
|
Rebase |
MaskRay
force-pushed
the
lld-symbols-in-discard2
branch
2 times, most recently
from
e5ddadd
to
a838f5e
Compare
MaskRay
force-pushed
the
lld-symbols-in-discard2
branch
from
a838f5e
to
ac00708
Compare
MaskRay
added a commit
that referenced
this pull request
Oct 17, 2023
…9295) When an input section is matched by /DISCARD/ in a linker script, GNU ld reports errors for relocations referencing symbols defined in the section: `.aaa' referenced in section `.bbb' of a.o: defined in discarded section `.aaa' of a.o Implement the error by demoting eligible symbols to `Undefined` and changing STB_WEAK to STB_GLOBAL. As a side benefit, in relocatable links, relocations referencing symbols defined relative to /DISCARD/ discarded sections no longer set symbol/type to zeros. It's arguable whether a weak reference to a discarded symbol should lead to errors. GNU ld reports an error and our demoting approach reports an error as well. Close #58891 Co-authored-by: Bevin Hansson <bevin.hansson@ericsson.com>
MaskRay
force-pushed
the
lld-symbols-in-discard2
branch
from
ac00708
to
1981b1b
Compare
MaskRay
added a commit
that referenced
this pull request
Oct 17, 2023
One use of setSymbolAndType (related to https://reviews.llvm.org/D53864 "Do not crash when -r output uses linker script with `/DISCARD/`") is no longer needed after commit 1981b1b demotes symbols in discarded sections to Undefined.
MaskRay
added a commit
that referenced
this pull request
Oct 18, 2023
#69425) Follow-up to #69295: 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.
|
Demoting The The hidden visibility is important for the test. If we use |
|
This broke Fuchsia ASan build which is now failing with the following error: I have uploaded the reproducer to https://storage.googleapis.com/fuchsia-build/undef-new-reference.tar.xz, would it be possible to take a look? |
MaskRay
added a commit
to MaskRay/llvm-project
that referenced
this pull request
Mar 14, 2024
llvm#69295 demoted Defined symbols relative to discarded sections. If such a symbol is unreferenced, the desired behavior is to eliminate it from .symtab just like --gc-sections discarded definitions. Linux kernel's CONFIG_DEBUG_FORCE_WEAK_PER_CPU=y configuration expects that the unreferenced `unused` is not emitted to .symtab (ClangBuiltLinux/linux#2006). For relocations referencing demoted symbols, the symbol index restores to 0 like older lld (`R_X86_64_64 0` in `discard-section.s`). Fix llvm#85048
MaskRay
added a commit
that referenced
this pull request
Mar 14, 2024
…85167) #69295 demoted Defined symbols relative to discarded sections. If such a symbol is unreferenced, the desired behavior is to eliminate it from .symtab just like --gc-sections discarded definitions. Linux kernel's CONFIG_DEBUG_FORCE_WEAK_PER_CPU=y configuration expects that the unreferenced `unused` is not emitted to .symtab (ClangBuiltLinux/linux#2006). For relocations referencing demoted symbols, the symbol index restores to 0 like older lld (`R_X86_64_64 0` in `discard-section.s`). Fix #85048
llvmbot
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Mar 14, 2024
…lvm#85167) llvm#69295 demoted Defined symbols relative to discarded sections. If such a symbol is unreferenced, the desired behavior is to eliminate it from .symtab just like --gc-sections discarded definitions. Linux kernel's CONFIG_DEBUG_FORCE_WEAK_PER_CPU=y configuration expects that the unreferenced `unused` is not emitted to .symtab (ClangBuiltLinux/linux#2006). For relocations referencing demoted symbols, the symbol index restores to 0 like older lld (`R_X86_64_64 0` in `discard-section.s`). Fix llvm#85048 (cherry picked from commit 8fe3e70)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When an input section is matched by /DISCARD/ in a linker script, GNU ld
reports errors for relocations referencing symbols defined in the section:
Implement the error by demoting eligible symbols to
Undefinedand changingSTB_WEAK to STB_GLOBAL. As a side benefit, in relocatable links, relocations
referencing symbols defined relative to /DISCARD/ discarded sections no longer
set symbol/type to zeros.
It's arguable whether a weak reference to a discarded symbol should lead to
errors. GNU ld reports an error and our demoting approach reports an error as
well.
Close #58891
Co-authored-by: Bevin Hansson bevin.hansson@ericsson.com