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

[George Rimar via llvm-commits]

grimar added a comment.

In http://reviews.llvm.org/D15610#316843, @rafael wrote:

> LGTM with nits.


Thanks for review !


================
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
----------------
rafael wrote:
> 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.
> 
Done.

================
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))
----------------
rafael wrote:
> 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.
Done.

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

================
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
----------------
rafael wrote:
> Use {{.*}} to avoid checking the binary representation.
Done.


Repository:
  rL LLVM

http://reviews.llvm.org/D15610