[lld] [lld][ELF] Demote symbols in discarded sections to Undefined. (PR #68777)
Bevin Hansson via llvm-commits
llvm-commits at lists.llvm.org
Fri Oct 13 00:12:28 PDT 2023
https://github.com/bevin-hansson updated https://github.com/llvm/llvm-project/pull/68777
>From c6cd3523ed5d8d424eda47b0b69bcdaf33af041c Mon Sep 17 00:00:00 2001
From: Bevin Hansson <bevin.hansson at ericsson.com>
Date: Wed, 11 Oct 2023 10:24:41 +0200
Subject: [PATCH 1/5] [lld][ELF] Demote symbols in discarded sections to
Undefined.
After linker script processing, it may be the case that sections
were discarded via /DISCARD/, but relocations to symbols in those
sections remain. Currently, such symbols are still considered
Defined even though their section is not live and will not be
placed anywhere. This results in a silently broken binary.
This patch goes through the symbols after placement and changes
them from Defined to Undefined if their section is no longer
live at that point. During relocation processing, we will catch
such undefined symbols and report an error.
See #58891.
---
lld/ELF/Driver.cpp | 24 ++++++++++++++++++++++
lld/ELF/Relocations.cpp | 3 +--
lld/ELF/Symbols.cpp | 11 ++++++----
lld/ELF/Writer.cpp | 16 +++++++++++++++
lld/test/ELF/linkerscript/discard-symbol.s | 16 +++++++++++++++
5 files changed, 64 insertions(+), 6 deletions(-)
create mode 100644 lld/test/ELF/linkerscript/discard-symbol.s
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index 6272276e94b2d35..a49e72ee96ef466 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -3062,6 +3062,30 @@ void LinkerDriver::link(opt::InputArgList &args) {
script->addOrphanSections();
}
+ // Explicitly demote symbols which didn't get placed. If we don't, any
+ // symbol in a discarded section will still be considered defined, even
+ // though it didn't end up in the output, and we get silently broken
+ // binaries.
+ if (script->hasSectionsCommand) {
+ llvm::TimeTraceScope timeScope("Demote symbols in discarded sections");
+ parallelForEach(symtab.getSymbols(), [](Symbol *sym) {
+ if (Defined *d = dyn_cast<Defined>(sym))
+ if (d->section && !d->section->isLive()) {
+ uint32_t secIdx = 0;
+ if (d->file)
+ secIdx = d->file->getSections().drop_while([=](auto *s) {
+ return s != d->section;
+ }).size();
+ // If we don't change the binding from WEAK to GLOBAL here, the
+ // undefined symbol reporting will think this is undefined weak and
+ // not give a warning.
+ Undefined(d->file, sym->getName(), sym->isWeak() ? (uint8_t)STB_GLOBAL
+ : sym->binding,
+ sym->stOther, sym->type, secIdx).overwrite(*sym);
+ }
+ });
+ }
+
{
llvm::TimeTraceScope timeScope("Merge/finalize input sections");
diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp
index ee27cc15e040a49..44111161619605a 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>();
diff --git a/lld/ELF/Symbols.cpp b/lld/ELF/Symbols.cpp
index 07061d3a1223e9e..3f16ede772cb4a4 100644
--- a/lld/ELF/Symbols.cpp
+++ b/lld/ELF/Symbols.cpp
@@ -321,7 +321,7 @@ void elf::maybeWarnUnorderableSymbol(const Symbol *sym) {
//
// 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 +330,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/ELF/Writer.cpp b/lld/ELF/Writer.cpp
index ce72189015348a4..7593bb14e1ffda8 100644
--- a/lld/ELF/Writer.cpp
+++ b/lld/ELF/Writer.cpp
@@ -714,6 +714,22 @@ template <class ELFT> void Writer<ELFT>::copyLocalSymbols() {
// No reason to keep local undefined symbol in symtab.
if (!dr)
continue;
+
+ // Demote locals which did not end up in any partition. This is similar
+ // to what we do in Driver.cpp, but that only works on globals.
+ // Don't do this for section symbols; they are expected to be lost.
+ if (script->hasSectionsCommand && dr->section && !dr->isSection() &&
+ !dr->section->isLive()) {
+ uint32_t secIdx = 0;
+ if (dr->file)
+ secIdx = dr->file->getSections().drop_while([=](auto *s) {
+ return s != dr->section;
+ }).size();
+ Undefined(dr->file, b->getName(), b->binding,
+ b->stOther, b->type, secIdx).overwrite(*b);
+ continue;
+ }
+
if (includeInSymtab(*b) && shouldKeepInSymtab(*dr))
in.symTab->addSymbol(b);
}
diff --git a/lld/test/ELF/linkerscript/discard-symbol.s b/lld/test/ELF/linkerscript/discard-symbol.s
new file mode 100644
index 000000000000000..739cd585af00372
--- /dev/null
+++ b/lld/test/ELF/linkerscript/discard-symbol.s
@@ -0,0 +1,16 @@
+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+# RUN: echo "SECTIONS { /DISCARD/ : { *(.aaa*) } }" > %t.script
+# RUN: not ld.lld -o %t1 --script %t.script %t | FileCheck %s
+
+# CHECK: error: relocation refers to a symbol in a discarded section: aab
+# CHECK-NEXT: defined in {{.*}}/discard-symbol.s.tmp
+# CHECK-NEXT: referenced by {{.*}}/discard-symbol.s.tmp:(.zzz+0x0)
+
+.global aab
+.section .aaa,"a"
+aab:
+ .quad 0
+
+.section .zzz,"a"
+ .quad aab
>From c224d08366c45ca6d6f733fee5dfd4a60a173cc2 Mon Sep 17 00:00:00 2001
From: Bevin Hansson <bevin.hansson at ericsson.com>
Date: Wed, 11 Oct 2023 10:41:45 +0200
Subject: [PATCH 2/5] Fix the test.
---
lld/test/ELF/linkerscript/discard-symbol.s | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lld/test/ELF/linkerscript/discard-symbol.s b/lld/test/ELF/linkerscript/discard-symbol.s
index 739cd585af00372..3c0d9671ed56929 100644
--- a/lld/test/ELF/linkerscript/discard-symbol.s
+++ b/lld/test/ELF/linkerscript/discard-symbol.s
@@ -1,7 +1,7 @@
# REQUIRES: x86
# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
# RUN: echo "SECTIONS { /DISCARD/ : { *(.aaa*) } }" > %t.script
-# RUN: not ld.lld -o %t1 --script %t.script %t | FileCheck %s
+# RUN: not ld.lld -o %t1 --script %t.script %t 2>&1 | FileCheck %s
# CHECK: error: relocation refers to a symbol in a discarded section: aab
# CHECK-NEXT: defined in {{.*}}/discard-symbol.s.tmp
>From 8ca814fa1a3c9a4e8a3e64958d1304f78794890f Mon Sep 17 00:00:00 2001
From: Bevin Hansson <bevin.hansson at ericsson.com>
Date: Thu, 12 Oct 2023 11:01:18 +0200
Subject: [PATCH 3/5] Remove the section symbol exception to see the test
failures.
---
lld/ELF/Writer.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lld/ELF/Writer.cpp b/lld/ELF/Writer.cpp
index 7593bb14e1ffda8..7e884e9eefe5cde 100644
--- a/lld/ELF/Writer.cpp
+++ b/lld/ELF/Writer.cpp
@@ -718,7 +718,7 @@ template <class ELFT> void Writer<ELFT>::copyLocalSymbols() {
// Demote locals which did not end up in any partition. This is similar
// to what we do in Driver.cpp, but that only works on globals.
// Don't do this for section symbols; they are expected to be lost.
- if (script->hasSectionsCommand && dr->section && !dr->isSection() &&
+ if (script->hasSectionsCommand && dr->section &&
!dr->section->isLive()) {
uint32_t secIdx = 0;
if (dr->file)
>From b6256d4ac800b489e1f81b36c9c18ef25f60c479 Mon Sep 17 00:00:00 2001
From: Bevin Hansson <bevin.hansson at ericsson.com>
Date: Fri, 13 Oct 2023 08:58:23 +0200
Subject: [PATCH 4/5] Fix issues with exidx and locals.
---
lld/ELF/Relocations.cpp | 3 ++-
lld/ELF/Writer.cpp | 4 +---
lld/test/ELF/linkerscript/discard-section-reloc.s | 14 ++++++++++++++
lld/test/ELF/linkerscript/discard-section.s | 7 ++++---
4 files changed, 21 insertions(+), 7 deletions(-)
create mode 100644 lld/test/ELF/linkerscript/discard-section-reloc.s
diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp
index 44111161619605a..f3fb0c71a8b3064 100644
--- a/lld/ELF/Relocations.cpp
+++ b/lld/ELF/Relocations.cpp
@@ -1574,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/Writer.cpp b/lld/ELF/Writer.cpp
index 7e884e9eefe5cde..a9c923a16edd55b 100644
--- a/lld/ELF/Writer.cpp
+++ b/lld/ELF/Writer.cpp
@@ -717,9 +717,7 @@ template <class ELFT> void Writer<ELFT>::copyLocalSymbols() {
// Demote locals which did not end up in any partition. This is similar
// to what we do in Driver.cpp, but that only works on globals.
- // Don't do this for section symbols; they are expected to be lost.
- if (script->hasSectionsCommand && dr->section &&
- !dr->section->isLive()) {
+ if (script->hasSectionsCommand && dr->section && !dr->section->isLive()) {
uint32_t secIdx = 0;
if (dr->file)
secIdx = dr->file->getSections().drop_while([=](auto *s) {
diff --git a/lld/test/ELF/linkerscript/discard-section-reloc.s b/lld/test/ELF/linkerscript/discard-section-reloc.s
new file mode 100644
index 000000000000000..e8b2f3567121f1a
--- /dev/null
+++ b/lld/test/ELF/linkerscript/discard-section-reloc.s
@@ -0,0 +1,14 @@
+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+# RUN: echo "SECTIONS { /DISCARD/ : { *(.aaa*) } }" > %t.script
+# RUN: ld.lld -r -o %t1 --script %t.script %t 2>&1 | FileCheck %s
+
+# CHECK: warning: relocation refers to a discarded section: .zzz
+# CHECK-NEXT: referenced by {{.*}}/discard-section-reloc.s.tmp:(.rela.zzz+0x0)
+
+.section .aaa,"a"
+aab:
+ .quad 0
+
+.section .zzz,"a"
+ .quad aab
diff --git a/lld/test/ELF/linkerscript/discard-section.s b/lld/test/ELF/linkerscript/discard-section.s
index fbdff5dfbe84993..935acc518c83201 100644
--- a/lld/test/ELF/linkerscript/discard-section.s
+++ b/lld/test/ELF/linkerscript/discard-section.s
@@ -1,10 +1,11 @@
# REQUIRES: x86
# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
# RUN: echo "SECTIONS { /DISCARD/ : { *(.aaa*) } }" > %t.script
-# RUN: ld.lld -o %t1 --script %t.script %t
-# RUN: llvm-objdump --section-headers %t1 | FileCheck %s
+# RUN: not ld.lld -o %t1 --script %t.script %t 2>&1 | FileCheck %s
-# CHECK-NOT: .aaa
+# CHECK: error: relocation refers to a discarded section: .zzz
+# CHECK-NEXT: defined in {{.*}}/discard-section.s.tmp
+# CHECK-NEXT: referenced by {{.*}}/discard-section.s.tmp:(.zzz+0x0)
.section .aaa,"a"
aab:
>From 3edf74546bc74ae30399ff41ddf4fc7a400a8b80 Mon Sep 17 00:00:00 2001
From: Bevin Hansson <bevin.hansson at ericsson.com>
Date: Fri, 13 Oct 2023 09:11:56 +0200
Subject: [PATCH 5/5] Add seenDiscard to LinkerScript and fix formatting.
---
lld/ELF/Driver.cpp | 15 ++++++++-------
lld/ELF/LinkerScript.cpp | 1 +
lld/ELF/LinkerScript.h | 1 +
lld/ELF/Writer.cpp | 13 +++++++------
4 files changed, 17 insertions(+), 13 deletions(-)
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index a49e72ee96ef466..5fc7b5d12324e7a 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -3066,22 +3066,23 @@ void LinkerDriver::link(opt::InputArgList &args) {
// symbol in a discarded section will still be considered defined, even
// though it didn't end up in the output, and we get silently broken
// binaries.
- if (script->hasSectionsCommand) {
+ if (script->seenDiscard) {
llvm::TimeTraceScope timeScope("Demote symbols in discarded sections");
parallelForEach(symtab.getSymbols(), [](Symbol *sym) {
if (Defined *d = dyn_cast<Defined>(sym))
if (d->section && !d->section->isLive()) {
uint32_t secIdx = 0;
if (d->file)
- secIdx = d->file->getSections().drop_while([=](auto *s) {
- return s != d->section;
- }).size();
+ secIdx = d->file->getSections()
+ .drop_while([=](auto *s) { return s != d->section; })
+ .size();
// If we don't change the binding from WEAK to GLOBAL here, the
// undefined symbol reporting will think this is undefined weak and
// not give a warning.
- Undefined(d->file, sym->getName(), sym->isWeak() ? (uint8_t)STB_GLOBAL
- : sym->binding,
- sym->stOther, sym->type, secIdx).overwrite(*sym);
+ Undefined(d->file, sym->getName(),
+ sym->isWeak() ? (uint8_t)STB_GLOBAL : sym->binding,
+ sym->stOther, sym->type, secIdx)
+ .overwrite(*sym);
}
});
}
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/Writer.cpp b/lld/ELF/Writer.cpp
index a9c923a16edd55b..75d8f670fed287a 100644
--- a/lld/ELF/Writer.cpp
+++ b/lld/ELF/Writer.cpp
@@ -717,14 +717,15 @@ template <class ELFT> void Writer<ELFT>::copyLocalSymbols() {
// Demote locals which did not end up in any partition. This is similar
// to what we do in Driver.cpp, but that only works on globals.
- if (script->hasSectionsCommand && dr->section && !dr->section->isLive()) {
+ if (script->seenDiscard && dr->section && !dr->section->isLive()) {
uint32_t secIdx = 0;
if (dr->file)
- secIdx = dr->file->getSections().drop_while([=](auto *s) {
- return s != dr->section;
- }).size();
- Undefined(dr->file, b->getName(), b->binding,
- b->stOther, b->type, secIdx).overwrite(*b);
+ secIdx = dr->file->getSections()
+ .drop_while([=](auto *s) { return s != dr->section; })
+ .size();
+ Undefined(dr->file, b->getName(), b->binding, b->stOther, b->type,
+ secIdx)
+ .overwrite(*b);
continue;
}
More information about the llvm-commits
mailing list