[PATCH] D15235: [ELF] - Implemented R_*_IRELATIVE relocations for x86, x64 targets.

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/OutputSections.cpp:279-283
@@ -278,4 +278,7 @@
                             Config->Mips64EL);
     } else {
-      P->setSymbolAndType(0, Target->getRelativeReloc(), Config->Mips64EL);
+      unsigned RelReloc = (Body && isGnuIfunc<ELFT>(*Body))
+                              ? Target->getIRelativeReloc()
+                              : Target->getRelativeReloc();
+      P->setSymbolAndType(0, RelReloc, Config->Mips64EL);
     }
----------------
ruiu wrote:
>   } else if (Body && isGnuIfunc<ELFT>(*Body)) {
>     P->setSymbolAndType(0, Target->getIRelativeReloc(), Config->Mips64EL);
>   } else {
>     P->setSymbolAndType(0, Target->getRelativeReloc(), Config->Mips64EL);
>   }
Done.

================
Comment at: ELF/OutputSections.cpp:325-326
@@ -321,3 +324,4 @@
 template <class ELFT> void RelocationSection<ELFT>::finalize() {
-  this->Header.sh_link = Out<ELFT>::DynSymTab->SectionIndex;
+  this->Header.sh_link = Static ? Out<ELFT>::SymTab->SectionIndex
+                                : Out<ELFT>::DynSymTab->SectionIndex;
   this->Header.sh_size = Relocs.size() * this->Header.sh_entsize;
----------------
ruiu wrote:
> Is this change related to IRELATIVE relocations?
Its helps to generate correct output for static linking. Before this patch RelaPlt was not added to static output. Now it is, but there is no DynSymTab still. And SymTab is used instead. If I remove that then llvm-readobj will complain about binary.
Another possible way probably would be to use DynSymTab and add it to the output, but thats too heavy for that I think.

================
Comment at: ELF/Symbols.h:194
@@ +193,3 @@
+  // where R_[*]_IRELATIVE relocations do live.
+  static Elf_Sym RelaIpltStart, RelaIpltEnd;
+
----------------
ruiu wrote:
>   static Elf_Sym RelaIpltStart;
>   static Elf_Sym RelaIpltEnd;
> 
Done.

================
Comment at: ELF/Writer.cpp:701
@@ +700,3 @@
+  // If RelaPlt is empty then there is no reason to create this symbols.
+  if (!isOutputDynamic() && Out<ELFT>::RelaPlt &&
+      Out<ELFT>::RelaPlt->hasRelocs()) {
----------------
ruiu wrote:
> Can you factor this new code out as a new function?
Done and rewrote it fully to look better.

================
Comment at: ELF/Writer.cpp:771
@@ -743,1 +770,3 @@
 
+  // rel[a].plt can contain R_[*]_IRELATIVE relocations when static linking.
+  if (Out<ELFT>::RelaPlt && Out<ELFT>::RelaPlt->hasRelocs()) {
----------------
ruiu wrote:
> Is that only when static linking?
No, updated the comment. Point is that we always need rel[a].plt in output if it has entries.

================
Comment at: ELF/Writer.cpp:993
@@ +992,3 @@
+  if (Out<ELFT>::RelaPlt) {
+    uintX_t RVA = Out<ELFT>::RelaPlt->getVA();
+    DefinedAbsolute<ELFT>::RelaIpltStart.st_value = RVA;
----------------
ruiu wrote:
> Is this relative? If not, please remove "R" from "RVA".
R here was first letter of RelaPlt. I didnt want to name it VA because outside of this scope variable VA already exist. I know that VA inside scope would override the outer one, but its confusing to see.
I renamed it to Start.


http://reviews.llvm.org/D15235