Page MenuHomePhabricator

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

Authored by grimar on Dec 17 2015, 7:38 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 43136.Dec 17 2015, 7:38 AM
grimar retitled this revision from to [ELF] - Fixed handling relocations against zero sized .eh_frame section..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: grimar, llvm-commits.
emaste added a subscriber: emaste.Dec 17 2015, 12:08 PM
ruiu added inline comments.Dec 18 2015, 3:50 PM
ELF/OutputSections.cpp
856–860 ↗(On Diff #43136)

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

grimar added inline comments.Dec 21 2015, 3:32 AM
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 accepted this revision.Dec 24 2015, 5:07 PM
rafael edited edge metadata.

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.

This revision is now accepted and ready to land.Dec 24 2015, 5:07 PM
This revision was automatically updated to reflect the committed changes.

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.