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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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? |
- Addressed review comments.
llvm/test/tools/llvm-readobj/elf-verneed-invalid.test | ||
---|---|---|
92–95 | It comes from reportWarning: And we also print an additional '\n' when reporting errors: I guess removing might cause issues like "error/warning is printed on the same like with the What do you think if I'll try to remove it in a follow-up? | |
340 | I could, but then we'll have one more warning reported: 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. |
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. |
- 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. |
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. |
No need for this EMPTY line.