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

Rui Ueyama via llvm-commits llvm-commits at lists.llvm.org
Mon Oct 19 09:44:43 PDT 2015


ruiu added inline comments.

================
Comment at: ELF/OutputSections.cpp:232
@@ +231,3 @@
+  uint32_t H = 5381;
+  for (char C : Name)
+    H = (H << 5) + H + C;
----------------
My previous comment is probably wrong. You may want to use uint8_t here.

================
Comment at: ELF/OutputSections.cpp:245-246
@@ +244,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);
----------------
OK, thank you for the explanation. I'd prefer Is64Bits ? 0 : 4, as that's what we want.

================
Comment at: ELF/OutputSections.cpp:247
@@ +246,3 @@
+      sizeof(Elf_Word) == sizeof(Elf_Off) ? sizeof(Elf_Word) : 0;
+  this->Header.sh_addralign = sizeof(Elf_Off);
+}
----------------
This is always 2, no? Is this correct?

================
Comment at: ELF/OutputSections.cpp:256-274
@@ +255,21 @@
+const unsigned GnuHashTableSection<ELFT>::Primes[] = {
+    1,     //              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..
+};
----------------
Remove comments and write them in one line (or two), and add a comment saying that these numbers are the smallest numbers that are not smaller than 2^n.

================
Comment at: ELF/OutputSections.cpp:300
@@ +299,3 @@
+  // less than hashed symbol count.
+  NBuckets = Primes[std::min(Log2, sizeof(Primes) / sizeof(Primes[0]) - 1)];
+
----------------
Use llvm::array_lengthof().

================
Comment at: ELF/OutputSections.cpp:306
@@ +305,3 @@
+      + sizeof(Elf_Word) * NBuckets              // Hash Buckets
+      + sizeof(Elf_Word) * HashedSymbols.size(); // Hash Values;
+
----------------
Remove ';'

================
Comment at: ELF/OutputSections.cpp:309
@@ +308,3 @@
+  // Hashed symbols have to be sorted by their (Hash % NBuckets) values.
+  std::stable_sort(begin(HashedSymbols), end(HashedSymbols),
+                   [this](const HashData &L, const HashData &R) {
----------------
  HashedSymbols.begin(), HashedSymbols.end(),

================
Comment at: ELF/OutputSections.cpp:311-313
@@ +310,5 @@
+                   [this](const HashData &L, const HashData &R) {
+                     unsigned LBucket = L.Hash % NBuckets;
+                     unsigned RBucket = R.Hash % NBuckets;
+                     return LBucket < RBucket;
+                   });
----------------
  return L.Hash % NBuckets < R.Hash % NBuckets;

================
Comment at: ELF/OutputSections.cpp:341
@@ +340,3 @@
+void GnuHashTableSection<ELFT>::writeBloomFilter(uint8_t *Buf) {
+  static const unsigned C = sizeof(Elf_Off) * CHAR_BIT;
+  const unsigned MaskWordsBitmask = MaskWords - 1;
----------------
CHAR_BIT is always 8, so write 8. Drop 'static const' -- it is static const but the compiler can easily figure that out.

================
Comment at: ELF/OutputSections.cpp:346
@@ +345,3 @@
+  for (const HashData &Item : HashedSymbols) {
+    Elf_Off &Mask = Masks[(Item.Hash / C) & MaskWordsBitmask];
+    Mask |= static_cast<uintX_t>(1u) << (Item.Hash % C);
----------------
You can directly write (MaskWords - 1) here.

================
Comment at: ELF/OutputSections.cpp:347-348
@@ +346,4 @@
+    Elf_Off &Mask = Masks[(Item.Hash / C) & MaskWordsBitmask];
+    Mask |= static_cast<uintX_t>(1u) << (Item.Hash % C);
+    Mask |= static_cast<uintX_t>(1u) << ((Item.Hash >> Shift2) % C);
+  }
----------------
You can just write 1 because RHS's type of << is the same as the result's type.

================
Comment at: ELF/OutputSections.cpp:356-357
@@ +355,4 @@
+
+  auto *const Buckets = reinterpret_cast<Elf_Word *>(Buf);
+  auto *const Values = Buckets + NBuckets;
+
----------------
Drop const.

================
Comment at: ELF/OutputSections.cpp:357
@@ +356,3 @@
+  auto *const Buckets = reinterpret_cast<Elf_Word *>(Buf);
+  auto *const Values = Buckets + NBuckets;
+
----------------
auto -> unsigned

================
Comment at: ELF/OutputSections.cpp:359
@@ +358,3 @@
+
+  signed PrevBucketIndex = -1;
+  signed PrevValueIndex = -1;
----------------
ruiu wrote:
> signed -> int
Please add a reference to the data structure this code is trying to create.

================
Comment at: ELF/OutputSections.cpp:359-361
@@ +358,5 @@
+
+  signed PrevBucketIndex = -1;
+  signed PrevValueIndex = -1;
+  signed ValueIndex = 0;
+  for (const HashData &Item : HashedSymbols) {
----------------
signed -> int

================
Comment at: ELF/OutputSections.cpp:363
@@ +362,3 @@
+  for (const HashData &Item : HashedSymbols) {
+    signed BucketIndex = Item.Hash % NBuckets;
+    assert(PrevBucketIndex <= BucketIndex);
----------------
signed -> int

================
Comment at: ELF/OutputSections.cpp:367
@@ +366,3 @@
+      if (PrevValueIndex >= 0)
+        Values[PrevValueIndex] |= 1u;
+      Buckets[BucketIndex] = ValueIndex + 1;
----------------
1u -> 1

================
Comment at: ELF/OutputSections.cpp:371
@@ +370,3 @@
+    }
+    Values[ValueIndex] = Item.Hash & HashMask;
+    PrevValueIndex = ValueIndex++;
----------------
HashMask -> ~1

================
Comment at: ELF/OutputSections.h:262
@@ +261,3 @@
+  static const unsigned Primes[];
+  std::vector<HashData> HashedSymbols;
+  unsigned MaskWords;
----------------
Rename this Symbols because all symbols handled by this class are subject of calculation of hash values (so it is redundant).

================
Comment at: ELF/OutputSections.h:299
@@ -266,3 +298,3 @@
   static GotSection<ELFT> *Got;
-  static HashTableSection<ELFT> *HashTab;
+  static HashTableSection<ELFT> *SysvHashTab;
   static InterpSection<ELFT> *Interp;
----------------
rafael wrote:
> Most of these members are named after the section name in the output file, so I would keep the old name.
Hm, makes sense.

================
Comment at: ELF/Writer.cpp:486
@@ +485,3 @@
+      } else {
+        Body->setDynamicSymbolTableIndex(++DynamicSymbolTableIndex);
+      }
----------------
Why do we have to call setDynamicSymbolTableIndex here? That's not obvious.


http://reviews.llvm.org/D13815





More information about the llvm-commits mailing list