Page MenuHomePhabricator

[llvm-size][test] Improve llvm-size testing
ClosedPublic

Authored by jhenderson on Aug 13 2019, 3:51 AM.

Details

Summary

This patch significantly improves the llvm-size testing. Changes I have made are:

  1. Change all ELF tests to use yaml2obj instead of assembly.
  2. Move the tests out of the X86 directory, since they don't need to be there after 1).
  3. Increased test coverage.
  4. Added comments to explain purpose of tests.

I haven't attempted to add test coverage for all Mach-O related code, as I am not familiar enough with that file format to be able to.

Diff Detail

Repository
rL LLVM

Event Timeline

jhenderson created this revision.Aug 13 2019, 3:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2019, 3:51 AM

The file renaming hasn't been reflected in my patch creation script for some reason. For reference:

  • no-input.test was originally basic.test
  • elf-berkeley.test was originally X86/elf-sizes.test
  • common.test was originally X86/test-common.s

Additionally, X86/format-berkeley-tabs.s and X86/ignore-sections.s have been folded into elf-berkeley.test and elf-sysv.test.

grimar added inline comments.Aug 13 2019, 4:37 AM
test/tools/llvm-size/archive.test
37 ↗(On Diff #214799)

archive.test.tmp1? Should it be [[FILE1]]?

test/tools/llvm-size/common.test
8 ↗(On Diff #214799)

nit: since --common is not the default, perhaps it should be SYSV + SYSVCOMMON?
Up to you.

48 ↗(On Diff #214799)

I'd use just 2 symbols of a different sizes probably.

test/tools/llvm-size/elf-m.test
34 ↗(On Diff #214799)

I am not sure you need any sections to show that the output format changed.
I.e. I guess that having an ELF yaml header should be enough here.

test/tools/llvm-size/elf-sysv.test
108 ↗(On Diff #214799)

You can probably just do: Info: 0?

test/tools/llvm-size/invalid-input.test
9 ↗(On Diff #214799)

It doesn't print the "error: " prefix, right? This is a bit strange behavior.

test/tools/llvm-size/long-format.test
4 ↗(On Diff #214799)

How much is hard/reasonable to switch to using a YAML instead of the precompeiled darwin-m.o?

test/tools/llvm-size/macho-sysv.test
10 ↗(On Diff #214799)

# CHECK-NOT:{{.}} perhaps?

test/tools/llvm-size/no-input.test
4 ↗(On Diff #214799)

I wonder if it is really important to check the tool executable name?
For other tools we often just check the "error: filename: text", I am not sure
that testing the {{.*}}llvm-size{{(\.EXE|\.exe)?}} makes a lot of sense? (looks a bit complicated and excessive)

test/tools/llvm-size/radix.test
108 ↗(On Diff #214799)

You can have a single input file for these tests here probably.

MaskRay added inline comments.Aug 13 2019, 5:18 AM
test/tools/llvm-size/archive.test
10 ↗(On Diff #214799)

| count 0

test/tools/llvm-size/no-input.test
4 ↗(On Diff #214799)

GNU size sets _bfd_error_program_name and thus prints argv[0]: filename: message.

I don't think duplicating the program name makes a lot of sense.. Though I'm not sure if you'd be happy to change the behavior.

test/tools/llvm-size/response-file.test
3 ↗(On Diff #214799)

Does this work on Windows?

jhenderson marked 8 inline comments as done.Aug 13 2019, 5:32 AM

I'll address the comments later today hopefully.

test/tools/llvm-size/archive.test
37 ↗(On Diff #214799)

No. [[FILE1]] is an absolute file path, whereas the member name is just the base file name

test/tools/llvm-size/common.test
8 ↗(On Diff #214799)

I was sticking with the existing prefixes for this test, so would prefer not to change it, though I don't feel strongly about it.

48 ↗(On Diff #214799)

Okay.

test/tools/llvm-size/invalid-input.test
9 ↗(On Diff #214799)

I agree. See the bug mentioned in the FIXME.

test/tools/llvm-size/macho-sysv.test
10 ↗(On Diff #214799)

I've got an --implicit-check-not={{.}}. We don't need both.

test/tools/llvm-size/no-input.test
4 ↗(On Diff #214799)

I don't have an issue with changing the behaviour, but not in this charge. I don't think error messages need to be consistent with GNU. I'll make a note on the error message bug.

test/tools/llvm-size/radix.test
108 ↗(On Diff #214799)

Yup, sorry!

test/tools/llvm-size/response-file.test
3 ↗(On Diff #214799)

Yes. I do my main development on Windows.

grimar added inline comments.Aug 13 2019, 5:52 AM
test/tools/llvm-size/common.test
8 ↗(On Diff #214799)

I see. It's ok to stick to the existent naming I think. I did not notice it was the original naming.

jhenderson marked 11 inline comments as done.

Addressed review comments. In particular, I have removed the pre-compiled Mach-O binaries, in favour of YAML inputs, as they're not too bad.

test/tools/llvm-size/archive.test
10 ↗(On Diff #214799)

Turns out in the -A case, this doesn't work, because a single blank line is printed in this case. The --implicit-check-not passes because FileCheck appears to ignore leading and trailing whitespace. This means that the blank line is not picked up for some reason.

test/tools/llvm-size/long-format.test
4 ↗(On Diff #214799)

It's not ideal, but it's doable. I used obj2yaml to generate the YAML, and tried (and failed) to reduce it further by hand.

test/tools/llvm-size/no-input.test
4 ↗(On Diff #214799)

I've simiplified the tool name pattern to be similar to what I've done elsewhere. I think we should test it until such point as we decide we don't want the tool name in the message.

grimar accepted this revision.Aug 14 2019, 12:38 AM

This LGTM.

This revision is now accepted and ready to land.Aug 14 2019, 12:38 AM
This revision was automatically updated to reflect the committed changes.