This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj/llvm-readelf] - Reimplement dumping of the SHT_GNU_verneed section.
ClosedPublic

Authored by grimar on Nov 28 2019, 7:12 AM.

Details

Summary

This is similar to D70495, but for SHT_GNU_verneed section.
It solves the same problems: different implementations, lack of error reporting
and no test coverage.

Diff Detail

Event Timeline

grimar created this revision.Nov 28 2019, 7:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2019, 7:12 AM
jhenderson added inline comments.Nov 29 2019, 1:03 AM
llvm/test/tools/llvm-readobj/elf-verneed-invalid.test
92–95

No need for this EMPTY line.

109

Ditto

340

Can you delete this Dependencies section to simplify the YAML?

372

Ditto

llvm/tools/llvm-readobj/ELFDumper.cpp
497–512

I wonder if this block, which is common at least with the verdef section, could be pulled out into a function getLinkAsStrtab or something similar?

grimar updated this revision to Diff 231499.Nov 29 2019, 2:12 AM
grimar marked 8 inline comments as done.
  • Addressed review comments.
llvm/test/tools/llvm-readobj/elf-verneed-invalid.test
92–95

It comes from reportWarning:
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-readobj/llvm-readobj.cpp#L393

And we also print an additional '\n' when reporting errors:
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-readobj/llvm-readobj.cpp#L369

I guess removing might cause issues like "error/warning is printed on the same like with the
normal output", but haven't tried yet.

What do you think if I'll try to remove it in a follow-up?
(I had plan to investigate this issue to address the similar comment for D70665 anyways).

340

I could, but then we'll have one more warning reported:
"invalid SHT_GNU_verneed section with index 1: version dependency 1 goes past the end of the section"

Here I tried to check that we emit only a single warning. I've extended this test to show that we continue printing the output.

372

Done.

llvm/tools/llvm-readobj/ELFDumper.cpp
497–512

Done.

jhenderson added inline comments.Nov 29 2019, 6:16 AM
llvm/test/tools/llvm-readobj/elf-verneed-invalid.test
92–95

Ah, okay. It might be nice to remove it, but if it's not sensible, it's fine to leave it completely. I'm happy for the attempt to be a follow-up.

372

Huh, just realised: how come 0xFFFFFFFF + 0 can't be represented?

396–397

I believe this can't be removed because you need a .dynstr, right? Can you add a comment please.

grimar updated this revision to Diff 231645.Dec 2 2019, 12:30 AM
grimar marked 3 inline comments as done.
  • Addressed review comments.
llvm/test/tools/llvm-readobj/elf-verneed-invalid.test
372

It happens because section offset (0xFFFFFFFF) + Size (0) > file size (560):

ELFFile<ELFT>::getSectionContentsAsArray(const Elf_Shdr *Sec) const {
...
  if ((std::numeric_limits<uintX_t>::max() - Offset < Size) ||
      Offset + Size > Buf.size())
    return createError("section " + getSecIndexForError(this, Sec) +
                       " has a sh_offset (0x" + Twine::utohexstr(Offset) +
                       ") + sh_size (0x" + Twine::utohexstr(Size) +
                       ") that cannot be represented");
396–397

Done.

jhenderson accepted this revision.Dec 2 2019, 1:06 AM

LGTM.

llvm/test/tools/llvm-readobj/elf-verneed-invalid.test
372

Okay, I think I must have missed that in a previous review: "cannot be represented" means the value can't fit in the appropriate uintX_t, whereas if an offset is greater than the file size, it should be a different error message e.g. "section (W) has a sh_offset (X) + sh_size (Y) that is greater than the file size (Z)". I think this should be fixed in a follow-up.

This revision is now accepted and ready to land.Dec 2 2019, 1:06 AM
This revision was automatically updated to reflect the committed changes.