Page MenuHomePhabricator

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

Authored by grimar on Oct 5 2020, 3:38 AM.

Details

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.

Diff Detail

Event Timeline

grimar created this revision.Oct 5 2020, 3:38 AM
Herald added a project: Restricted Project. · View Herald Transcript
grimar requested review of this revision.Oct 5 2020, 3:38 AM

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 added a comment.Oct 6 2020, 1:10 AM

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.

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 added a comment.Oct 6 2020, 1:30 AM

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.

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 updated this revision to Diff 296978.Oct 8 2020, 8:22 AM
grimar marked an inline comment as done.
  • Reimplemented in according to the discussion.
MaskRay added inline comments.Oct 8 2020, 9:05 AM
llvm/tools/llvm-readobj/ELFDumper.cpp
2672

Can It be at the end of ElfMachineType?

jhenderson added inline comments.Oct 9 2020, 12:30 AM
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.

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 updated this revision to Diff 297150.Oct 9 2020, 1:47 AM
grimar marked 4 inline comments as done.
  • Addressed review comments.
  • Added the test with sh_entsize == 8.
grimar added a comment.Oct 9 2020, 1:47 AM
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 accepted this revision.Oct 12 2020, 1:37 AM

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).

This revision is now accepted and ready to land.Oct 12 2020, 1:37 AM
grimar marked an inline comment as done.Oct 12 2020, 2:11 AM
grimar added inline comments.
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.

This revision was automatically updated to reflect the committed changes.
grimar marked an inline comment as done.