This is an archive of the discontinued LLVM Phabricator instance.

Differential D30033

[ELF] - Do not crash when discarding sections that are referenced by others.
ClosedPublic

Authored by grimar on Feb 16 2017, 2:10 AM.

Details

Reviewers
ruiu
• rafael
Commits
rG505ac8dc412a: [ELF] - Do not crash when discarding sections that are referenced by others.
rLLD295332: [ELF] - Do not crash when discarding sections that are referenced by others.
rL295332: [ELF] - Do not crash when discarding sections that are referenced by others.
Summary

SHF_LINK_ORDER sections adds special ordering requirements.
Such sections references other sections. Previously we would crash
if section that other were referenced to was discarded by script.

Patch fixes that by discarding all dependent sections in that case.
It supports chained dependencies, testcase is provided.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Feb 16 2017, 2:10 AM
tpimh added a subscriber: tpimh.Feb 16 2017, 3:01 AM
Closed by commit rL295332: [ELF] - Do not crash when discarding sections that are referenced by others. (authored by grimar). · Explain WhyFeb 16 2017, 8:17 AM
This revision was automatically updated to reflect the committed changes.
ruiu added inline comments.Feb 16 2017, 11:49 AM
lld/trunk/ELF/LinkerScript.cpp
282

You don't need to check if a container is empty before passing it to a for-each-ish function, just like you don't check for emptiness foo before doing for (auto bar : foo).

grimar added inline comments.Feb 17 2017, 12:42 AM
lld/trunk/ELF/LinkerScript.cpp
282

I know. And even applied that for simplicity in D29273.
But there I do not have cast. Since in this patch I had to do cast and anyways had to check !IS condition, I see nothing wrong in checking empty container to exit early. Code would not became simpler here, early continue still simplifies reading.

Revision Contents

PathSize
lld/
trunk/
ELF/
5 lines
test/
ELF/
linkerscript/
32 lines

Diff 88739

lld/trunk/ELF/LinkerScript.cpp

Show First 20 Lines • Show All 271 Lines • ▼ Show 20 Linesvoid LinkerScript<ELFT>::computeInputSections(InputSectionDescription *I) {
} }
} }
template <class ELFT> template <class ELFT>
void LinkerScript<ELFT>::discard(ArrayRef<InputSectionBase<ELFT> *> V) { void LinkerScript<ELFT>::discard(ArrayRef<InputSectionBase<ELFT> *> V) {
for (InputSectionBase<ELFT> *S : V) { for (InputSectionBase<ELFT> *S : V) {
S->Live = false; S->Live = false;
reportDiscarded(S); reportDiscarded(S);
InputSection<ELFT> *IS = dyn_cast<InputSection<ELFT>>(S);
if (!IS || IS->DependentSections.empty())
ruiuUnsubmitted
Not Done
ReplyInline Actions

You don't need to check if a container is empty before passing it to a for-each-ish function, just like you don't check for emptiness foo before doing for (auto bar : foo).

ruiu: You don't need to check if a container is empty before passing it to a for-each-ish function…
grimarAuthorUnsubmitted
Not Done
ReplyInline Actions

I know. And even applied that for simplicity in D29273.
But there I do not have cast. Since in this patch I had to do cast and anyways had to check !IS condition, I see nothing wrong in checking empty container to exit early. Code would not became simpler here, early continue still simplifies reading.

grimar: I know. And even applied that for simplicity in D29273. But there I do not have cast. Since in…
continue;
discard(IS->DependentSections);
} }
} }
template <class ELFT> template <class ELFT>
std::vector<InputSectionBase<ELFT> *> std::vector<InputSectionBase<ELFT> *>
LinkerScript<ELFT>::createInputSectionList(OutputSectionCommand &OutCmd) { LinkerScript<ELFT>::createInputSectionList(OutputSectionCommand &OutCmd) {
std::vector<InputSectionBase<ELFT> *> Ret; std::vector<InputSectionBase<ELFT> *> Ret;
▲ Show 20 Lines • Show All 1,847 Lines • Show Last 20 Lines

lld/trunk/test/ELF/linkerscript/discard-section-metadata.s

# REQUIRES: x86
# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
# RUN: echo "SECTIONS { /DISCARD/ : { *(.foo) } }" > %t.script
# RUN: ld.lld -o %t1 --script %t.script %t
# RUN: llvm-objdump -section-headers %t1 | FileCheck %s
# CHECK-NOT: .foo
# CHECK-NOT: .bar
# CHECK-NOT: .zed
# CHECK-NOT: .moo
## Sections dependency tree for testcase is:
## (.foo)
## | |
## | --(.bar)
## |
## --(.zed)
## |
## --(.moo)
##
.section .foo,"a"
.quad 0
.section .bar,"am",@progbits,.foo
.quad 0
.section .zed,"am",@progbits,.foo
.quad 0
.section .moo,"am",@progbits,.zed
.quad 0