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

[MaskRay]

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

[MaskRay]

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

[MaskRay]

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

[ruiu]

ELF/SyntheticSections.cpp

1494

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]

ELF/SyntheticSections.cpp

1494

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]

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

[MaskRay]

ELF/SyntheticSections.cpp

1494

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]

Better comment

[MaskRay]

ELF/SyntheticSections.cpp

1494

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]

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]

Apply pcc's suggestion

[ruiu]

ELF/SyntheticSections.cpp

1496

Please move the assignment out of the if.

[ruiu]

Other than that LGTM

[MaskRay]

move the assignment out of the if.

[MaskRay]

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

ELF/SyntheticSections.cpp

 
 void RelocationBaseSection::addReloc(const DynamicReloc &Reloc) {
   if (Reloc.Type == Target->RelativeRel)
     ++NumRelativeRelocs;
   Relocs.push_back(Reloc);
 }
 
 void RelocationBaseSection::finalizeContents() {
-  // If all relocations are R_*_{,I}RELATIVE they don't refer to any dynamic
-  // symbol and we don't need a dynamic symbol table. If that is the case, use
-  // the index of the regular symbol table section (if exists) or 0.
-  if (In.DynSymTab)
-    getParent()->Link = In.DynSymTab->getParent()->SectionIndex;
-  else if (In.SymTab)
-    getParent()->Link = In.SymTab->getParent()->SectionIndex;
-  else
-    getParent()->Link = 0;
+  // When linking glibc statically, .rela?.plt contains R_*_IRELATIVE
+  // relocations due to IFUNC (e.g. strcpy). Just set sh_link to 0 as there is
+  // no .dynsym. We cannot set it to .symtab as otherwise GNU strip (as of
+  // 2018-10) will change the address of .rela.plt (and rename it to
+  // .rela.got.plt) and break __rela_iplt_{start,end} used in
+  // glibc/csu/libc-start.c:apply_irel. Note, GNU readelf will warn "Link field
+  // (0) should index a symtab section", but we can live with that.
+  getParent()->Link =
+      In.DynSymTab ? In.DynSymTab->getParent()->SectionIndex : 0;
 
   if (In.RelaIplt == this || In.RelaPlt == this)
     getParent()->Info = In.GotPlt->getParent()->SectionIndex;
 }
 
 RelrBaseSection::RelrBaseSection()
     : SyntheticSection(SHF_ALLOC,
                        Config->UseAndroidRelrTags ? SHT_ANDROID_RELR : SHT_RELR,

test/ELF/aarch64-gnu-ifunc.s

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

test/ELF/gnu-ifunc-i386.s

 // CHECK-NEXT:  Name: .rel.plt
 // CHECK-NEXT:  Type: SHT_REL
 // CHECK-NEXT:  Flags [
 // CHECK-NEXT:    SHF_ALLOC
 // CHECK-NEXT:  ]
 // CHECK-NEXT:  Address: [[RELA:.*]]
 // CHECK-NEXT:  Offset: 0xD4
 // CHECK-NEXT:  Size: 16
-// CHECK-NEXT:  Link: 6
+// CHECK-NEXT:  Link: 0
 // CHECK-NEXT:  Info: 4
 // CHECK-NEXT:  AddressAlignment: 4
 // CHECK-NEXT:  EntrySize: 8
 // CHECK-NEXT: }
 // CHECK:     Relocations [
 // CHECK-NEXT:   Section ({{.*}}) .rel.plt {
 // CHECK-NEXT:     0x402000 R_386_IRELATIVE
 // CHECK-NEXT:     0x402004 R_386_IRELATIVE

test/ELF/gnu-ifunc.s

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