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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.





================
Comment at: llvm/test/tools/llvm-readobj/ELF/hash-table.test:69
+
+# NOWARN: warning: '[[FILE]]': string table was not found
+
----------------
jhenderson wrote:
> Seems like we shouldn't be getting this warning either, but that's presumably a separate change and more general. Perhaps add a TODO?
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?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:334-335
+    // sections. This violates the ELF specification.
+    if (Obj.getHeader().e_machine != ELF::EM_S390 &&
+        Obj.getHeader().e_machine != ELF::EM_ALPHA)
+      return 4;
----------------
jhenderson wrote:
> 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.
> 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.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2669
+  if (Dumper.getHashTableEntSize() == 8) {
+    auto It = llvm::find_if(ElfMachineType, [&](const EnumEntry<unsigned> &E) {
+      return E.Value == Obj.getHeader().e_machine;
----------------
MaskRay wrote:
> Can It be at the end of ElfMachineType?
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.


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

https://reviews.llvm.org/D88817