[PATCH] D66134: [llvm-size][test] Improve llvm-size testing

[James Henderson via Phabricator via llvm-commits]

jhenderson marked 8 inline comments as done.
jhenderson added a comment.

I'll address the comments later today hopefully.



================
Comment at: test/tools/llvm-size/archive.test:37
+# BERKELEY-3-NEXT:   1       2       4       7       7 [[FILE1]]
+# BERKELEY-3-NEXT:   1       2       4       7       7 archive.test.tmp1 (ex [[ARCHIVE2]])
+# BERKELEY-3-NEXT:   8      16      32      56      38 [[FILE2]]
----------------
grimar wrote:
> `archive.test.tmp1`? Should it be `[[FILE1]]`?
No. [[FILE1]] is an absolute file path, whereas the member name is just the base file name


================
Comment at: test/tools/llvm-size/common.test:8
+# RUN: llvm-size -A %t.o | FileCheck --check-prefix=SYSVNOCOMM %s
+# RUN: llvm-size -B %t.o | FileCheck --check-prefix=BSDNOCOMM %s
 
----------------
grimar wrote:
> nit: since `--common` is not the default, perhaps it should be `SYSV` + `SYSVCOMMON`?
> Up to you.
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.


================
Comment at: test/tools/llvm-size/common.test:48
+    Size:  4
+    Index: SHN_COMMON
----------------
grimar wrote:
> I'd use just 2 symbols of a different sizes probably.
Okay.


================
Comment at: test/tools/llvm-size/invalid-input.test:9
+# RUN: not llvm-size %s 2>&1 | FileCheck %s -DFILE=%s --check-prefix=UNRECOGNIZED
+# UNRECOGNIZED: {{.*}}llvm-size{{(.*)}}: [[FILE]] The file was not recognized as a valid object file
+
----------------
grimar wrote:
> It doesn't print the "error: " prefix, right? This is a bit strange behavior.
I agree. See the bug mentioned in the FIXME.


================
Comment at: test/tools/llvm-size/macho-sysv.test:10
+# CHECK-NEXT: __data         4      4
+# CHECK-NEXT: Total          8
----------------
grimar wrote:
> `# CHECK-NOT:{{.}}` perhaps?
I've got an `--implicit-check-not={{.}}`. We don't need both.


================
Comment at: test/tools/llvm-size/no-input.test:4
+# RUN: not llvm-size %t.blah 2>&1 | FileCheck %s -DFILE=%t.blah --check-prefix=ENOENT
+# ENOENT: {{.*}}llvm-size{{(\.EXE|\.exe)?}}: [[FILE]] {{[Nn]}}o such file or directory
+
----------------
MaskRay wrote:
> grimar wrote:
> > 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)
> 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.
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.


================
Comment at: test/tools/llvm-size/radix.test:108
+## Case 9: Bad values.
+# RUN: not llvm-size %t1.o %t2.o --radix=0 2>&1 \
+# RUN:   | FileCheck %s --check-prefix=BAD-VAL -DNUM=0
----------------
grimar wrote:
> You can have a single input file for these tests here probably.
Yup, sorry!


================
Comment at: test/tools/llvm-size/response-file.test:3
+
+# RUN: echo "-t %t" > %t.rsp
+# RUN: yaml2obj %s -o %t
----------------
MaskRay wrote:
> Does this work on Windows?
Yes. I do my main development on Windows.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66134/new/

https://reviews.llvm.org/D66134