This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj/llvm-readelf] - Report a warning instead of a error when dumping a broken dynamic section.
ClosedPublic

Authored by grimar on Jul 10 2019, 2:12 AM.

Details

Summary

I've splitted this from D64401.
It does not make sence to stop dumping the object if the broken
dynamic section was found. In this patch I changed the behavior from
"report an error" to "report a warning". This matches GNU.

Diff Detail

Event Timeline

grimar created this revision.Jul 10 2019, 2:12 AM
grimar updated this revision to Diff 208903.Jul 10 2019, 2:36 AM
  • Improve one of the test cases a bit.
MaskRay added inline comments.Jul 10 2019, 2:45 AM
tools/llvm-readobj/ELFDumper.cpp
139

Include sh_entsize in the warning?

grimar updated this revision to Diff 208910.Jul 10 2019, 3:32 AM
  • Addressed review comment.

How about a test case that shows that more stuff can be printed after the aborted dynamic printing, perhaps using --all? Also, is there a risk that this will print the same warning more than once for the same section, if multiple options are used?

How about a test case that shows that more stuff can be printed after the aborted dynamic printing, perhaps using --all?

Working on that.

Also, is there a risk that this will print the same warning more than once for the same section, if multiple options are used?

Yes, it might do that. It can be fixed easily, for example DynRegionInfo can have a flag saying that region is not valid and so
we then can report a warning only once and set this flag. Though I am not sure this is a problem to report this warning in each place we were
unable to dump something too. I would not expect to see too many places and it makes the output very clear. I.e. it is visible
that there was no information dumped because of a particular error and not something else.

grimar updated this revision to Diff 209130.Jul 10 2019, 11:56 PM
  • Addressed review comments.
  • Added flush calls to reportWarning
jhenderson added inline comments.Jul 11 2019, 1:47 AM
test/tools/llvm-readobj/elf-dynamic-malformed.test
14

I think you've gone a bit over the top with this test case now. I think it's sufficient to test the context either side of each warning, and add --implicit-check-not=warning to ensure that you've caught them all. Something like:

WARN: warning: invalid section size (4) or entity size (16)
WARN-EMPTY:
WARN-NEXT: File:
WARN: Symbols [
WARN: ]
WARN-EMPTY:
WARN-NEXT: warning: invalid section size (4) or entity size (16)
WARN-NEXT: ProgramHeaders

You can follow a similar process with the GNU output.

125

Why is there a warning here? I think it's where the dynamic table should be printed, in which case, please add a comment stating as such.

tools/llvm-readobj/llvm-readobj.cpp
383 ↗(On Diff #209130)

I'm not sure this flush is needed, since errs() is unbuffered.

grimar updated this revision to Diff 209170.Jul 11 2019, 4:32 AM
grimar marked 3 inline comments as done.
  • Addressed review comments.
test/tools/llvm-readobj/elf-dynamic-malformed.test
125

Yes, it is because of that. Added a comment.

tools/llvm-readobj/llvm-readobj.cpp
383 ↗(On Diff #209130)

You're right. I forgot or didn't know that.

jhenderson accepted this revision.Jul 11 2019, 4:37 AM

LGTM, with one suggestion.

test/tools/llvm-readobj/elf-dynamic-malformed.test
14

I'd be inclined to add WARN-NEXT: File: ... here to show the context of this warning.

This revision is now accepted and ready to land.Jul 11 2019, 4:37 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2019, 5:26 AM