[PATCH] D13815: [ELF2] Add support for Gnu Hash section

[Igor Kudrin via llvm-commits]

ikudrin added inline comments.

================
Comment at: ELF/OutputSections.cpp:194
@@ -193,3 +195,1 @@
 
-template <class ELFT> void HashTableSection<ELFT>::addSymbol(SymbolBody *S) {
-  StringRef Name = S->getName();
----------------
ikudrin wrote:
> rafael wrote:
> > This is being removed because the symbols have to be sorted, right? 
> > 
> > It is a bit untidy that we now call setDynamicSymbolTableIndex from two places.
> > 
> > Maybe organize this as: 
> > 
> > * In the current loop:
> >    Out<ELFT>::DynSymTab->addSymbol(...)
> > * Sort the DynSymTab if needed.
> > * Walk the DynSymTab, calling setDynamicSymbolTableIndex and adding the symbol to one or both hash tables.
> > 
> > 
> I consider the main reason to remove this function as it is not a hash table's responsibility to fill up a dynamic symbol table.
> 
> I like the idea to sort entries of a symbol table within the symbol table class, but in case of the Gnu hash section, it imposes not only some requirements to the order of entries, but also the oreder itself, as it is calculated using hash values and NBuckets parameter. I can't find a better solution than sorting the symbols which belongs to Gnu Hash table within the GnuHashTableSection class.
One more thing. If we implement one method to sort the entire dynamic symbol table and set indexes of the entries, we have to decide, when it should be called. The right place to call thist sort function might be the finalize() method of the symbol table class. But the sort function will require some information, which can only be provided by the Gnu hash table class, and the right place to calculate it is the finalize() method of this class. I don't think it's a good idea to rely on the order of calling of finalize() methods.


http://reviews.llvm.org/D13815