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

[Rui Ueyama via llvm-commits]

ruiu added a comment.

First round of comments.


================
Comment at: ELF/Config.h:53-54
@@ -52,2 +52,4 @@
   bool ExportDynamic;
+  bool HashStyleGnu = false;
+  bool HashStyleSysV = true;
   bool NoInhibitExec;
----------------
Let's name them GnuHash and SysvHash.

================
Comment at: ELF/OutputSections.cpp:233
@@ +232,3 @@
+  uint32_t H = 5381;
+  for (unsigned char C : Name)
+    H = (H << 5) + H + C;
----------------
Remove unsigned.

================
Comment at: ELF/OutputSections.cpp:246-247
@@ +245,4 @@
+                              llvm::ELF::SHF_ALLOC) {
+  this->Header.sh_entsize =
+      sizeof(Elf_Word) == sizeof(Elf_Off) ? sizeof(Elf_Word) : 0;
+  this->Header.sh_addralign = sizeof(Elf_Off);
----------------
What is this condition for?

================
Comment at: ELF/OutputSections.cpp:255-276
@@ +254,24 @@
+
+template <class ELFT>
+const unsigned GnuHashTableSection<ELFT>::NBucketsEstimation[] = {
+    1,     //      0..     1
+    1,     //              2
+    3,     //      3..     4
+    3,     //      5..     8
+    7,     //      9..    16
+    13,    //     17..    32
+    31,    //     33..    64
+    61,    //     65..   128
+    127,   //    129..   256
+    251,   //    257..   512
+    509,   //    513..  1024
+    1021,  //   1025..  2048
+    2039,  //   2049..  4096
+    4093,  //   4097..  8192
+    8191,  //   8193.. 16384
+    16381, //  16385.. 32768
+    32749, //  32769.. 65536
+    65521, //  65537..131072
+    131071 // 131073..
+};
+
----------------
Where does this implementation come from? 

================
Comment at: ELF/OutputSections.cpp:285-286
@@ +284,4 @@
+
+  const unsigned Log2 =
+      HashedSymbols.empty() ? 0 : Log2_32_Ceil(HashedSymbols.size());
+
----------------
Can you return here if empty?

================
Comment at: ELF/OutputSections.cpp:786-804
@@ -632,17 +785,21 @@
 template <class ELFT>
-void SymbolTableSection<ELFT>::writeGlobalSymbols(uint8_t *&Buf) {
+void SymbolTableSection<ELFT>::writeGlobalSymbols(uint8_t *Buf) {
   // Write the internal symbol table contents to the output symbol table
   // pointed by Buf.
-  uint8_t *Start = Buf;
+  auto *Start = reinterpret_cast<Elf_Sym *>(Buf);
+  auto *Current = Start;
   for (const std::pair<StringRef, Symbol *> &P : Table.getSymbols()) {
     StringRef Name = P.first;
     Symbol *Sym = P.second;
     SymbolBody *Body = Sym->Body;
     if (!includeInSymtab<ELFT>(*Body))
       continue;
-    if (StrTabSec.isDynamic() && !includeInDynamicSymtab(*Body))
-      continue;
-
-    auto *ESym = reinterpret_cast<Elf_Sym *>(Buf);
-    Buf += sizeof(*ESym);
+    Elf_Sym *ESym;
+    if (StrTabSec.isDynamic()) {
+      if (!includeInDynamicSymtab(*Body))
+        continue;
+      ESym = Start + Body->getDynamicSymbolTableIndex() - 1;
+    } else {
+      ESym = Current++;
+    }
 
----------------
What is this change for?

================
Comment at: ELF/OutputSections.h:253
@@ +252,3 @@
+    HashData(SymbolBody *S);
+
+    SymbolBody *Body;
----------------
Remove this line.

================
Comment at: ELF/OutputSections.h:262
@@ +261,3 @@
+
+  static const unsigned NBucketsEstimation[];
+  static const unsigned NBucketsEstimationCount;
----------------
I'd name this Primes.

================
Comment at: ELF/OutputSections.h:263
@@ +262,3 @@
+  static const unsigned NBucketsEstimation[];
+  static const unsigned NBucketsEstimationCount;
+  std::vector<HashData> HashedSymbols;
----------------
Remove this. You can use sizeof(Primes) / sizeof(Prime[0]).

================
Comment at: ELF/OutputSections.h:317
@@ -281,3 +316,3 @@
 template <class ELFT> GotSection<ELFT> *Out<ELFT>::Got;
 template <class ELFT> HashTableSection<ELFT> *Out<ELFT>::HashTab;
 template <class ELFT> InterpSection<ELFT> *Out<ELFT>::Interp;
----------------
Rename this SysvHashTab.


http://reviews.llvm.org/D13815