This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj] - Do not fail to dump the object which has wrong type of .shstrtab.
ClosedPublic

Authored by grimar on Jun 13 2019, 7:06 AM.

Details

Summary

Imagine we have object that has .shstrtab with type != SHT_STRTAB.
In this case, we fail to dump the object, though GNU readelf dumps it without
any issues and warnings.

This patch fixes that. It adds a code to ELFDumper.cpp which is based on the implementation of getSectionName from the ELF.h:

https://github.com/llvm-mirror/llvm/blob/master/include/llvm/Object/ELF.h#L608
https://github.com/llvm-mirror/llvm/blob/master/include/llvm/Object/ELF.h#L431
https://github.com/llvm-mirror/llvm/blob/master/include/llvm/Object/ELF.h#L539

The difference is that all non critical errors are ommitted what allows us to
improve the dumping on a tool side. Also, this opens a road for a follow-up that
should allow us to dump the section headers, but drop the section names in case if .shstrtab is completely absent and/or broken.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Jun 13 2019, 7:06 AM
jhenderson added inline comments.Jun 13 2019, 7:38 AM
tools/llvm-readobj/ELFDumper.cpp
3012 ↗(On Diff #204525)

Could this call reportError(Twine) at least? Ideally, I'd prefer it if it called a function that included the file name too. I think there's one in llvm-readobj.cpp that would be useful: reportError(StringRef Input, Error Err). I don't see a reason that this couldn't be exposed to the rest of llvm-readobj.

grimar updated this revision to Diff 204730.Jun 14 2019, 3:06 AM
grimar marked an inline comment as done.
  • Addressed review comments.
  • Fixed compilation with GCC.
  • Added 2 lines of code to fix the Object/no-section-header-string-table.test

(It is needed for no section string table case, it matches the existent behavior).

tools/llvm-readobj/ELFDumper.cpp
3012 ↗(On Diff #204525)

I changed to reportError(Twine). It looks a bit problematic to get a file name here and I am not sure it is very useful to do this change right now, because actually, I think we do not need this error at all here. I introduced it to minimize the change of the existent logic (i.e. we already had this fatal error at deeper levels), but I think getSectionName actually should never fail and terminate the proccess. It probably might return a Error instead or a pair of section name + warning message, or something else, but idea is that if we can`t get the section name because of any reason we should probably continue dumping and just show something like <section name unknown> instead of the section name.

jhenderson accepted this revision.Jun 14 2019, 4:32 AM

LGTM.

tools/llvm-readobj/ELFDumper.cpp
3012 ↗(On Diff #204525)

The easiest way would be to pass in ELFObjectFile rather than ELFFile, I think, but maybe that doesn't make sense.

This revision is now accepted and ready to land.Jun 14 2019, 4:32 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2019, 4:54 AM