This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jhenderson on May 22 2019, 7:54 AM.

Details

Summary

This is the third commit in a series of patches to improve test coverage of llvm-objdump. In this patch I have added a number of tests testing various aspects of disassembly.

Diff Detail

Repository
rL LLVM

Event Timeline

jhenderson created this revision.May 22 2019, 7:54 AM
rupprecht accepted this revision.May 22 2019, 10:55 AM

btw, a common tag I see in titles for a patch like this is [test] since you aren't touching any code. I should have mentioned that for the previous patches...

Just a few minor comments

test/tools/llvm-objdump/X86/disasm-specific-funcs-mangled-name.test
6 ↗(On Diff #200743)

--disassemble-functions=foo() should be single quoted to handle (), e.g.

$ echo foo()
bash: syntax error near unexpected token `('
test/tools/llvm-objdump/X86/elf-disasm-dynamic-symbols.test
10 ↗(On Diff #200743)

nit: use llvm-strip in-place (no %t2)

This revision is now accepted and ready to land.May 22 2019, 10:55 AM
jhenderson marked 2 inline comments as done.May 23 2019, 1:42 AM
jhenderson added inline comments.
test/tools/llvm-objdump/X86/disasm-specific-funcs-mangled-name.test
6 ↗(On Diff #200743)

I'll check this before committing. I had this vague impression that lit quotes the arguments, but I might be completely wrong.

test/tools/llvm-objdump/X86/elf-disasm-dynamic-symbols.test
10 ↗(On Diff #200743)

I'd actually refer not to use llvm-strip here, because that then means if the second part of the test fails, you can't inspect the object used in the first part. But I'm not opposed to it if you feel strongly. Happy to update this afterwards.

jhenderson marked an inline comment as done.May 23 2019, 1:43 AM
jhenderson added inline comments.
test/tools/llvm-objdump/X86/elf-disasm-dynamic-symbols.test
10 ↗(On Diff #200743)

refer -> prefer

jhenderson retitled this revision from [llvm-objdump] Improve testing of some switches #3 to [llvm-objdump][test] Improve testing of some switches #3.May 23 2019, 1:43 AM
grimar accepted this revision.May 23 2019, 3:15 AM

Few nits. LGTM.

test/tools/llvm-objdump/X86/disasm-specific-funcs-mangled-name.test
13 ↗(On Diff #200743)

I would suggest to reduce amount of spaces here and below.
Looks a bit better when compact:
0: 90 nop

Up to you though.

test/tools/llvm-objdump/X86/disasm-specific-funcs.test
1 ↗(On Diff #200743)

Add a test case description?

15 ↗(On Diff #200743)

Remove excessive spaces between FOO-NEXT: and addresses?

40 ↗(On Diff #200743)

--implicit-check-not=Disassembly ?

MaskRay accepted this revision.May 23 2019, 3:33 AM

Thank you for improving tests! I only have a couple of nits.

test/tools/llvm-objdump/X86/disasm-specific-funcs-mangled-name.test
6 ↗(On Diff #200743)

Even if lit supports it, quoting makes it easier to copy-n-paste the command.

In zsh it starts a function definition:

% echo foo()  
function>
10 ↗(On Diff #200743)

Disassembly of section .text: and the next line (empty) may be omitted.

test/tools/llvm-objdump/X86/disassemble-long-instructions.test
1 ↗(On Diff #200743)

print the disassembly a long instruction.

Should this be reworded a bit?

So we have both disasm-* and disassemble-* tests... You may rename the existing ones and rename this to disasm-* if you want to keep consistency.

test/tools/llvm-objdump/X86/elf-disassembly-no-symtab.test
1 ↗(On Diff #200743)

This patch adds

elf-disassembly-no-symtab.test
elf-disassemble-relocs.test
elf-disasm-dynamic-symbols.test

I hope the inconsistency is not intentional..

jhenderson marked 11 inline comments as done.May 23 2019, 3:57 AM

Thanks for the comments. I'll update the patch again shortly.

test/tools/llvm-objdump/X86/disasm-specific-funcs-mangled-name.test
6 ↗(On Diff #200743)

Single and double quotes both work (no quotes don't), so I'll update it.

13 ↗(On Diff #200743)

How about I get rid of the 90 and nop completely? I don't think they're needed for the test.

test/tools/llvm-objdump/X86/disasm-specific-funcs.test
15 ↗(On Diff #200743)

I also removed the actual disassembly from the test check, since I don't see a need for it.

test/tools/llvm-objdump/X86/disassemble-long-instructions.test
1 ↗(On Diff #200743)

I'll do the renaming of files that don't start with "elf-" in a follow-up patch, as we have quite a mixture already.

test/tools/llvm-objdump/X86/elf-disassembly-no-symtab.test
1 ↗(On Diff #200743)

Believe it or not, I think there was some intention behind it but I cannot remember what! I think it depended on which feature or switch I was testing in particular. But I agree that inconsistency is unhelpful and will rename them.

grimar added inline comments.May 23 2019, 4:01 AM
test/tools/llvm-objdump/X86/disasm-specific-funcs-mangled-name.test
13 ↗(On Diff #200743)

I though about removing the Content: '90', yes, but was not sure if that works.

jhenderson marked 4 inline comments as done.May 23 2019, 4:23 AM
jhenderson added inline comments.
test/tools/llvm-objdump/X86/disasm-specific-funcs-mangled-name.test
13 ↗(On Diff #200743)

I think the only bit that is important is showing the symbol label. After that, we're just testing that disassembly in general works, which is independent of the switch behaviour, I think.

Address review comments (mostly renaming of tests and simplifying disassembly checks).

jhenderson edited the summary of this revision. (Show Details)May 23 2019, 4:49 AM

I made some non-trivial changes to do with the disassembly checks, so I'd appreciate a final LGTM for someone before submitting.

grimar accepted this revision.May 23 2019, 5:03 AM

I do not see any issues. LGTM.

This revision was automatically updated to reflect the committed changes.

FYI, I did the file renaming in rL361491.

llvm/trunk/test/tools/llvm-objdump/X86/start-stop-address.test