This is an archive of the discontinued LLVM Phabricator instance.

Differential D53993

[ELF] Change sh_link of .rel{,a}.plt to make GNU strip happy
ClosedPublic

Authored by MaskRay on Nov 1 2018, 12:44 PM.

Details

Reviewers
ruiu
grimar
emaste
• espindola
Commits
rG27d036e99523: [ELF] Change sh_link of .rel{,a}.plt to make GNU strip happy
rLLD345899: [ELF] Change sh_link of .rel{,a}.plt to make GNU strip happy
rL345899: [ELF] Change sh_link of .rel{,a}.plt to make GNU strip happy
Summary

D52830 sets sh_link to .symtab in static link, which breaks executable stripped by GNU strip.
It may also be odd that .rela.plt (SHF_ALLOC) points to .symtab (non-SHF_ALLOC).

Change the logic as pcc's suggestion.

Before:

% clang -fuse-ld=lld -static -xc =(printf 'int main(){}') # or gcc
% strip a.out; ./a.out
unexpected reloc type in static binary[1] 61634 segmentation fault ./a.out

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Nov 1 2018, 12:44 PM
Herald added a reviewer: • espindola. · View Herald TranscriptNov 1 2018, 12:44 PM
Herald added subscribers: llvm-commits, arichardson. · View Herald Transcript
Harbormaster completed remote builds in B24472: Diff 172205.Nov 1 2018, 12:45 PM
MaskRay updated this revision to Diff 172209.Nov 1 2018, 12:55 PM

Add tests and GNU strip date (git HEAD as of 2018-10)

Herald added a subscriber: javed.absar. · View Herald TranscriptNov 1 2018, 12:55 PM
Harbormaster completed remote builds in B24474: Diff 172209.Nov 1 2018, 12:55 PM
MaskRay updated this revision to Diff 172212.Nov 1 2018, 12:57 PM
MaskRay edited the summary of this revision. (Show Details)
MaskRay removed a subscriber: javed.absar.

A simple reproduce command. I forgot to add --verbatim in my arc diff command :)

