[PATCH] D114434: [XCOFF] [llvm-readobj] replace binaries with YAMLs (only tests for Symbols)

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Mostly looks good, but I think we can simplify the test a little whilst we're here, as suggested in a couple of places inline.

My comments in the two tests apply to both tests, so please address in both cases/respond for both cases.



================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/symbols-invalid.test:4-5
+
+# RUN: yaml2obj %s -o %t5
+# RUN: llvm-readobj --syms %t5 | FileCheck %s
+
----------------
I'm not necessarily opposed to this being a separate file, although I think it's more common in newer tests to keep invalid behaviour checks in the same files as their corresponding regular behaviour.

Also the `%t5` needs changing to just `%t`.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/symbols.test:30
+        FileStringType:   XFT_CV
+## The C_STAT symbol with a SECT auxiliary entry.
+  - Name:               .text
----------------
Should this be `CSECT` rather than `SECT`?


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/symbols.test:90
+
+# SYMBOL32:      AddressSize: 32bit
+# SYMBOL32-NEXT: Symbols [
----------------
The addition of the `#` characters has made this a little hard to review. Are there any actual changes in the checks performed, or is the output semantically identical?


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/symbols64.test:88
+# SYMBOL64-NEXT:       Index: 1
+# SYMBOL64-NEXT:       Name: test8.c
+# SYMBOL64-NEXT:       Type: XFT_FN (0x0)
----------------
The `8` sounds important. I'd change to just `test.c`


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/symbols64.test:94
+# SYMBOL64-NEXT:       Index: 2
+# SYMBOL64-NEXT:       Name: Sun Apr 28 15:56:49 2019
+# SYMBOL64-NEXT:       Type: XFT_CT (0x1)
----------------
shchenz wrote:
> Maybe use a meaningful date here?
More preceisely, as this is just a free string in the YAML, let's just change it to something more obviously arbitrary e.g. "foo"


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/symbols64.test:100
+# SYMBOL64-NEXT:       Index: 3
+# SYMBOL64-NEXT:       Name: IBM XL C for AIX, Version 16.1.0.2
+# SYMBOL64-NEXT:       Type: XFT_CV (0x2)
----------------
shchenz wrote:
> We don't need to mention IBM XLC V16.1 now. Please use IBM Open XL instead.
Does the test actually need this many auxiliary entries to exercise the llvm-readobj behaviour appropriately? If so, as above, just use an arbitrary string here e.g. "bar".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114434