[ELF] - Fixed handling relocations against zero sized .eh_frame section.

[grimar]

Summary

Relocations refering zero sized .eh_frame sections can happen when linking against crtbeginT.o.

Section Headers:

[Nr] Name              Type             Address           Offset
     Size              EntSize          Flags  Link  Info  Align

...

[ 4] .bss              NOBITS           0000000000000000  00000140
     0000000000000050  0000000000000000  WA       0     0     32
[ 5] .eh_frame         PROGBITS         0000000000000000  00000140
     0000000000000000  0000000000000000   A       0     0     4
[ 6] .jcr              PROGBITS         0000000000000000  00000140
     0000000000000000  0000000000000000  WA       0     0     8

...
0000000000000080 <__do_global_dtors_aux>:

80:	80 3d 00 00 00 00 00 	cmpb   $0x0,0x0(%rip)        # 87 <__do_global_dtors_aux+0x7>
87:	75 22                	jne    ab <__do_global_dtors_aux+0x2b>
89:	55                   	push   %rbp
8a:	48 89 e5             	mov    %rsp,%rbp
8d:	e8 6e ff ff ff       	callq  0 <deregister_tm_clones>
92:	b8 00 00 00 00       	mov    $0x0,%eax
97:	48 85 c0             	test   %rax,%rax
9a:	74 07                	je     a3 <__do_global_dtors_aux+0x23>
9c:	bf 00 00 00 00       	mov    $0x0,%edi
a1:	ff d0                	callq  *%rax
a3:	5d                   	pop    %rbp
a4:	c6 05 00 00 00 00 01 	movb   $0x1,0x0(%rip)        # ab <__do_global_dtors_aux+0x2b>
ab:	f3 c3                	repz retq 
ad:	0f 1f 00             	nopl   (%rax)

...
00000000009d 00050000000a R_X86_64_32 0000000000000000 .eh_frame + 0
...
0000000000c4 00050000000a R_X86_64_32 0000000000000000 .eh_frame + 0

The result of linking without this patch is assertion fail, details can be found in https://llvm.org/bugs/show_bug.cgi?id=25762&list_id=89776.
With this patch behavior seems to be consistent with gold.

[ruiu]

ELF/OutputSections.cpp

856–860 ↗

(On Diff #43136)

Is there any reason to not make getOffset a virtual function?

[grimar]

ELF/OutputSections.cpp

856–860 ↗

(On Diff #43136)

That would require some design change. For some reason approach that I used is seems to be as standart in new lld, for example there is such method:

template <class ELFT>
typename ELFFile<ELFT>::uintX_t
InputSectionBase<ELFT>::getOffset(uintX_t Offset) {
  switch (SectionKind) {
  case Regular:
    return cast<InputSection<ELFT>>(this)->OutSecOff + Offset;
  case EHFrame:
    return cast<EHInputSection<ELFT>>(this)->getOffset(Offset);
  case Merge:
    return cast<MergeInputSection<ELFT>>(this)->getOffset(Offset);
  case MipsReginfo:
    return cast<MipsReginfoInputSection<ELFT>>(this)->getOffset(Offset);
  }
  llvm_unreachable("Invalid section kind");
}

I can only guess why it was done in that way. But answering your question: to make it virtual all such places should be reimplemented first, there will be casting problem if I make it virtual only in SplitInputSection class (which is base for EHInputSection an MergeInputSection).
To use cast<SplitInputSection<ELFT>> I would need to add something to next enum for classof() implementation to recognize the type of SplitInputSection:

enum Kind { Regular, EHFrame, Merge, MipsReginfo };

That looks very inconsistent with what we already have and if requires change then for all at once I think.

[• rafael]

LGTM with nits.

ELF/InputSection.cpp

258 ↗

(On Diff #43136)

I would expand the comment a bit. What about

// The file crtbeginT.o has relocations pointing to the start of an empty
// .eh_frame that is known to be the first in the link. It does that to
// identify the start of the output .eh_frame. Handle this special case.

ELF/OutputSections.cpp

856 ↗

(On Diff #43136)

You can just remove the cast actually. I have done that in r256404. You should be able to just rebase.

test/ELF/ehframe-relocation.s

14 ↗

(On Diff #43136)

Just test that size is 0. That ways you can delete next lines and avoid passing -section-data .

26 ↗

(On Diff #43136)

Use {{.*}} to avoid checking the binary representation.

[grimar]

In D15610#316843, @rafael wrote:

LGTM with nits.

Thanks for review !

ELF/InputSection.cpp

258 ↗

(On Diff #43136)

Done.

ELF/OutputSections.cpp

856 ↗

(On Diff #43136)

Done.

test/ELF/ehframe-relocation.s

14 ↗

(On Diff #43136)

Done.

26 ↗

(On Diff #43136)

Done.

lld/trunk/ELF/InputSection.cpp

 template <class ELFT>
 bool EHInputSection<ELFT>::classof(const InputSectionBase<ELFT> *S) {
   return S->SectionKind == InputSectionBase<ELFT>::EHFrame;
 }
 
 template <class ELFT>
 typename EHInputSection<ELFT>::uintX_t
 EHInputSection<ELFT>::getOffset(uintX_t Offset) {
+  // The file crtbeginT.o has relocations pointing to the start of an empty
+  // .eh_frame that is known to be the first in the link. It does that to
+  // identify the start of the output .eh_frame. Handle this special case.
+  if (this->getSectionHdr()->sh_size == 0)
+    return Offset;
   std::pair<uintX_t, uintX_t> *I = this->getRangeAndSize(Offset).first;
   uintX_t Base = I->second;
   if (Base == uintX_t(-1))
     return -1; // Not in the output
 
   uintX_t Addend = Offset - I->first;
   return Base + Addend;
 }

lld/trunk/test/ELF/ehframe-relocation.s

+// REQUIRES: x86
+// RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+// RUN: ld.lld %t.o -o %t
+// RUN: llvm-readobj -s %t | FileCheck %s
+// RUN: llvm-objdump -d %t | FileCheck --check-prefix=DISASM %s
+
+// CHECK:      Name: .eh_frame
+// CHECK-NEXT: Type: SHT_X86_64_UNWIND
+// CHECK-NEXT: Flags [
+// CHECK-NEXT:   SHF_ALLOC
+// CHECK-NEXT: ]
+// CHECK-NEXT: Address: 0x10120
+// CHECK-NEXT: Offset:
+// CHECK-NEXT: Size: 0
+
+// 0x10120 = 65824
+// 0x10120 + 5 = 65829
+// DISASM:      Disassembly of section .text:
+// DISASM-NEXT: _start:
+// DISASM-NEXT:    11000: {{.*}} movq 65824, %rax
+// DISASM-NEXT:    11008: {{.*}} movq 65829, %rax
+
+.section .eh_frame,"ax",@unwind
+
+.section .text
+.globl _start
+_start:
+ movq .eh_frame, %rax
+ movq .eh_frame + 5, %rax