This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readelf/llvm-readobj] - Improve dumping of broken versioning sections.
ClosedPublic

Authored by grimar on Oct 9 2019, 7:23 AM.

Details

Summary

This updates the elf-invalid-versioning.test test case:
makes a cleanup, adds llvm-readobj calls and fixes 2
crash/assert issues I've found (test cases are provided).

Diff Detail

Event Timeline

grimar created this revision.Oct 9 2019, 7:23 AM
grimar retitled this revision from [llvm-readelf/llvm-readobj] - Improve dumping of the broken versioning sections. to [llvm-readelf/llvm-readobj] - Improve dumping of broken versioning sections..Oct 9 2019, 7:24 AM
grimar planned changes to this revision.Oct 10 2019, 1:03 AM

Going to remove a dependency on D68704.

grimar updated this revision to Diff 224325.Oct 10 2019, 6:13 AM
grimar edited the summary of this revision. (Show Details)
  • Removed dependency on D68704.
grimar updated this revision to Diff 226627.Oct 28 2019, 3:22 AM
  • Rebased. Ping.
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2019, 3:22 AM
jhenderson added inline comments.Oct 28 2019, 7:58 AM
llvm/test/tools/llvm-readobj/elf-invalid-versioning.test
154 ↗(On Diff #226627)

I found it slightly hard to parse this last sentence. Perhaps change != 0 to non-zero.

llvm/tools/llvm-readobj/ELFDumper.cpp
671 ↗(On Diff #226627)

This and the other errors here shouldn't use report_fatal_error. They should use reportError or similar. I'm okay with that being a separate change, but perhaps it should be first to avoid adding another bad usage.

The message could include more context, for example giving the offset or similar, so that a user might be able to identify the bad entry.

3927–3928 ↗(On Diff #226627)

You probably need a test case for vn_file > StringTable.size() (but the string table exists) and vn_file == StringTable.size().

3941–3943 ↗(On Diff #226627)

You probably need a test case for vna_name == StringTable.size().

5720–5723 ↗(On Diff #226627)

Same comment as GNU style.

5735–5738 ↗(On Diff #226627)

Same comment as GNU style.

grimar updated this revision to Diff 227095.Oct 30 2019, 7:46 AM
grimar marked 6 inline comments as done.
  • Addressed review comments.
llvm/tools/llvm-readobj/ELFDumper.cpp
671 ↗(On Diff #226627)

I've used reportError here instead of report_fatal_error and updated the test case accordingly.

3927–3928 ↗(On Diff #226627)

It had an issue when vn_file == StringTable.size(). I've fixed the condition.

jhenderson accepted this revision.Oct 30 2019, 8:01 AM

Looks good to me, aside from some minor comments.

llvm/test/tools/llvm-readobj/elf-invalid-versioning.test
195 ↗(On Diff #227095)

"with the .gnu.version_r section."
"we use the same"

This revision is now accepted and ready to land.Oct 30 2019, 8:01 AM
This revision was automatically updated to reflect the committed changes.