This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj] - Refactor the MipsGOTParser<ELFT> to stop using report_fatal_error().
ClosedPublic

Authored by grimar on Jul 6 2020, 6:58 AM.

Details

Summary

MipsGOTParser is a helper class that is used to dump MIPS GOT and PLT.
There is a problem with it: it might call report_fatal_error() on invalid input.
When this happens, the tool reports a crash:

# command stderr:
LLVM ERROR: Cannot find PLTGOT dynamic table tag.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backt
race.
Stack dump:
...

Such error were not tested. In this patch I've refactored MipsGOTParser:

I've splitted handling of GOT and PLT to separate methods. This allows to propagate
any possible errors to caller and should allow to dump the PLT when something is wrong
with the GOT and vise versa in the future.

I've added tests for each report_fatal_error()
and now calling the reportError instead. In the future we might want to switch to
reporting warnings, but it requres the additional testing and should
be performed independently.

I've kept unwrapOrError calls untouched for now as I'd like to focus on eliminating
report_fatal_error calls in this patch only (to stop crashing on invalid inputs
when doing inputs fuzzing).

Diff Detail

Event Timeline

grimar created this revision.Jul 6 2020, 6:58 AM
grimar updated this revision to Diff 275701.Jul 6 2020, 7:04 AM
  • Remove unused YAML line.
atanasyan accepted this revision.Jul 6 2020, 1:18 PM

LGTM. Thanks

This revision is now accepted and ready to land.Jul 6 2020, 1:18 PM
MaskRay accepted this revision.Jul 6 2020, 5:46 PM

Some nits.

llvm/test/tools/llvm-readobj/ELF/mips-plt.test
7

# prepending can be committed separately.

llvm/tools/llvm-readobj/ELFDumper.cpp
3034

The canonical term is dynamic tag, not dynamic table tag.

jhenderson added inline comments.Jul 7 2020, 1:37 AM
llvm/test/tools/llvm-readobj/ELF/mips-plt.test
7

+1 to this (in both tests)

llvm/tools/llvm-readobj/ELFDumper.cpp
3087–3094

These two cases don't appear to have tests?

Also, the errors should be "not empty" -> "non-empty" and have lower-case first letters.

grimar updated this revision to Diff 276019.Jul 7 2020, 5:55 AM
grimar marked 5 inline comments as done.
  • Addressed review comments.
llvm/tools/llvm-readobj/ELFDumper.cpp
3034

Fixed.

3087–3094

Oh, right. I've missed them for unknown reason. I've added tests for these 2.

jhenderson accepted this revision.Jul 7 2020, 5:58 AM

LGTM, with message fix.

llvm/tools/llvm-readobj/ELFDumper.cpp
3087–3094

not empty -> non-empty

(in both errors still)

grimar marked an inline comment as done.Jul 7 2020, 6:43 AM
grimar added inline comments.
llvm/tools/llvm-readobj/ELFDumper.cpp
3087–3094

(in both errors still)

Sorry!

This revision was automatically updated to reflect the committed changes.