[llvm-readobj/elf] - Ignore the hash table when on EM_S390/EM_ALPHA platforms.

[grimar]

Summary

Specification for SHT_HASH table says (https://refspecs.linuxbase.org/elf/gabi4+/ch5.dynamic.html#hash)
that it contains Elf32_Word entries for both 32/64 bit objects.

But there is a problem with EM_S390 and ELF::EM_ALPHA platforms: they use 8-bytes entries.
(see the issue reported: https://bugs.llvm.org/show_bug.cgi?id=47681).

Currently we might infer the size of the dynamic symbols table from hash table,
but because of the issue mentioned, the calculation is wrong. And also we don't dump the hash table
properly.

I am not sure if we want to support 8-bytes entries as they violates specification and also the
.hash table is kind of deprecated by itself (the .gnu.hash table is used nowadays).
So, the solution this patch suggests is to ban using of the hash table on EM_S390/EM_ALPHA platforms.

[uweigand]

It looks like this will trigger a warning being emitted on every use of llvm-readobj on a EM_S390 object file, which is probably not what we want. (They all also contain .gnu.hash, so the fact that .hash is ignored is not anything that the user should be concerned about. -- This warning may just cause unnecessary confusion, or parse error with automated tools etc.)

[grimar]

In D88817#2311572, @uweigand wrote:

It looks like this will trigger a warning being emitted on every use of llvm-readobj on a EM_S390 object file, which is probably not what we want. (They all also contain .gnu.hash, so the fact that .hash is ignored is not anything that the user should be concerned about. -- This warning may just cause unnecessary confusion, or parse error with automated tools etc.)

I'd like to wait for opinions of @jhenderson/@MaskRay to confirm them are happy to silently ignore the .hash section on EM_S390/EM_ALPHA platforms. It might probably
be confusing to see a .hash section in an object and no output with --hash-table.

[uweigand]

In D88817#2313657, @grimar wrote:

In D88817#2311572, @uweigand wrote:

It looks like this will trigger a warning being emitted on every use of llvm-readobj on a EM_S390 object file, which is probably not what we want. (They all also contain .gnu.hash, so the fact that .hash is ignored is not anything that the user should be concerned about. -- This warning may just cause unnecessary confusion, or parse error with automated tools etc.)

I'd like to wait for opinions of @jhenderson/@MaskRay to confirm them are happy to silently ignore the .hash section on EM_S390/EM_ALPHA platforms. It might probably
be confusing to see a .hash section in an object and no output with --hash-table.

If you use an option that specifically refers to the hash table like --hash-table, I'd be fine with a warning in that case. But the warning as currently implemented seems to trigger with any invocation of llvm-readobj, even though most users will never care about the hash table (or even know about it in the first place).

[grimar]

In D88817#2313661, @uweigand wrote:

In D88817#2313657, @grimar wrote:

In D88817#2311572, @uweigand wrote:

It looks like this will trigger a warning being emitted on every use of llvm-readobj on a EM_S390 object file, which is probably not what we want. (They all also contain .gnu.hash, so the fact that .hash is ignored is not anything that the user should be concerned about. -- This warning may just cause unnecessary confusion, or parse error with automated tools etc.)

I'd like to wait for opinions of @jhenderson/@MaskRay to confirm them are happy to silently ignore the .hash section on EM_S390/EM_ALPHA platforms. It might probably
be confusing to see a .hash section in an object and no output with --hash-table.

If you use an option that specifically refers to the hash table like --hash-table, I'd be fine with a warning in that case. But the warning as currently implemented seems to trigger with any invocation of llvm-readobj, even though most users will never care about the hash table (or even know about it in the first place).

Yes, I see. The current logic also always tries to infer the size of the dynamic symbol table from the DT_HASH hash table, if present. It will be inconsistent that we do it in one case (on all other platforms),
but silently don't do in another case (on EM_S390/EM_ALPHA). But since the reason of the inconsistency is that the .hash table on these platforms violates ELF specification, personally I think we can ignore it.
Because the only another real option I see is to change the code to support 8 bytes entries, what looks excessive, given that the .hash table is a depricated section that was just broken from the begining on s390/alpha.

So I just want to hear from others that they are happy with doing it, before I start adjusting the patch.

[jhenderson]

I agree that it doesn't make sense to warn for the hash table not being valid in this context, at least for every invocation. I think a warning when attempting to use it for something specific that cannot be derived from the GNU hash table (or possibly where the GNU hash table is missing) would be useful. It's probably a good idea to have a test case that shows that the warning is NOT produced when the hash table isn't needed for anything.

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

47

(no need to say "normal" - it doesn't provide any cases where it doesn't in the ELF spec)

[grimar]

• Reimplemented in according to the discussion.

[MaskRay]

llvm/tools/llvm-readobj/ELFDumper.cpp

2672

Can It be at the end of ElfMachineType?

[jhenderson]

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

62

70

Seems like we shouldn't be getting this warning either, but that's presumably a separate change and more general. Perhaps add a TODO?

llvm/tools/llvm-readobj/ELFDumper.cpp

334–335

I don't know why, but I'd find it slightly easier to follow this logic if it was inverted, i.e. check whether the machine is (not "is not") S390/Alpha, then return 8 if so.

[jhenderson]

Maybe worth a test case showing the behaviour for a sh_entsize == 8 hash table for a platform that normally uses 4-byte entsize (e.g. x86_64)?

[grimar]

• Addressed review comments.
• Added the test with sh_entsize == 8.

[grimar]

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

70

We report the same warning below (--docnum=2). It happens because the code always tries to get the SOName string (DT_SONAME, which is 0 by default), but we have no DT_STRTAB tag. The way to fix it is to add it, but I am not sure we should, it is probably unrelated to what we test?

llvm/tools/llvm-readobj/ELFDumper.cpp

334–335

not "is not"

It would be:

if (!(Obj.getHeader().e_machine != ELF::EM_S390 &&
    Obj.getHeader().e_machine != ELF::EM_ALPHA))
  return 8;

Looks complex. I think just the following looks better, if you want to revert the condition:

if (Obj.getHeader().e_machine == ELF::EM_S390 ||
    Obj.getHeader().e_machine == ELF::EM_ALPHA)
  return 8;

That is what I did.

2672

Nope. getHashTableEntSize returns 8 only for EM_S390 and EM_ALPHA currently.
If it start return it for other EM_* types (unlikely I think) that are not in ElfMachineType in future,
then ElfMachineType will have to be updated too.

[jhenderson]

My comment re. SONAME are unrelated to this patch. LGTM, with nit.

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

47

70

I agree it's unrelated here. I'm just thinking that we shouldn't report the warning at all, even if there is no SOName string unless it is actually required. At least for llvm-readelf output, the DT_SONAME tag isn't needed for anything in most cases (in llvm-readobj output I see there's the LoadName field, although I wonder whether we should simply not print things like that without some option being specified - but that's another issue again).

[grimar]

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

70

Perhaps we can adjust the logic of llvm-readobj to stop printing LoadName field when there is no
DT_SONAME tag. And we also can delay getting the SOName until that moment.
With it llvm-readelf will never report a warning, as it does not use the SOName at all and llvm-readobjs output will be probably more reasonable even without adding any additional options.

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

 # HASH-NEXT:   Num Buckets: 2
 # HASH-NEXT:   Num Chains: 3
 # HASH-NEXT:   Buckets: [1, 2]
 # HASH-NEXT:   Chains: [3, 4, 5]
 # HASH-NEXT: }
 
 --- !ELF
 FileHeader:
-  Class:   ELFCLASS[[BITS]]
+  Class:   ELFCLASS[[BITS=64]]
   Data:    ELFDATA2LSB
   Type:    ET_DYN
   Machine: [[MACHINE]]
 Sections:
-  - Name:   .hash
-    Type:   SHT_HASH
-    Flags:  [ SHF_ALLOC ]
-    Bucket: [ 1, 2 ]
-    Chain:  [ 3, 4, 5 ]
+  - Name:    .hash
+    Type:    SHT_HASH
+    Flags:   [ SHF_ALLOC ]
+    Bucket:  [ 1, 2 ]
+    Chain:   [ 3, 4, 5 ]
+    EntSize: [[ENTSIZE=4]]
   - Name:  .dynamic
     Type:  SHT_DYNAMIC
     Flags: [ SHF_ALLOC ]
     Entries:
       - Tag:   DT_HASH
         Value: 0x0
       - Tag:   DT_NULL
         Value: 0x0
 ProgramHeaders:
   - Type: PT_LOAD
     Sections:
       - Section: .hash
       - Section: .dynamic
 
+## Document that we ignore the sh_entsize value when dumping the hash section.
+## Implementation assumes that the size of entries is 4, matching the ELF specification.
+
+# RUN: yaml2obj --docnum=1 -DENTSIZE=8 -DBITS=64 -DMACHINE=EM_X86_64 %s -o %t.x64.ent8
+# RUN: yaml2obj --docnum=1 -DENTSIZE=8 -DBITS=32 -DMACHINE=EM_386 %s -o %t.x32.ent8
+
+# RUN: llvm-readobj --hash-table %t.x64.ent8 | FileCheck %s --check-prefix=HASH
+# RUN: llvm-readelf --hash-table %t.x64.ent8 | FileCheck %s --check-prefix=HASH
+# RUN: llvm-readobj --hash-table %t.x32.ent8 | FileCheck %s --check-prefix=HASH
+# RUN: llvm-readelf --hash-table %t.x32.ent8 | FileCheck %s --check-prefix=HASH
+
+## We don't support dumping hash tables on EM_S390 and EM_ALPHA platforms and report a warning.
+## On these platforms the size of entries is 8, which violates the ELF specification, which says that the size
+## of hash entries in the hash table must be 4.
+
+# RUN: yaml2obj --docnum=1 -DMACHINE=EM_S390 %s -o %t.s390
+# RUN: llvm-readobj --hash-table %t.s390 2>&1 | FileCheck %s -DFILE=%t.s390 --check-prefixes=WARN-HASH -DNAME="IBM S/390"
+# RUN: llvm-readelf --hash-table %t.s390 2>&1 | FileCheck %s -DFILE=%t.s390 --check-prefixes=WARN-HASH -DNAME="IBM S/390"
+
+# WARN-HASH:      HashTable {
+# WARN-HASH-NEXT: warning: '[[FILE]]': the hash table at 0x78 is not supported: it contains non-standard 8 byte entries on [[NAME]] platform
+# WARN-HASH-NEXT: }
+
+# RUN: yaml2obj --docnum=1 -DMACHINE=EM_ALPHA %s -o %t.alpha
+# RUN: llvm-readobj --hash-table %t.alpha 2>&1 | FileCheck %s -DFILE=%t.alpha --check-prefixes=WARN-HASH -DNAME="EM_ALPHA"
+# RUN: llvm-readelf --hash-table %t.alpha 2>&1 | FileCheck %s -DFILE=%t.alpha --check-prefixes=WARN-HASH -DNAME="EM_ALPHA"
+
+## We don't report warnings about the unsupported hash table on EM_S390 and EM_ALPHA platforms
+## when --hash-table is not requested.
+
+# RUN: llvm-readobj %t.s390 2>&1 | FileCheck %s -DFILE=%t.s390 --implicit-check-not="warning:"  --check-prefix=NOWARN
+# RUN: llvm-readelf %t.s390 2>&1 | FileCheck %s -DFILE=%t.s390 --implicit-check-not="warning:"  --check-prefix=NOWARN
+# RUN: llvm-readobj %t.alpha 2>&1 | FileCheck %s -DFILE=%t.alpha --implicit-check-not="warning:" --check-prefix=NOWARN
+# RUN: llvm-readelf %t.alpha 2>&1 | FileCheck %s -DFILE=%t.alpha --implicit-check-not="warning:" --check-prefix=NOWARN
+
+# NOWARN: warning: '[[FILE]]': string table was not found
+
 ## Check we can dump the SHT_HASH section even when an object
 ## does not have the section header table.
 
 # RUN: yaml2obj --docnum=2 -DNOHEADERS=true %s -o %t.noshdr
 # RUN: llvm-readobj --hash-table %t.noshdr 2>&1 | \
 # RUN:   FileCheck %s -DFILE=%t.noshdr --check-prefix=NOSHDR --implicit-check-not=warning:
 # RUN: llvm-readelf --hash-table %t.noshdr 2>&1 | \
 # RUN:   FileCheck %s -DFILE=%t.noshdr --check-prefix=NOSHDR --implicit-check-not=warning:

llvm/tools/llvm-readobj/ELFDumper.cpp

private:
   };
   mutable SmallVector<Optional<VersionEntry>, 16> VersionMap;
 
   std::unordered_set<std::string> Warnings;
 
   std::string describe(const Elf_Shdr &Sec) const;
 
 public:
+  unsigned getHashTableEntSize() const {
+    // EM_S390 and ELF::EM_ALPHA platforms use 8-bytes entries in SHT_HASH
+    // sections. This violates the ELF specification.
+    if (Obj.getHeader().e_machine == ELF::EM_S390 ||
+        Obj.getHeader().e_machine == ELF::EM_ALPHA)
+      return 8;
+    return 4;
+  }
+
   Elf_Dyn_Range dynamic_table() const {
     // A valid .dynamic section contains an array of entries terminated
     // with a DT_NULL entry. However, sometimes the section content may
     // continue past the DT_NULL entry, so to dump the section correctly,
     // we first find the end of the entries by iterating over them.
     Elf_Dyn_Range Table = DynamicTable.getAsArrayRef<Elf_Dyn>();
 
     size_t Size = 0;
if (StringTableSize > FileSize - Offset)
           " goes past the end of the file (0x" + Twine::utohexstr(FileSize) +
           ") with DT_STRSZ = 0x" + Twine::utohexstr(StringTableSize)));
     else
       DynamicStringTable = StringRef(StringTableBegin, StringTableSize);
   }
 
   SOName = getDynamicString(SONameOffset);
 
