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

[Michael Spencer]

  The reason for the changes to section size and `updateSection` is due to the order in which everything runs. The dynamic symbol table needs to have its link member set to the index of the dynamic string table, which we don't know until after `buildSectionHeaderTable` is done. However, `buildSectionHeaderTable` is what sets the link member, thus we have to run `updateSection` to fix the link member. I think I can actually get rid of the size change though, it was needed before I added linkOnly to `updateSection`.

  I don't really see a need to make the dynamic symbol table a different type.

  I'll add comments.


================
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();
 }
 
----------------
Shankar Kalpathi Easwaran wrote:
> Do we have a requirement of sorting the dynamic symbol table too.
Yes.

================
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();
----------------
Shankar Kalpathi Easwaran wrote:
> Did you mean, section doesnot have a link here ?
If a merged section already has a non-zero link, setting the link again is most likely wrong.

================
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);
+  }
+
----------------
Shankar Kalpathi Easwaran wrote:
> Couldnt find a call to this function
This is used by a later patch. I'll move it to that patch.

================
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 {
----------------
Shankar Kalpathi Easwaran wrote:
> The type cannot be always FUNC, it can be data, and binding can be WEAK etc ...
Yep, but that information isn't currently in SharedLibraryAtom. This will be fixed in a separate patch.

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


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