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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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



================
Comment at: llvm/test/tools/llvm-readobj/ELF/hash-histogram.test:226
+
+# GNU-HASH-B: warning: '[[FILE]]': the hash table at offset 0x78 goes past the end of the file (0x350), nbucket = 4294967295, nchain = 1
+# GNU-HASH-B: Histogram for `.gnu.hash' bucket list length (total of 1 buckets)
----------------
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.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/hash-histogram.test:240
+    Bucket:  [ 0 ]
+## NBUCKET == 1 is a no-op.
+    NBucket: [[NBUCKET]]
----------------
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.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4565
 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;
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80204/new/

https://reviews.llvm.org/D80204