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

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/Driver.cpp:159
@@ +158,3 @@
+  if (auto *Arg = Args.getLastArg(OPT_hash_style)) {
+    StringRef Style = Arg->getValue();
+    if (Style == "gnu") {
----------------
I'd name this just "S".

================
Comment at: ELF/OutputSections.cpp:237
@@ +236,3 @@
+    H = (H << 5) + H + C;
+  return H & 0xffffffff;
+}
----------------
ikudrin wrote:
> rafael wrote:
> > Given that you are using a uint32_t, this is a nop, no?
> Thanks for reminding. The reference implementation uses uint_fast32_t, I think we should do the same.
Unlike uint_fast32_t, uint32_t is always 32-bit. You don't need this mask.

================
Comment at: ELF/OutputSections.cpp:246
@@ +245,3 @@
+  this->Header.sh_addralign = ELFT::Is64Bits ? 8 : 4;
+}
+
----------------
Yeah, this looks better than before.

================
Comment at: ELF/OutputSections.cpp:256
@@ +255,3 @@
+  // which is not greater than NumHashed.
+  // No deep investigation was done, it just looks good for the first try.
+  static const unsigned Primes[] = {
----------------
Let's remove this line. This implementation should be good enough (not only as a first try).

================
Comment at: ELF/OutputSections.cpp:258-259
@@ +257,4 @@
+  static const unsigned Primes[] = {
+      1,   1,    3,    3,    7,    13,    31,    61,    127,   251,
+      509, 1021, 2039, 4093, 8191, 16381, 32749, 65521, 131071};
+
----------------
No need to align them vertically.

================
Comment at: ELF/OutputSections.cpp:315-317
@@ +314,5 @@
+       Out<ELFT>::DynSymTab->getGnuHashSymbols())
+    Masks[(Item.GnuHash / C) & (MaskWords - 1)] |=
+        (static_cast<uintX_t>(1) << (Item.GnuHash % C)) |
+        (static_cast<uintX_t>(1) << ((Item.GnuHash >> Shift2) % C));
+}
----------------
It's probably better to split this into multiple statements.

  size_t Pos = (Item.GnuHash / C) & (MaskWords - 1);
  Elf_Off V = uintX_t(1) << (Item.GnuHash % C) | uintX_t(1) << ((Item.GnuHash >> Shift2) % C);
  Mask[Pos] |= V;


================
Comment at: ELF/OutputSections.h:62
@@ -60,3 +61,3 @@
 
 template <class ELFT>
 bool shouldKeepInSymtab(
----------------
Agreed.

================
Comment at: ELF/OutputSections.h:280
@@ +279,3 @@
+
+// Outputs Gnu Hash section. For detailed explanation see:
+// https://blogs.oracle.com/ali/entry/gnu_hash_elf_sections
----------------
Gnu -> GNU


http://reviews.llvm.org/D13815