[llvm-readobj] - Split the printGnuHashTable(). NFCI.

[grimar]

Summary

printGnuHashTable contains the code to check the GNU hash table.
This patch splits it to getGnuHashTableChains helper (and reorders slightly to reduce).

I am planning to reuse it from the printGnuHashHistogram() for
the same purpose.

Also there is a one possible crash issue in this code,
that needs adding a bit more code to it to fix (i.e. it is better to
split it out as it is becoming large).

[jhenderson]

llvm/tools/llvm-readobj/ELFDumper.cpp

2742

Does the change to return an empty array in D81937 actually belong here? I'm a little confused why we seem to be introducing a function that is known to be broken from the outset, only to fix it immediately, rather than just doing it right straight up.

2782

Does it make sense to change this to reportUniqueWarning whilst you're here?

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

2742

I just want to keep this patch NFC, It opens road for other patches (D81937 is only one of them). And to fix this place, we need a test case. This is what D81937 does, it adds a test case and fixes another one. I think it deserves to be an independent change.

2782

I can do this. It seems to be a no-op change,

[grimar]

• Use reportUniqueWarning.

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

2742

In fact, D81937 does not need this piece of code to be splitted out to fix the issue it fixes.

[jhenderson]

LGTM.

llvm/tools/llvm-readobj/ELFDumper.cpp

if (Err) {
     reportUniqueWarning(std::move(Err));
     return;
   }
 
   W.printList("Buckets", HashTable->buckets());
   W.printList("Chains", HashTable->chains());
 }
 
+template <class ELFT>
+static Expected<ArrayRef<typename ELFT::Word>>
+getGnuHashTableChains(const typename ELFT::SymRange DynSymTable,
+                      const typename ELFT::GnuHash *GnuHashTable) {
+  size_t NumSyms = DynSymTable.size();
+  if (!NumSyms)
+    return createError("unable to dump 'Values' for the SHT_GNU_HASH "
+                       "section: the dynamic symbol table is empty");
+
+  if (GnuHashTable->symndx < NumSyms)
+    return GnuHashTable->values(NumSyms);
+
+  // A normal empty GNU hash table section produced by linker might have
+  // symndx set to the number of dynamic symbols + 1 (for the zero symbol)
+  // and have dummy null values in the Bloom filter and in the buckets
+  // vector. It happens because the value of symndx is not important for
+  // dynamic loaders when the GNU hash table is empty. They just skip the
+  // whole object during symbol lookup. In such cases, the symndx value is
+  // irrelevant and we should not report a warning.
+  ArrayRef<typename ELFT::Word> Buckets = GnuHashTable->buckets();
+  if (!llvm::all_of(Buckets, [](typename ELFT::Word V) { return V == 0; }))
+    return createError("the first hashed symbol index (" +
+                       Twine(GnuHashTable->symndx) +
+                       ") is larger than the number of dynamic symbols (" +
+                       Twine(NumSyms) + ")");
+
+  return GnuHashTable->values(NumSyms);
+}
+
 template <typename ELFT>
 void ELFDumper<ELFT>::printGnuHashTable(const object::ObjectFile *Obj) {
   DictScope D(W, "GnuHashTable");
   if (!GnuHashTable)
     return;
 
   bool IsHeaderValid;
   Error Err =
void ELFDumper<ELFT>::printGnuHashTable(const object::ObjectFile *Obj) {
 
   if (!DynSymRegion) {
     reportWarning(createError("unable to dump 'Values' for the SHT_GNU_HASH "
                               "section: no dynamic symbol table found"),
                   ObjF->getFileName());
     return;
   }
 
-  size_t NumSyms = dynamic_symbols().size();
-  if (!NumSyms) {
-    reportWarning(createError("unable to dump 'Values' for the SHT_GNU_HASH "
-                              "section: the dynamic symbol table is empty"),
-                  ObjF->getFileName());
-    return;
-  }
-
-  if (GnuHashTable->symndx >= NumSyms) {
-    // A normal empty GNU hash table section produced by linker might have
-    // symndx set to the number of dynamic symbols + 1 (for the zero symbol)
-    // and have dummy null values in the Bloom filter and in the buckets
-    // vector. It happens because the value of symndx is not important for
-    // dynamic loaders when the GNU hash table is empty. They just skip the
-    // whole object during symbol lookup. In such cases, the symndx value is
-    // irrelevant and we should not report a warning.
-    bool IsEmptyHashTable =
-        llvm::all_of(Buckets, [](Elf_Word V) { return V == 0; });
-
-    if (!IsEmptyHashTable) {
-      reportWarning(
-          createError("the first hashed symbol index (" +
-                      Twine(GnuHashTable->symndx) +
-                      ") is larger than the number of dynamic symbols (" +
-                      Twine(NumSyms) + ")"),
-          ObjF->getFileName());
-      return;
-    }
-  }
+  Expected<ArrayRef<Elf_Word>> Chains =
+      getGnuHashTableChains<ELFT>(dynamic_symbols(), GnuHashTable);
+  if (!Chains) {
+    reportWarning(Chains.takeError(), ObjF->getFileName());
 
-  W.printHexList("Values", GnuHashTable->values(NumSyms));
+  W.printHexList("Values", *Chains);
 }
 
 template <typename ELFT> void ELFDumper<ELFT>::printLoadName() {
   W.printString("LoadName", SOName);
 }
 
 template <class ELFT> void ELFDumper<ELFT>::printArchSpecificInfo() {
   const ELFFile<ELFT> *Obj = ObjF->getELFFile();