Harbormaster completed remote builds in B24475: Diff 172212.Nov 1 2018, 12:57 PM
ruiu added inline comments.Nov 1 2018, 1:09 PM
ELF/SyntheticSections.cpp
1494 ↗(On Diff #172212)

Looks like this new comment is not easy to understand for those who do not already know the problem you are trying to fix in this. What is this link supposed to have by the spec? You shouldn't focus too much on a workaround when describing something, but focus on what it is supposed to be (and briefly mention the deviation from the expected situation.)

MaskRay added inline comments.Nov 1 2018, 1:29 PM
ELF/SyntheticSections.cpp
1494 ↗(On Diff #172212)

D52830 says:

ELF spec (https://docs.oracle.com/cd/E19683-01/816-1386/chapter6-94076/index.html)
says that for SHT_REL and SHT_RELA, sh_link references the associated symbol table
and sh_info the "section to which the relocation applies."

I think either .symtab and .dynsym can be used. binutils/readelf.c emits the warning when the pointed symbol table section is of neither SHT_SYMTAB nor SHT_DYNSYM.

case SHT_REL:
case SHT_RELA:

	  if (section->sh_link < 1
	      || section->sh_link >= filedata->file_header.e_shnum
	      || (filedata->section_headers[section->sh_link].sh_type != SHT_SYMTAB
		  && filedata->section_headers[section->sh_link].sh_type != SHT_DYNSYM))
	    warn (_("[%2u]: Link field (%u) should index a symtab section.\n"),
		  i, section->sh_link);
	  break;

Let me report an issue on binutils and simplify the comment with a link to that bug

MaskRay updated this revision to Diff 172227.Nov 1 2018, 1:49 PM

Simplify the comment with a link to https://sourceware.org/bugzilla/show_bug.cgi?id=23850

Harbormaster completed remote builds in B24479: Diff 172227.Nov 1 2018, 1:49 PM
MaskRay marked an inline comment as done.Nov 1 2018, 1:52 PM
MaskRay added inline comments.
ELF/SyntheticSections.cpp
1494 ↗(On Diff #172212)

Simplified it with a link to https://docs.oracle.com/cd/E19683-01/816-1386/6m7qcoblh/index.html

Basically it reverts the sh_link part of D52830

I am happy to change the comments at your request.

MaskRay updated this revision to Diff 172229.Nov 1 2018, 2:02 PM
MaskRay marked an inline comment as done.

Better comment

Harbormaster completed remote builds in B24480: Diff 172229.Nov 1 2018, 2:04 PM
MaskRay added inline comments.Nov 1 2018, 2:09 PM
ELF/SyntheticSections.cpp
1494 ↗(On Diff #172212)

Or how about this:

if (In.DynSymTab)
  getParent()->Link = In.DynSymTab->getParent()->SectionIndex;
else if (In.SymTab && In.RelaIplt != this) // && In.RelaIplt != this  is the new condition
  getParent()->Link = In.SymTab->getParent()->SectionIndex;
else
  getParent()->Link = 0;

.rela.plt (In.RelaIplt) is a special relocation section invented for IFUNC. We can keep the work around to a minimum case.

pcc added a subscriber: pcc.Nov 1 2018, 2:25 PM

D52830 sets sh_link to .symtab in static link, which makes more sense than 0,

I don't know about that. Irrespective of the bug it seems a little odd for a SHF_ALLOC section to be linked to a non-SHF_ALLOC section. Should this be instead:

if (InputSection *SymTab = Config->Relocatable ? In.SymTab : In.DynSymTab)
  getParent()->Link = SymTab->getParent()->SectionIndex;
else
  getParent()->Link = 0;

?

MaskRay updated this revision to Diff 172243.Nov 1 2018, 2:48 PM
MaskRay retitled this revision from [ELF] Set sh_link to 0 in static link to make GNU strip happy to [ELF] Change sh_link of .rel{,a}.plt to make GNU strip happy.
MaskRay edited the summary of this revision. (Show Details)

Apply pcc's suggestion

Harbormaster completed remote builds in B24482: Diff 172243.Nov 1 2018, 2:49 PM
ruiu added inline comments.Nov 1 2018, 3:22 PM
ELF/SyntheticSections.cpp
1497 ↗(On Diff #172243)

Please move the assignment out of the if.

ruiu accepted this revision.Nov 1 2018, 3:25 PM

Other than that LGTM

This revision is now accepted and ready to land.Nov 1 2018, 3:25 PM
MaskRay updated this revision to Diff 172251.Nov 1 2018, 3:28 PM
MaskRay edited the summary of this revision. (Show Details)

move the assignment out of the if.

MaskRay marked 3 inline comments as done.Nov 1 2018, 3:29 PM
Closed by commit rL345899: [ELF] Change sh_link of .rel{,a}.plt to make GNU strip happy (authored by MaskRay). · Explain WhyNov 1 2018, 3:31 PM
This revision was automatically updated to reflect the committed changes.
MaskRay added a comment.Nov 2 2018, 12:54 AM

Heads-up https://sourceware.org/bugzilla/show_bug.cgi?id=23850 Alan Modra has fixed the GNU strip issue (when .rela.pla has sh_link pointing to .symtab, the stripped executable will crash) and the readelf -S warning.

The patches should be included in GNU binutils 2.32

Revision Contents

PathSize
lld/
trunk/
ELF/
14 lines
test/
ELF/
2 lines
2 lines
5 lines

Diff 172252

lld/trunk/ELF/SyntheticSections.cpp

Show First 20 Lines • Show All 1,485 Lines • ▼ Show 20 Lines
void RelocationBaseSection::addReloc(const DynamicReloc &Reloc) { void RelocationBaseSection::addReloc(const DynamicReloc &Reloc) {
if (Reloc.Type == Target->RelativeRel) if (Reloc.Type == Target->RelativeRel)
++NumRelativeRelocs; ++NumRelativeRelocs;
Relocs.push_back(Reloc); Relocs.push_back(Reloc);
} }
void RelocationBaseSection::finalizeContents() { void RelocationBaseSection::finalizeContents() {
// If all relocations are R_*_{,I}RELATIVE they don't refer to any dynamic // When linking glibc statically, .rel{,a}.plt contains R_*_IRELATIVE
// symbol and we don't need a dynamic symbol table. If that is the case, use // relocations due to IFUNC (e.g. strcpy). sh_link will be set to 0 in that
// the index of the regular symbol table section (if exists) or 0. // case.
if (In.DynSymTab) InputSection *SymTab = Config->Relocatable ? In.SymTab : In.DynSymTab;
getParent()->Link = In.DynSymTab->getParent()->SectionIndex; getParent()->Link = SymTab ? SymTab->getParent()->SectionIndex : 0;
else if (In.SymTab)
getParent()->Link = In.SymTab->getParent()->SectionIndex;
else
getParent()->Link = 0;
if (In.RelaIplt == this || In.RelaPlt == this) if (In.RelaIplt == this || In.RelaPlt == this)
getParent()->Info = In.GotPlt->getParent()->SectionIndex; getParent()->Info = In.GotPlt->getParent()->SectionIndex;
} }
RelrBaseSection::RelrBaseSection() RelrBaseSection::RelrBaseSection()
: SyntheticSection(SHF_ALLOC, : SyntheticSection(SHF_ALLOC,
Config->UseAndroidRelrTags ? SHT_ANDROID_RELR : SHT_RELR, Config->UseAndroidRelrTags ? SHT_ANDROID_RELR : SHT_RELR,
▲ Show 20 Lines • Show All 1,616 Lines • Show Last 20 Lines

lld/trunk/test/ELF/aarch64-gnu-ifunc.s

Show All 9 Lines
// CHECK-NEXT: Name: .rela.plt // CHECK-NEXT: Name: .rela.plt
// CHECK-NEXT: Type: SHT_RELA // CHECK-NEXT: Type: SHT_RELA
// CHECK-NEXT: Flags [ // CHECK-NEXT: Flags [
// CHECK-NEXT: SHF_ALLOC // CHECK-NEXT: SHF_ALLOC
// CHECK-NEXT: ] // CHECK-NEXT: ]
// CHECK-NEXT: Address: [[RELA:.*]] // CHECK-NEXT: Address: [[RELA:.*]]
// CHECK-NEXT: Offset: 0x158 // CHECK-NEXT: Offset: 0x158
// CHECK-NEXT: Size: 48 // CHECK-NEXT: Size: 48
// CHECK-NEXT: Link: 6 // CHECK-NEXT: Link: 0
// CHECK-NEXT: Info: 4 // CHECK-NEXT: Info: 4
// CHECK-NEXT: AddressAlignment: 8 // CHECK-NEXT: AddressAlignment: 8
// CHECK-NEXT: EntrySize: 24 // CHECK-NEXT: EntrySize: 24
// CHECK-NEXT: } // CHECK-NEXT: }
// CHECK: Relocations [ // CHECK: Relocations [
// CHECK-NEXT: Section ({{.*}}) .rela.plt { // CHECK-NEXT: Section ({{.*}}) .rela.plt {
// CHECK-NEXT: 0x220000 R_AARCH64_IRELATIVE // CHECK-NEXT: 0x220000 R_AARCH64_IRELATIVE
// CHECK-NEXT: 0x220008 R_AARCH64_IRELATIVE // CHECK-NEXT: 0x220008 R_AARCH64_IRELATIVE
▲ Show 20 Lines • Show All 113 Lines • Show Last 20 Lines

lld/trunk/test/ELF/gnu-ifunc-i386.s

Show All 9 Lines
// CHECK-NEXT: Name: .rel.plt // CHECK-NEXT: Name: .rel.plt
// CHECK-NEXT: Type: SHT_REL // CHECK-NEXT: Type: SHT_REL
// CHECK-NEXT: Flags [ // CHECK-NEXT: Flags [
// CHECK-NEXT: SHF_ALLOC // CHECK-NEXT: SHF_ALLOC
// CHECK-NEXT: ] // CHECK-NEXT: ]
// CHECK-NEXT: Address: [[RELA:.*]] // CHECK-NEXT: Address: [[RELA:.*]]
// CHECK-NEXT: Offset: 0xD4 // CHECK-NEXT: Offset: 0xD4
// CHECK-NEXT: Size: 16 // CHECK-NEXT: Size: 16
// CHECK-NEXT: Link: 6 // CHECK-NEXT: Link: 0
// CHECK-NEXT: Info: 4 // CHECK-NEXT: Info: 4
// CHECK-NEXT: AddressAlignment: 4 // CHECK-NEXT: AddressAlignment: 4
// CHECK-NEXT: EntrySize: 8 // CHECK-NEXT: EntrySize: 8
// CHECK-NEXT: } // CHECK-NEXT: }
// CHECK: Relocations [ // CHECK: Relocations [
// CHECK-NEXT: Section ({{.*}}) .rel.plt { // CHECK-NEXT: Section ({{.*}}) .rel.plt {
// CHECK-NEXT: 0x402000 R_386_IRELATIVE // CHECK-NEXT: 0x402000 R_386_IRELATIVE
// CHECK-NEXT: 0x402004 R_386_IRELATIVE // CHECK-NEXT: 0x402004 R_386_IRELATIVE
▲ Show 20 Lines • Show All 100 Lines • Show Last 20 Lines

lld/trunk/test/ELF/gnu-ifunc.s

Show All 9 Lines
// CHECK-NEXT: Name: .rela.plt // CHECK-NEXT: Name: .rela.plt
// CHECK-NEXT: Type: SHT_RELA // CHECK-NEXT: Type: SHT_RELA
// CHECK-NEXT: Flags [ // CHECK-NEXT: Flags [
// CHECK-NEXT: SHF_ALLOC // CHECK-NEXT: SHF_ALLOC
// CHECK-NEXT: ] // CHECK-NEXT: ]
// CHECK-NEXT: Address: [[RELA:.*]] // CHECK-NEXT: Address: [[RELA:.*]]
// CHECK-NEXT: Offset: 0x158 // CHECK-NEXT: Offset: 0x158
// CHECK-NEXT: Size: 48 // CHECK-NEXT: Size: 48
// CHECK-NEXT: Link: [[SYMTAB:.*]] // CHECK-NEXT: Link: 0
// CHECK-NEXT: Info: [[GOTPLT:.*]] // CHECK-NEXT: Info: [[GOTPLT:.*]]
// CHECK-NEXT: AddressAlignment: 8 // CHECK-NEXT: AddressAlignment: 8
// CHECK-NEXT: EntrySize: 24 // CHECK-NEXT: EntrySize: 24
// CHECK-NEXT: } // CHECK-NEXT: }
// CHECK: Index: [[GOTPLT]] // CHECK: Index: [[GOTPLT]]
// CHECK-NEXT: Name: .got.plt // CHECK-NEXT: Name: .got.plt
// CHECK: Index: [[SYMTAB]]
// CHECK-NEXT: Name: .symtab
// CHECK-NEXT: Type: SHT_SYMTAB
// CHECK: Relocations [ // CHECK: Relocations [
// CHECK-NEXT: Section ({{.*}}) .rela.plt { // CHECK-NEXT: Section ({{.*}}) .rela.plt {
// CHECK-NEXT: 0x202000 R_X86_64_IRELATIVE // CHECK-NEXT: 0x202000 R_X86_64_IRELATIVE
// CHECK-NEXT: 0x202008 R_X86_64_IRELATIVE // CHECK-NEXT: 0x202008 R_X86_64_IRELATIVE
// CHECK-NEXT: } // CHECK-NEXT: }
// CHECK-NEXT: ] // CHECK-NEXT: ]
// CHECK: Symbols [ // CHECK: Symbols [
// CHECK-NEXT: Symbol { // CHECK-NEXT: Symbol {
▲ Show 20 Lines • Show All 97 Lines • Show Last 20 Lines