[PATCH] [lld][ELF][Writer] Add dynamic symbol table.

[Shankar Kalpathi Easwaran]

  I think it make sense to have a seperate dynamic symbol table class, future additions here may cause trouble if merged with symbol table.

  For example when a dynamic hash table section is created off the symbol table.


================
Comment at: lib/ReaderWriter/ELF/HeaderChunks.h:295
@@ -294,3 +294,3 @@
   shdr->sh_addr   = section->virtualAddr();
-  shdr->sh_size   = section->memSize();
+  shdr->sh_size   = std::max(section->fileSize(), section->memSize());
   shdr->sh_link   = section->link();
----------------
memsize should always be pointing to the right value, unless something was not updated.

================
Comment at: lib/ReaderWriter/ELF/HeaderChunks.h:314
@@ -310,4 +313,3 @@
   shdr->sh_addr   = section->virtualAddr();
-  shdr->sh_size   = section->fileSize();
-  shdr->sh_link = section->getLink();
+  shdr->sh_size   = std::max(section->fileSize(), section->memSize());
   shdr->sh_info = section->getInfo();
----------------
same here.

================
Comment at: lib/ReaderWriter/ELF/Writer.cpp:244-246
@@ -241,1 +243,5 @@
 
+  if (_targetInfo.isDynamic()) {
+    buildDynamicSymbolTable(file);
+  }
+
----------------
may be { } are not required here. not sure what the coding convention says here.

================
Comment at: lib/ReaderWriter/ELF/SectionChunks.h:692-707
@@ -650,18 +691,18 @@
 
 template <class ELFT> void SymbolTable<ELFT>::finalize() {
   // sh_info should be one greater than last symbol with STB_LOCAL binding
   // we sort the symbol table to keep all local symbols at the beginning
   std::stable_sort(_symbolTable.begin(), _symbolTable.end(),
-  [](const Elf_Sym *A, const Elf_Sym *B) {
-     return A->getBinding() < B->getBinding();
+  [](const SymbolEntry &A, const SymbolEntry &B) {
+     return A._symbol.getBinding() < B._symbol.getBinding();
   });
   uint16_t shInfo = 0;
-  for (auto i : _symbolTable) {
-    if (i->getBinding() != llvm::ELF::STB_LOCAL)
+  for (const auto &i : _symbolTable) {
+    if (i._symbol.getBinding() != llvm::ELF::STB_LOCAL)
       break;
     shInfo++;
   }
   this->_info = shInfo;
   this->_link = _stringSection->ordinal();
 }
 
----------------
Do we have a requirement of sorting the dynamic symbol table too.

================
Comment at: lib/ReaderWriter/ELF/SectionChunks.h:572-582
@@ -552,1 +571,13 @@
 
+  /// \brief Get the symbol table index for an Atom. If it's not in the symbol
+  /// table, return STN_UNDEF.
+  uint32_t getSymbolTableIndex(const Atom *a) const {
+    auto se = std::find_if(_symbolTable.begin(), _symbolTable.end(),
+                           [=](const SymbolEntry &se) {
+      return se._atom == a;
+    });
+    if (se == _symbolTable.end())
+      return STN_UNDEF;
+    return std::distance(_symbolTable.begin(), se);
+  }
+
----------------
Couldnt find a call to this function

================
Comment at: lib/ReaderWriter/ELF/SectionChunks.h:462
@@ -461,2 +461,3 @@
   if (const auto section = dyn_cast<Section<ELFT>>(c)) {
+    assert(!_link && "Section already has a link!");
     _link = section->getLink();
----------------
Did you mean, section doesnot have a link here ?

================
Comment at: lib/ReaderWriter/ELF/Writer.cpp:152-154
@@ -152,2 +151,5 @@
         _shdrtab->updateSection(section);
+      else
+        _shdrtab->updateSection(section, true);
+    }
 }
----------------
The link from dynsym to dynstr is already established when the sections are added to the layout, why is there a need to do this ?

Also if the section has a segment already, there is no need to call updateSection. updateSection is called only for sections that are not allocated.

Not sure if I am missing something.

================
Comment at: lib/ReaderWriter/ELF/SectionChunks.h:676-679
@@ -641,1 +675,6 @@
+    symbol.st_value = addr;
+  } else if (isa<const SharedLibraryAtom>(atom)) {
+    type = llvm::ELF::STT_FUNC;
+    symbol.st_shndx = llvm::ELF::SHN_UNDEF;
+    binding = llvm::ELF::STB_GLOBAL;
   } else {
----------------
The type cannot be always FUNC, it can be data, and binding can be WEAK etc ...


http://llvm-reviews.chandlerc.com/D442