[PATCH] D14382: [ELF2] - Basic implementation of -r/--relocatable

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/OutputSections.cpp:174
@@ -173,3 +173,3 @@
       if (NeedsGot)
-        P->setSymbolAndType(Body->getDynamicSymbolTableIndex(),
+        P->setSymbolAndType(Body->getSymbolTableIndex(),
                             LazyReloc ? Target->getPltReloc()
----------------
ruiu wrote:
> Why did you change the function name? If not relevant to this change, please revert.
There is no more such function in lld code, but I changed the member name also. For explanations why - please see comments below.

================
Comment at: ELF/OutputSections.cpp:646-649
@@ +645,6 @@
+void StaticRelocSection<ELFT>::addSection(InputSection<ELFT> *C) {
+  if (!LinkedOut)
+    LinkedOut = C->RelocatingSection->OutSec;
+  else if (LinkedOut != C->RelocatingSection->OutSec)
+    error("Sections to which the relocations applies doesn`t match");
+  OutputSection<ELFT>::addSection(C);
----------------
ruiu wrote:
> I'm not quite sure what you are doing here.
I need to have the section index for finalize() (The section header index of the section to which the relocation applies.). 
So I save output section here for that. Also I perform a check that such output section is the same. As it would be error for them to have different index. Last is just for safety, I am not sure how much is possible.

================
Comment at: ELF/OutputSections.cpp:654-655
@@ +653,4 @@
+template <class ELFT> void StaticRelocSection<ELFT>::finalize() {
+  if (Out<ELFT>::SymTab)
+    Header.sh_link = Out<ELFT>::SymTab->SectionIndex;
+  Header.sh_info = LinkedOut->SectionIndex;
----------------
ruiu wrote:
> Is it possible that we do not have the symbol table if -r is specified?
Since StripAll is forced disabled, then looks like not.

================
Comment at: ELF/OutputSections.cpp:942
@@ -917,2 +941,3 @@
                      });
-    return;
+    if (!Config->Relocatable)
+      return;
----------------
ruiu wrote:
> Why did you need this change?
I need to set correct symtab index for relacations in
void InputSectionBase<ELFT>::copyRelocatable(uint8_t *Buf, uint8_t *BufEnd, RelIteratorRange<isRela> Rels)
Alternative would be to iterate over bodies in symtab right in place there, but that would be much slower.
So I am using SymIndex (it is no more DynSymIndex) and keep correct index in it. For relocatable output in keeps index in symtab, otherwize - in dynsym, like was before.

================
Comment at: ELF/OutputSections.cpp:948
@@ -922,3 +947,3 @@
     Out<ELFT>::GnuHashTab->addSymbols(Symbols);
-  size_t I = 0;
+  size_t I = NumLocals;
   for (SymbolBody *B : Symbols)
----------------
ruiu wrote:
> What is this for?
Because of written above - I need to skip locals to assign correct indexes. DynSym does not have locals, so it is always 0 for it. But for SymTab I need to start iteration from its value.

================
Comment at: ELF/OutputSections.h:243
@@ +242,3 @@
+private:
+  OutputSectionBase<ELFT> *LinkedOut = nullptr;
+};
----------------
ruiu wrote:
> What is this variable for?
Updated name, added comment + please see comments for void StaticRelocSection<ELFT>::addSection(InputSection<ELFT> *C),
where it is used.


http://reviews.llvm.org/D14382