[ELF] - Avoid reusing DynsymIndex for -r

[grimar]

Summary

DynsymIndex was used as symbol table index for -r.
Also it was reused in the same way in D28612 for
--emit-relocs, though it introduced few restrictions.
Since DynsymIndex is busy with symtab index,
there is no way to create .dynsym table.

Patch shows one of possible way to avoid reusing this variable.

Another and much simpler way would be just to add
SymtabIndex variable to Symbol class.

I am not sure what is better.

[• rafael]

Keeping the Symbol type small is important for the common case, so I think this is better than adding another member variable.

If this is too slow -r, we can probably reorder the loop to gain some speed.

Rui, what do you think?

[ruiu]

ELF/InputSection.cpp

233

This function seems too heavy to run for each relocation. It is probably not ok to do this much here.

[ruiu]

So I took a look at the code, and I think the current implementation of the -r option in regard to symbol indices is unnecessarily complicated. The logic is scattered over Writer.cpp and SyntheticSections.cpp, and we are handling -r in an obscure way there. It needs fixing from a broader perspective (e.g. moving the logic to SynethticSections) instead of a local fix.

[grimar]

In D28773#652532, @ruiu wrote:

So I took a look at the code, and I think the current implementation of the -r option in regard to symbol indices is unnecessarily complicated. The logic is scattered over Writer.cpp and SyntheticSections.cpp, and we are handling -r in an obscure way there. It needs fixing from a broader perspective (e.g. moving the logic to SynethticSections) instead of a local fix.

Done in D29021.

ELF/InputSection.cpp

233

That affects only -relocatable.

But anyways, since this patch is abandoned and D29021 which landed instead also contains linear search,
I had in mind two possible optimizations for new SymbolTableSection::getSymbolIndex()

1. We can cache the result for Body in place of call (copyRelocations) and call getSymbolIndex only if result is not in cache.
2. We can add some new member for doing mapping from Body to index to SymbolTableSection class. To replace linear search with mapping call.

As an alternative we can do nothing for now and see if somebody mets perfomance issues with relocatable option.

What do you think ?

ELF/InputSection.cpp

 #include "Config.h"
 #include "EhFrame.h"
 #include "Error.h"
 #include "InputFiles.h"
 #include "LinkerScript.h"
 #include "Memory.h"
 #include "OutputSections.h"
 #include "Relocations.h"
+#include "SymbolTable.h"
 #include "SyntheticSections.h"
 #include "Target.h"
 #include "Thunks.h"
 #include "llvm/Object/Decompressor.h"
 #include "llvm/Support/Compression.h"
 #include "llvm/Support/Endian.h"
 #include <mutex>
 

 
 template <class ELFT> uint64_t InputSection<ELFT>::getThunksSize() const {
   uint64_t Total = 0;
   for (const Thunk<ELFT> *T : Thunks)
     Total += T->size();
   return Total;
 }
 
+template <class ELFT> static uint32_t getSymbolIndex(SymbolBody &Body) {
+  if (Body.IsLocal) {
+    size_t Ndx = 0;
+    auto *F = cast<elf::ObjectFile<ELFT>>(Body.File);
+    for (elf::ObjectFile<ELFT> *File : Symtab<ELFT>::X->getObjectFiles()) {
+      if (F == File) {
+        ArrayRef<SymbolBody *> Arr = File->getLocalSymbols();
+        return Ndx + llvm::find(Arr, &Body) - Arr.begin() + 1;
+      }
+      Ndx += File->getLocalSymbols().size();
+    }
+  }
+
+  assert(!Body.IsLocal);
+  ArrayRef<Symbol *> Symbols = Symtab<ELFT>::X->getSymbols();
+  return llvm::find(Symbols, Body.symbol()) - Symbols.begin() +
+         In<ELFT>::SymTab->NumLocals + 1;
+}
+
 // This is used for -r. We can't use memcpy to copy relocations because we need
 // to update symbol table offset and section index for each relocation. So we
 // copy relocations one by one.
 template <class ELFT>
 template <class RelTy>
 void InputSection<ELFT>::copyRelocations(uint8_t *Buf, ArrayRef<RelTy> Rels) {
   InputSectionBase<ELFT> *RelocatedSection = getRelocatedSection();
 
   for (const RelTy &Rel : Rels) {
     uint32_t Type = Rel.getType(Config->Mips64EL);
     SymbolBody &Body = this->File->getRelocTargetSym(Rel);
 
     Elf_Rela *P = reinterpret_cast<Elf_Rela *>(Buf);
     Buf += sizeof(RelTy);
 
     if (Config->Rela)
       P->r_addend = getAddend<ELFT>(Rel);
     P->r_offset = RelocatedSection->getOffset(Rel.r_offset);
-    P->setSymbolAndType(Body.DynsymIndex, Type, Config->Mips64EL);
+    P->setSymbolAndType(getSymbolIndex<ELFT>(Body), Type, Config->Mips64EL);
   }
 }
 
 static uint32_t getARMUndefinedRelativeWeakVA(uint32_t Type, uint32_t A,
                                               uint32_t P) {
   switch (Type) {
   case R_ARM_THM_JUMP11:
     return P + 2;

ELF/SyntheticSections.cpp

static bool sortMipsSymbols(const SymbolBody *L, const SymbolBody *R) {
   return L->GotIndex < R->GotIndex;
 }
 
 template <class ELFT> void SymbolTableSection<ELFT>::finalize() {
   this->OutSec->Link = this->Link = StrTabSec.OutSec->SectionIndex;
   this->OutSec->Info = this->Info = NumLocals + 1;
   this->OutSec->Entsize = this->Entsize;
 
-  if (Config->Relocatable) {
-    size_t I = NumLocals;
-    for (const SymbolTableEntry &S : Symbols)
-      S.Symbol->DynsymIndex = ++I;
+  if (Config->Relocatable)
     return;
-  }
 
   if (!StrTabSec.isDynamic()) {
     std::stable_sort(
         Symbols.begin(), Symbols.end(),
         [](const SymbolTableEntry &L, const SymbolTableEntry &R) {
           return L.Symbol->symbol()->computeBinding() == STB_LOCAL &&
                  R.Symbol->symbol()->computeBinding() != STB_LOCAL;
         });

ELF/Writer.cpp

for (SymbolBody *B : F->getLocalSymbols()) {
         continue;
       if (!includeInSymtab<ELFT>(*B))
         continue;
 
       InputSectionBase<ELFT> *Sec = DR->Section;
       if (!shouldKeepInSymtab<ELFT>(Sec, B->getName(), *B))
         continue;
       ++In<ELFT>::SymTab->NumLocals;
-      if (Config->Relocatable)
-        B->DynsymIndex = In<ELFT>::SymTab->NumLocals;
       F->KeptLocalSyms.push_back(std::make_pair(
           DR, In<ELFT>::SymTab->StrTabSec.addString(B->getName())));
     }
   }
 }
 
 // PPC64 has a number of special SHT_PROGBITS+SHF_ALLOC+SHF_WRITE sections that
 // we would like to make sure appear is a specific order to maximize their