+  const bool IsHashTableSupported = getHashTableEntSize() == 4;
   if (DynSymRegion) {
     // Often we find the information about the dynamic symbol table
     // location in the SHT_DYNSYM section header. However, the value in
     // DT_SYMTAB has priority, because it is used by dynamic loaders to
     // locate .dynsym at runtime. The location we find in the section header
     // and the location we find here should match.
     if (DynSymFromTable && DynSymFromTable->Addr != DynSymRegion->Addr)
       reportUniqueWarning(
           createError("SHT_DYNSYM section header and DT_SYMTAB disagree about "
                       "the location of the dynamic symbol table"));
 
     // According to the ELF gABI: "The number of symbol table entries should
     // equal nchain". Check to see if the DT_HASH hash table nchain value
     // conflicts with the number of symbols in the dynamic symbol table
     // according to the section header.
-    if (HashTable) {
+    if (HashTable && IsHashTableSupported) {
       if (DynSymRegion->EntSize == 0)
         reportUniqueWarning(
             createError("SHT_DYNSYM section has sh_entsize == 0"));
       else if (HashTable->nchain != DynSymRegion->Size / DynSymRegion->EntSize)
         reportUniqueWarning(createError(
             "hash table nchain (" + Twine(HashTable->nchain) +
             ") differs from symbol count derived from SHT_DYNSYM section "
             "header (" +
if (!DynSymRegion) {
       DynSymRegion->Addr = DynSymFromTable->Addr;
       DynSymRegion->EntSize = DynSymFromTable->EntSize;
       DynSymRegion->EntSizePrintName = DynSymFromTable->EntSizePrintName;
     }
   }
 
   // Derive the dynamic symbol table size from the DT_HASH hash table, if
   // present.
-  if (HashTable && DynSymRegion) {
+  if (HashTable && IsHashTableSupported && DynSymRegion) {
     const uint64_t FileSize = Obj.getBufSize();
     const uint64_t DerivedSize =
         (uint64_t)HashTable->nchain * DynSymRegion->EntSize;
     const uint64_t Offset = (const uint8_t *)DynSymRegion->Addr - Obj.base();
     if (DerivedSize > FileSize - Offset)
       reportUniqueWarning(createError(
           "the size (0x" + Twine::utohexstr(DerivedSize) +
           ") of the dynamic symbol table at 0x" + Twine::utohexstr(Offset) +
template <class ELFT> void ELFDumper<ELFT>::printNeededLibraries() {
 
   llvm::sort(Libs);
 
   for (StringRef L : Libs)
     W.startLine() << L << "\n";
 }
 
 template <class ELFT>
-static Error checkHashTable(const ELFFile<ELFT> &Obj,
+static Error checkHashTable(const ELFDumper<ELFT> &Dumper,
                             const typename ELFT::Hash *H,
                             bool *IsHeaderValid = nullptr) {
-  auto MakeError = [&](uint64_t Off, const Twine &Msg = "") {
-    return createError("the hash table at offset 0x" + Twine::utohexstr(Off) +
+  const ELFFile<ELFT> &Obj = *Dumper.getElfObject().getELFFile();
+  const uint64_t SecOffset = (const uint8_t *)H - Obj.base();
+  if (Dumper.getHashTableEntSize() == 8) {
+    auto It = llvm::find_if(ElfMachineType, [&](const EnumEntry<unsigned> &E) {
+      return E.Value == Obj.getHeader().e_machine;
+    });
+    if (IsHeaderValid)
+      *IsHeaderValid = false;
+    return createError("the hash table at 0x" + Twine::utohexstr(SecOffset) +
+                       " is not supported: it contains non-standard 8 "
+                       "byte entries on " +
+                       It->AltName + " platform");
+  }
+
+  auto MakeError = [&](const Twine &Msg = "") {
+    return createError("the hash table at offset 0x" +
+                       Twine::utohexstr(SecOffset) +
                        " goes past the end of the file (0x" +
                        Twine::utohexstr(Obj.getBufSize()) + ")" + Msg);
   };
 
   // Each SHT_HASH section starts from two 32-bit fields: nbucket and nchain.
   const unsigned HeaderSize = 2 * sizeof(typename ELFT::Word);
-  const uint64_t SecOffset = (const uint8_t *)H - Obj.base();
 
   if (IsHeaderValid)
     *IsHeaderValid = Obj.getBufSize() - SecOffset >= HeaderSize;
 
   if (Obj.getBufSize() - SecOffset < HeaderSize)
-    return MakeError(SecOffset);
+    return MakeError();
 
   if (Obj.getBufSize() - SecOffset - HeaderSize <
       ((uint64_t)H->nbucket + H->nchain) * sizeof(typename ELFT::Word))
-    return MakeError(SecOffset, ", nbucket = " + Twine(H->nbucket) +
-                                    ", nchain = " + Twine(H->nchain));
+    return MakeError(", nbucket = " + Twine(H->nbucket) +
   return Error::success();
 }
 
 template <class ELFT>
 static Error checkGNUHashTable(const ELFFile<ELFT> &Obj,
                                const typename ELFT::GnuHash *GnuHashTable,
                                bool *IsHeaderValid = nullptr) {
   const uint8_t *TableData = reinterpret_cast<const uint8_t *>(GnuHashTable);

 }
 
 template <typename ELFT> void ELFDumper<ELFT>::printHashTable() {
   DictScope D(W, "HashTable");
   if (!HashTable)
     return;
 
   bool IsHeaderValid;
-  Error Err = checkHashTable(Obj, HashTable, &IsHeaderValid);
+  Error Err = checkHashTable(*this, HashTable, &IsHeaderValid);
   if (IsHeaderValid) {
     W.printNumber("Num Buckets", HashTable->nbucket);
     W.printNumber("Num Chains", HashTable->nchain);
   }
 
   if (Err) {
     reportUniqueWarning(std::move(Err));
     return;
while (true) {
         break;
     }
   }
 }
 
 template <class ELFT> void GNUStyle<ELFT>::printHashSymbols() {
   if (const Elf_Hash *SysVHash = this->dumper().getHashTable()) {
     OS << "\n Symbol table of .hash for image:\n";
-    if (Error E = checkHashTable<ELFT>(this->Obj, SysVHash))
+    if (Error E = checkHashTable<ELFT>(this->dumper(), SysVHash))
       this->reportUniqueWarning(std::move(E));
     else
       printHashTableSymbols(*SysVHash);
   }
 
   // Try printing the .gnu.hash table.
   if (const Elf_GnuHash *GnuHash = this->dumper().getGnuHashTable()) {
     OS << "\n Symbol table of .gnu.hash for image:\n";

 
 // Hash histogram shows statistics of how efficient the hash was for the
 // dynamic symbol table. The table shows the number of hash buckets for
 // different lengths of chains as an absolute number and percentage of the total
 // buckets, and the cumulative coverage of symbols for each set of buckets.
 template <class ELFT> void GNUStyle<ELFT>::printHashHistograms() {
   // Print histogram for the .hash section.
   if (const Elf_Hash *HashTable = this->dumper().getHashTable()) {
-    if (Error E = checkHashTable<ELFT>(this->Obj, HashTable))
+    if (Error E = checkHashTable<ELFT>(this->dumper(), HashTable))
       this->reportUniqueWarning(std::move(E));
     else
       printHashHistogram(*HashTable);
   }
 
   // Print histogram for the .gnu.hash section.
   if (const Elf_GnuHash *GnuHashTable = this->dumper().getGnuHashTable()) {
     if (Error E = checkGNUHashTable<ELFT>(this->Obj, GnuHashTable))