[llvm-readobj] - Do not skip building of the GNU hash table histogram.

[grimar]

Summary

When the --elf-hash-histogram is used, the code first tries to build
a histogram for the .hash table and then for the .gnu.hash table.

The problem is that dumper might return early when unable or do not need to
build a histogram for the .hash.

This patch reorders the code slightly to fix the issue and adds a test case.

[grimar]

• Fix legacy comments.

[grimar]

Ping.

[jhenderson]

Another one that somehow didn't get submitted for whatever reason. Sorry!

llvm/test/tools/llvm-readobj/ELF/hash-histogram.test

226

Did you consider the following?

## Case A:
...
# RUN:   FileCheck %s --check-prefix=GNU-HASH --implicit-check-not=Histogram

## Case B:
...
# RUN:   FileCheck %s -DFILE=%t6.o --check-prefixes=WARN,GNU-HASH

# WARN: warning: '[[FILE]]': the hash table at offset 0x78 goes past the end of the file (0x350), nbucket = 4294967295, nchain = 1
# GNU-HASH: Histogram for `.gnu.hash' bucket list length (total of 1 buckets)

It saves having to include two separate checks for the same thing.

240

I'm not sure what this comment is trying to tell me, but since the value is a parameter, perhaps it belongs with where the parameter is specified to be 1.

llvm/tools/llvm-readobj/ELFDumper.cpp

4565–4569

Up to you, but these lambdas are quite large, so it might be easier to follow if they were in independent functions (possibly methods of GNUStyle if needed). I'm also happy if you want to save that for a separate change to make it a pure NFC, so that the logic change here can easily be seen.

[grimar]

llvm/test/tools/llvm-readobj/ELF/hash-histogram.test

226

Done.

240

Fixed.

llvm/tools/llvm-readobj/ELFDumper.cpp

4565–4569

Up to you, but these lambdas are quite large, so it might be easier to follow if they were in independent functions

True. I had the same concerns, but stopped on the current version to reduce amount of changes.
I've prepared the follow-up doing this: D80546

[grimar]

• Upload the diff forgotten.

[jhenderson]

LGTM, thanks.

llvm/test/tools/llvm-readobj/ELF/hash-histogram.test

Entries:
       - Tag:   DT_NULL
         Value: 0x0
 DynamicSymbols: []
 ProgramHeaders:
   - Type: PT_LOAD
     Sections:
       - Section: .hash
       - Section: .dynamic
+
+## Check we dump a histogram for the .gnu.hash table even when the .hash table is skipped.
+
+## Case A: the .hash table has no data to build histogram and it is skipped.
+##         (NBUCKET == 0x1 is a no-op: it does not change the number of buckets described with the "Bucket" key).
+# RUN: yaml2obj --docnum=5 -DNBUCKET=0x1 %s -o %t5.o
+# RUN: llvm-readelf --elf-hash-histogram %t5.o 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=GNU-HASH --implicit-check-not="Histogram"
+
+## Case B: the .hash table has a broken nbucket field. We report a warning
+##         and skip dumping of the .hash table.
+# RUN: yaml2obj --docnum=5 -DNBUCKET=0xffffffff %s -o %t6.o
+# RUN: llvm-readelf --elf-hash-histogram %t6.o 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t6.o --check-prefixes=WARN,GNU-HASH
+
+# WARN:     warning: '[[FILE]]': the hash table at offset 0x78 goes past the end of the file (0x350), nbucket = 4294967295, nchain = 1
+# GNU-HASH: Histogram for `.gnu.hash' bucket list length (total of 1 buckets)
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_X86_64
+Sections:
+  - Name:    .hash
+    Type:    SHT_HASH
+    Flags:   [ SHF_ALLOC ]
+    Bucket:  [ 0 ]
+    NBucket: [[NBUCKET]]
+    Chain:   [ 0 ]
+  - Name:  .gnu.hash
+    Type:  SHT_GNU_HASH
+    Flags: [ SHF_ALLOC ]
+    Header:
+      SymNdx: 0x1
+      Shift2: 0x0
+    BloomFilter: [ 0x0 ]
+    HashBuckets: [ 0x00000001, 0x00000004, 0x00000000 ]
+    HashValues:  [ 0x0B887388 ]
+  - Name:  .dynamic
+    Type:  SHT_DYNAMIC
+    Flags: [ SHF_WRITE, SHF_ALLOC ]
+    Entries:
+      - Tag:   DT_HASH
+        Value: 0x0
+      - Tag:   DT_GNU_HASH
+## sizeof(.hash) == 0x28.
+        Value: 0x28
+      - Tag:   DT_NULL
+        Value: 0x0
+DynamicSymbols:
+  - Name: foo
+ProgramHeaders:
+  - Type:  PT_LOAD
+    Sections:
+      - Section: .hash
+      - Section: .gnu.hash
+      - Section: .dynamic

