This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj] - Improve dumping of the SHT_LLVM_LINKER_OPTIONS sections.
ClosedPublic

Authored by grimar on Nov 12 2019, 1:55 AM.

Details

Summary

I've added a few tests that shows how the current code could overrun the section data
buffer while dumping. I had to rewrite the code to fix this.
Main problem was that the current code assumed that the section data is correct.
But when it is not - it failed.

Diff Detail

Event Timeline

grimar created this revision.Nov 12 2019, 1:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 12 2019, 1:55 AM
grimar updated this revision to Diff 228832.Nov 12 2019, 1:59 AM
  • Added a test for "empty section" case.
jhenderson added inline comments.Nov 12 2019, 2:07 AM
llvm/test/tools/llvm-readobj/elf-linker-options.test
32

attemp -> attempt
a data -> data
outside of -> outside

39

Should we make these INCOMPLETE-NEXT to show that nothing is printed for the entry? In that case, what do you think about adding a valid pair before to show that earlier valid entries are (not) printed?

56

Same comments for this case as above (both comments and check).

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

found, the -> found. The

MaskRay added inline comments.Nov 12 2019, 10:06 AM
llvm/tools/llvm-readobj/ELFDumper.cpp
6023

Should this be continue?

compnerd added inline comments.Nov 12 2019, 2:15 PM
llvm/tools/llvm-readobj/ELFDumper.cpp
6020

Contents is the correct word for this.

6029

This should probably be a continue.

grimar planned changes to this revision.Nov 14 2019, 3:40 AM
grimar marked an inline comment as done.

Thanks for all the comments. I am going to be off until the next week, so will update this patch after 18th of november.

grimar updated this revision to Diff 229805.Nov 18 2019, 4:23 AM
grimar marked 9 inline comments as done.
  • Addressed review comments and reimplemented.
llvm/test/tools/llvm-readobj/elf-linker-options.test
39

Yeah, I had to significantly change the structure here.

Using returns in the code was not correct I think,
then, if we want to continue dumping sections after reporting a warning, it is probably worth
to show how we dump the possible mixed cases too. So I've combined the valid/invalid cases and improved the error messages displayed.

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

Right. I've changed all the logic here to continue dumping sections even after reporting an error.

6029

Yes.

jhenderson accepted this revision.Nov 18 2019, 4:42 AM

LGTM with two nits.

llvm/test/tools/llvm-readobj/elf-linker-options.test
14

'"c' -> "c"?

43

a one more -> another

This revision is now accepted and ready to land.Nov 18 2019, 4:42 AM

@MaskRay, @compnerd, do you have more comments?

MaskRay accepted this revision.Nov 19 2019, 3:50 PM

LG once @jhenderson's comments are addressed.

This revision was automatically updated to reflect the committed changes.