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

Rafael Ávila de Espíndola via llvm-commits llvm-commits at lists.llvm.org
Thu Dec 24 17:07:13 PST 2015


rafael accepted this revision.
rafael added a comment.
This revision is now accepted and ready to land.

LGTM with nits.


================
Comment at: ELF/InputSection.cpp:258
@@ -257,1 +257,3 @@
 EHInputSection<ELFT>::getOffset(uintX_t Offset) {
+  // Relocations can be against .eh_frame section
+  // which has zero size. For example crtbeginT.o
----------------
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.


================
Comment at: ELF/OutputSections.cpp:856
@@ -855,3 +855,3 @@
   }
-  return VA + cast<MergeInputSection<ELFT>>(Section)->getOffset(Offset) +
-         Addend;
+  uintX_t SecOff;
+  if (isa<EHInputSection<ELFT>>(Section))
----------------
grimar wrote:
> ruiu wrote:
> > Is there any reason to not make getOffset a virtual function?
> 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.
You can just remove the cast actually. I have done that in r256404. You should be able to just rebase.

================
Comment at: test/ELF/ehframe-relocation.s:14
@@ +13,3 @@
+// CHECK-NEXT: Offset:
+// CHECK-NEXT: Size:
+// CHECK-NEXT: Link: 0
----------------
Just test that size is 0. That ways you can delete next lines and avoid passing -section-data .

================
Comment at: test/ELF/ehframe-relocation.s:26
@@ +25,3 @@
+// DISASM-NEXT: _start:
+// DISASM-NEXT:    11000: 48 8b 04 25 20 01 01 00 movq 65824, %rax
+// DISASM-NEXT:    11008: 48 8b 04 25 25 01 01 00 movq 65829, %rax
----------------
Use {{.*}} to avoid checking the binary representation.


http://reviews.llvm.org/D15610





More information about the llvm-commits mailing list