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

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/OutputSections.cpp:367-368
@@ +366,4 @@
+    size_t Pos = (Item.GnuHash / C) & (MaskWords - 1);
+    uintX_t V = uintX_t(1) << (Item.GnuHash % C) |
+                uintX_t(1) << ((Item.GnuHash >> Shift2) % C);
+    Masks[Pos] |= V;
----------------
It's better to parenthesize `uintX_t(1) << (Item.GnuHash % C)` and `uintX_t(1) << ((Item.GnuHash >> Shift2) % C)`. Not everybody remembers the operator precedence table precisely.

================
Comment at: ELF/OutputSections.cpp:379-396
@@ +378,20 @@
+
+  int PrevBucketIndex = -1;
+  int PrevValueIndex = -1;
+  int ValueIndex = 0;
+  for (const typename SymbolTableSection<ELFT>::SymbolData &Item :
+       Out<ELFT>::DynSymTab->getGnuHashSymbols()) {
+    int BucketIndex = Item.GnuHash % NBuckets;
+    assert(PrevBucketIndex <= BucketIndex);
+    if (BucketIndex != PrevBucketIndex) {
+      if (PrevValueIndex >= 0)
+        Values[PrevValueIndex] |= 1;
+      Buckets[BucketIndex] = Item.Body->getDynamicSymbolTableIndex();
+      PrevBucketIndex = BucketIndex;
+    }
+    Values[ValueIndex] = Item.GnuHash & ~1;
+    PrevValueIndex = ValueIndex++;
+  }
+  if (PrevValueIndex >= 0)
+    Values[PrevValueIndex] |= 1;
+}
----------------
This part of code is a bit hard to read because too many local variables are used and they look similar. Let's use less variables and make their name shorter.

  int PrevHash = -1;
  int I = 0;
  for (const typename SymbolTableSection<ELFT>::SymbolData &Item :
       Out<ELFT>::DynSymTab->getGnuHashSymbols()) {
    int Hash = Item.GnuHash % NBuckets;
    if (Hash != PrevHash) {
      if (I > 0)
        Values[I - 1] |= 1;
      Buckets[Hash] = Item.Body->getDynamicSymbolTableIndex();
      PrevHash = Hash;
    }
    Values[I] = Item.GnuHash & ~1;
    ++I;
  }
  if (I > 0)
    Values[I - 1] |= 1;


================
Comment at: ELF/OutputSections.h:185
@@ +184,3 @@
+    SymbolBody *Body;
+    bool GnuHashed;
+    uint32_t GnuHash;
----------------
I'd name this HasGnuHash.


http://reviews.llvm.org/D13815