[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