This is an archive of the discontinued LLVM Phabricator instance.

Differential D47234

ELF: Allow ICF on .data.rel.ro sections.
ClosedPublic

Authored by pcc on May 22 2018, 3:54 PM.

Details

Reviewers
ruiu
• espindola
Commits
rG3a9e2ca05112: ELF: Allow ICF on .data.rel.ro sections.
rLLD333040: ELF: Allow ICF on .data.rel.ro sections.
rL333040: ELF: Allow ICF on .data.rel.ro sections.

Diff Detail

Event Timeline

pcc created this revision.May 22 2018, 3:54 PM
Herald added a reviewer: • espindola. · View Herald TranscriptMay 22 2018, 3:54 PM
Herald added subscribers: arichardson, emaste. · View Herald Transcript
Harbormaster completed remote builds in B18484: Diff 148120.May 22 2018, 3:55 PM
ruiu added inline comments.May 22 2018, 4:06 PM
lld/ELF/ICF.cpp
165–167

This expressionis perhaps a bit too long. I'd split it by defining bool IsWritable or something, with a comment explaning that .data.rel.ro is essentially read-only.

pcc updated this revision to Diff 148127.May 22 2018, 4:16 PM
pcc marked an inline comment as done.
  • Add comment
Harbormaster completed remote builds in B18487: Diff 148127.May 22 2018, 4:16 PM
ruiu accepted this revision.May 22 2018, 4:24 PM

LGTM

This revision is now accepted and ready to land.May 22 2018, 4:24 PM
Closed by commit rL333040: ELF: Allow ICF on .data.rel.ro sections. (authored by pcc). · Explain WhyMay 22 2018, 4:26 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lld/
ELF/
3 lines
test/
ELF/
12 lines

Diff 148120

lld/ELF/ICF.cpp

Show First 20 Lines • Show All 156 Lines • ▼ Show 20 Lines
// Returns a hash value for S. Note that the information about // Returns a hash value for S. Note that the information about
// relocation targets is not included in the hash value. // relocation targets is not included in the hash value.
template <class ELFT> static uint32_t getHash(InputSection *S) { template <class ELFT> static uint32_t getHash(InputSection *S) {
return hash_combine(S->Flags, S->getSize(), S->NumRelocations, S->Data); return hash_combine(S->Flags, S->getSize(), S->NumRelocations, S->Data);
} }
// Returns true if section S is subject of ICF. // Returns true if section S is subject of ICF.
static bool isEligible(InputSection *S) { static bool isEligible(InputSection *S) {
if (!S->Live || S->KeepUnique || !(S->Flags & SHF_ALLOC) || if (!S->Live || S->KeepUnique || !(S->Flags & SHF_ALLOC) ||
(S->Flags & SHF_WRITE)) ((S->Flags & SHF_WRITE) && S->Name != ".data.rel.ro" &&
!S->Name.startswith(".data.rel.ro.")))
ruiuUnsubmitted
Done
ReplyInline Actions

This expressionis perhaps a bit too long. I'd split it by defining bool IsWritable or something, with a comment explaning that .data.rel.ro is essentially read-only.

ruiu: This expressionis perhaps a bit too long. I'd split it by defining `bool IsWritable` or…
return false; return false;
// Don't merge read only data sections unless // Don't merge read only data sections unless
// --ignore-data-address-equality was passed. // --ignore-data-address-equality was passed.
if (!(S->Flags & SHF_EXECINSTR) && !Config->IgnoreDataAddressEquality) if (!(S->Flags & SHF_EXECINSTR) && !Config->IgnoreDataAddressEquality)
return false; return false;
// Don't merge synthetic sections as their Data member is not valid and empty. // Don't merge synthetic sections as their Data member is not valid and empty.
▲ Show 20 Lines • Show All 300 Lines • Show Last 20 Lines

lld/test/ELF/icf-relro.s

  • This file was added.
# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
# RUN: ld.lld %t -o %t2 --icf=all --ignore-data-address-equality --print-icf-sections | FileCheck %s
# CHECK: selected section {{.*}}:(.data.rel.ro)
# CHECK: removing identical section {{.*}}:(.data.rel.ro.foo)
.section .data.rel.ro,"aw"
.quad foo
.section .data.rel.ro.foo,"aw"
foo:
.quad foo