llvm/tools/llvm-readobj/ELFDumper.cpp

 }
 
 // Hash histogram shows  statistics of how efficient the hash was for the
 // dynamic symbol table. The table shows number of hash buckets for different
 // lengths of chains as absolute number and percentage of the total buckets.
 // Additionally cumulative coverage of symbols for each set of buckets.
 template <class ELFT>
 void GNUStyle<ELFT>::printHashHistogram(const ELFFile<ELFT> *Obj) {
-  // Print histogram for .hash section
-  if (const Elf_Hash *HashTable = this->dumper()->getHashTable()) {
-    if (!checkHashTable(Obj, HashTable, this->FileName))
-      return;
-
-    size_t NBucket = HashTable->nbucket;
-    size_t NChain = HashTable->nchain;
-    ArrayRef<Elf_Word> Buckets = HashTable->buckets();
-    ArrayRef<Elf_Word> Chains = HashTable->chains();
+  auto PrintHashHist = [&](const Elf_Hash &HashTable) {
+    size_t NBucket = HashTable.nbucket;
+    size_t NChain = HashTable.nchain;
+    ArrayRef<Elf_Word> Buckets = HashTable.buckets();
+    ArrayRef<Elf_Word> Chains = HashTable.chains();
     size_t TotalSyms = 0;
     // If hash table is correct, we have at least chains with 0 length
     size_t MaxChain = 1;
     size_t CumulativeNonZero = 0;
 
     if (NChain == 0 || NBucket == 0)
       return;
 
for (size_t B = 0; B < NBucket; B++) {
           MaxChain++;
       }
       TotalSyms += ChainLen[B];
     }
 
     if (!TotalSyms)
       return;
 
-    std::vector<size_t> Count(MaxChain, 0) ;
+    std::vector<size_t> Count(MaxChain, 0);
     // Count how long is the chain for each bucket
     for (size_t B = 0; B < NBucket; B++)
       ++Count[ChainLen[B]];
     // Print Number of buckets with each chain lengths and their cumulative
     // coverage of the symbols
     OS << "Histogram for bucket list length (total of " << NBucket
        << " buckets)\n"
        << " Length  Number     % of total  Coverage\n";
     for (size_t I = 0; I < MaxChain; I++) {
       CumulativeNonZero += Count[I] * I;
       OS << format("%7lu  %-10lu (%5.1f%%)     %5.1f%%\n", I, Count[I],
                    (Count[I] * 100.0) / NBucket,
                    (CumulativeNonZero * 100.0) / TotalSyms);
     }
-  }
+  };
 
-  // Print histogram for .gnu.hash section
-  if (const Elf_GnuHash *GnuHashTable = this->dumper()->getGnuHashTable()) {
-    size_t NBucket = GnuHashTable->nbuckets;
-    ArrayRef<Elf_Word> Buckets = GnuHashTable->buckets();
+  auto PrintGnuHashHist = [&](const Elf_GnuHash &GnuHashTable) {
+    size_t NBucket = GnuHashTable.nbuckets;
+    ArrayRef<Elf_Word> Buckets = GnuHashTable.buckets();
     unsigned NumSyms = this->dumper()->dynamic_symbols().size();
     if (!NumSyms)
       return;
-    ArrayRef<Elf_Word> Chains = GnuHashTable->values(NumSyms);
-    size_t Symndx = GnuHashTable->symndx;
+    ArrayRef<Elf_Word> Chains = GnuHashTable.values(NumSyms);
+    size_t Symndx = GnuHashTable.symndx;
     size_t TotalSyms = 0;
     size_t MaxChain = 1;
     size_t CumulativeNonZero = 0;
 
     if (Chains.empty() || NBucket == 0)
       return;
 
     std::vector<size_t> ChainLen(NBucket, 0);
for (size_t B = 0; B < NBucket; B++) {
       ChainLen[B] = Len;
       TotalSyms += Len;
     }
     MaxChain++;
 
     if (!TotalSyms)
       return;
 
-    std::vector<size_t> Count(MaxChain, 0) ;
+    std::vector<size_t> Count(MaxChain, 0);
     for (size_t B = 0; B < NBucket; B++)
       ++Count[ChainLen[B]];
     // Print Number of buckets with each chain lengths and their cumulative
     // coverage of the symbols
     OS << "Histogram for `.gnu.hash' bucket list length (total of " << NBucket
        << " buckets)\n"
        << " Length  Number     % of total  Coverage\n";
-    for (size_t I = 0; I <MaxChain; I++) {
+    for (size_t I = 0; I < MaxChain; I++) {
       CumulativeNonZero += Count[I] * I;
       OS << format("%7lu  %-10lu (%5.1f%%)     %5.1f%%\n", I, Count[I],
                    (Count[I] * 100.0) / NBucket,
                    (CumulativeNonZero * 100.0) / TotalSyms);
     }
-  }
+  };
+
+  // Print histogram for the .hash section.
+  if (const Elf_Hash *HashTable = this->dumper()->getHashTable())
+    if (checkHashTable(Obj, HashTable, this->FileName))
+      PrintHashHist(*HashTable);
+
+  // Print histogram for the .gnu.hash section.
+  if (const Elf_GnuHash *GnuHashTable = this->dumper()->getGnuHashTable())
+    PrintGnuHashHist(*GnuHashTable);
 }
 
 template <class ELFT>
 void GNUStyle<ELFT>::printCGProfile(const ELFFile<ELFT> *Obj) {
   OS << "GNUStyle::printCGProfile not implemented\n";
 }
 
 template <class ELFT>