This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objdump]Improve testing of some switches #1
ClosedPublic

Authored by jhenderson on May 15 2019, 6:21 AM.

Details

Summary

This is the first in a set of patches I have to improve testing of llvm-objdump. This patch targets --all-headers, --section, and --full-contents. In the --section case, it deletes a pre-canned binary which is only used by the one test and replaces it with yaml.

Diff Detail

Repository
rL LLVM

Event Timeline

jhenderson created this revision.May 15 2019, 6:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2019, 6:21 AM
MaskRay added inline comments.May 16 2019, 1:06 AM
test/tools/llvm-objdump/section-filter.test
66 ↗(On Diff #199594)

objdump -d -j.data dumps non-executable sections.

74 ↗(On Diff #199594)

This is no GNU behavior, either... objdump -d -r -j.text dumps .rela.text.

Thanks for the tests. We can improve these aspects...

213 ↗(On Diff #199594)

Apparently we missed a GNU objdump warning...

MaskRay accepted this revision.May 16 2019, 1:06 AM
This revision is now accepted and ready to land.May 16 2019, 1:06 AM

I only have a minor comment (inlined).

Out of curiosity are you doing some code coverage check on the llvm-objdump to find the places uncovered?
Asking because I did that few times for LLD and this gave interesting results that time (IMO). I added many test cases and found few interesting bugs and a dead code pieces.
(i.e. if not - I can try to do that and share the results).

test/tools/llvm-objdump/section-filter.test
222 ↗(On Diff #199594)

Will it be better/simpler just to check no output explicitly?
i.e. something like

# NO-SECTION: Sections:
# NO-SECTION-NOT: {{.*}}

(not sure this is valid, but shows the idea)

jhenderson marked 4 inline comments as done.May 16 2019, 3:12 AM

I only have a minor comment (inlined).

Out of curiosity are you doing some code coverage check on the llvm-objdump to find the places uncovered?
Asking because I did that few times for LLD and this gave interesting results that time (IMO). I added many test cases and found few interesting bugs and a dead code pieces.
(i.e. if not - I can try to do that and share the results).

I've not been doing code coverage checks as such, but I have been inspecting the code to see what behaviour each feature has (and sometimes cross-referencing with GNU). A number of the test cases in this and the following patches that I'm still preparing are based on an internal test suite we have for a proprietary tool.

test/tools/llvm-objdump/section-filter.test
66 ↗(On Diff #199594)

Thanks. I'll file a bug for this.

74 ↗(On Diff #199594)

I agree with this too. See the FIXME above.

213 ↗(On Diff #199594)

Yes, I agree. I'll file a bug for this too.

222 ↗(On Diff #199594)

Yes, probably. I'll try. (Note that {{.*}} won't work since that matchese a string with 0 or more characters, i.e. any string - you just want {{.}}).

grimar added a comment.EditedMay 16 2019, 3:16 AM

I only have a minor comment (inlined).

Out of curiosity are you doing some code coverage check on the llvm-objdump to find the places uncovered?
Asking because I did that few times for LLD and this gave interesting results that time (IMO). I added many test cases and found few interesting bugs and a dead code pieces.
(i.e. if not - I can try to do that and share the results).

I've not been doing code coverage checks as such, but I have been inspecting the code to see what behaviour each feature has (and sometimes cross-referencing with GNU). A number of the test cases in this and the following patches that I'm still preparing are based on an internal test suite we have for a proprietary tool.

I see.
I am curious to run it. For LLD I used our LLD test cases set as a base. I.e. idea was to check which code is covered by our tests. I think it should fit nicely for tools like this one too.

jhenderson marked 6 inline comments as done.

Address review comments: add bug references for GNU compatibility issues, reflow a couple of comments, replace CHECK-NOT list for last case with checking for no output at all.

test/tools/llvm-objdump/section-filter.test
66 ↗(On Diff #199594)
213 ↗(On Diff #199594)
grimar accepted this revision.May 16 2019, 5:04 AM

LGTM

This revision was automatically updated to reflect the committed changes.

@MaskRay, I've committed a slightly different fix to your one for section-filter.test, that preserves the generic parts in a non target-specific test. See rL360904.

@MaskRay, I've committed a slightly different fix to your one for section-filter.test, that preserves the generic parts in a non target-specific test. See rL360904.

Nice!