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

[Rafael Ávila de Espíndola via llvm-commits]

rafael added inline comments.

================
Comment at: ELF/OutputSections.cpp:237
@@ +236,3 @@
+    H = (H << 5) + H + C;
+  return H & 0xffffffff;
+}
----------------
Given that you are using a uint32_t, this is a nop, no?

================
Comment at: ELF/OutputSections.cpp:270
@@ +269,3 @@
+  // Bloom filter estimation: at least 8 bits for each hashed symbol.
+  return std::max<unsigned>((1u << Log2_32_Ceil(NumHashed)) / sizeof(Elf_Off),
+                            1);
----------------
The expression is always bigger than 1, no?
Also, given the comment, should it be

RoundUpToAlignment(NumHashed, sizeof(Elf_Off))/sizeof(Elf_Off)?


================
Comment at: ELF/OutputSections.cpp:292
@@ +291,3 @@
+    return;
+  Buf += sizeof(Elf_Word) * 4;
+  writeBloomFilter(Buf);
----------------
Maybe pass a reference to writeHeader and others so that you don't have to also increment the buf pointer in here?

================
Comment at: ELF/OutputSections.cpp:335
@@ +334,3 @@
+        Values[PrevValueIndex] |= 1;
+      Buckets[BucketIndex] = ValueIndex + 1;
+      PrevBucketIndex = BucketIndex;
----------------
This means that the entries in Bucket are inedxes into the Value vector.

According to https://blogs.oracle.com/ali/entry/gnu_hash_elf_sections, they are indexes into the dynamic symbol table, so in here you would have to add the number of non-hashed symbols.

Is that reference wrong?

================
Comment at: ELF/OutputSections.cpp:760
@@ +759,3 @@
+      NumGnuHashed ? GnuHashTableSection<ELFT>::calcNBuckets(NumGnuHashed) : 0;
+  std::stable_sort(
+      Symbols.begin(), Symbols.end(),
----------------
You are also sorting the non dynamic symbols, right?

I t is probably cleaner if there are two independent sorts, with just the constraints that each section has

if (!StrTabSec.isDynamic()) {
 simple sort on just getBinding() == STB_LOCAL.
 return;
}
The more complicated sort for the dynamic table.



================
Comment at: ELF/OutputSections.cpp:907
@@ +906,3 @@
+template <class ELFT>
+unsigned char SymbolTableSection<ELFT>::getSymbolBinding(SymbolBody *Body) {
+  unsigned char Visibility = Body->getMostConstrainingVisibility();
----------------
Adding getSymbolBinding is a nice implement on its own. Please commit and rebase.


http://reviews.llvm.org/D13815