Page MenuHomePhabricator

[llvm-readobj] - Report warnings instead of errors for broken relocations.
Needs ReviewPublic

Authored by grimar on Fri, Feb 14, 1:52 AM.

Details

Summary

This is a follow-up for https://reviews.llvm.org/D74545.

It adds test cases for each incorrect case returned in getRelocationTarget.

Diff Detail

Event Timeline

grimar created this revision.Fri, Feb 14, 1:52 AM
Herald added a project: Restricted Project. · View Herald Transcript
grimar planned changes to this revision.Fri, Feb 14, 1:57 AM

Will check something.

grimar updated this revision to Diff 244607.Fri, Feb 14, 3:26 AM
  • Improved how errors looks like.
  • Fixed one more test case.
MaskRay added inline comments.Fri, Feb 14, 2:35 PM
llvm/test/Object/invalid.test
420–421

We probably should split invalid.test. For example, segments related errors and sections related errors can be separated.

llvm/test/tools/llvm-readobj/ELF/relocations-errors.test
1 ↗(On Diff #244607)

Maybe relocation-errors.test?

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

Target->first

5752

Target->second

grimar updated this revision to Diff 244868.Sun, Feb 16, 4:30 AM
grimar marked 5 inline comments as done.
  • Addressed review comments.
llvm/test/Object/invalid.test
420–421

It feels that some of the tests might go to the corresponding tools test folders.
(e.g. docnum 21 below)

jhenderson added inline comments.Mon, Feb 17, 7:47 AM
llvm/test/Object/invalid.test
420–421

Better yet would be to replace those tests which are testing library code with gtest unit tests to test the interfaces directly instead (and then additional tests in the tools folder to show the tools correctly handle the respective places properly).

llvm/test/tools/llvm-readobj/ELF/relocation-errors.test
2

Delete "do"

56–59

Seems like this overlaps with reloc-no-sym.test? Could we get rid of one or the other?

70

relocation in a section...

71

is neither SHT_SYMTAB nor SHT_DYNSYM

grimar updated this revision to Diff 245094.Tue, Feb 18, 12:58 AM
grimar marked 4 inline comments as done.
  • Addressed review comments.
llvm/test/tools/llvm-readobj/ELF/relocation-errors.test
56–59

This one is a bit different: here we have a SHT_REL section.
(I can't test an addend here, like reloc-no-sym.test do).

And the intention to have it also was to show the order of warnings/normal relocations.
I.e. I wanted to have at least one valid relocation in the middle and
"relocation against a symbol with index 0" works good as it is related to the context and the
current code.

So, I'd keep both. (btw, reloc-no-sym.test seems lacks GNU test and SHT_REL test and might need to be improved).