[PATCH] D118193: [llvm-nm] add a new option -X to specify the type of object file llvm-nm should examine

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-nm/option-X.test:7
+
+# RUN: llvm-nm --format=just-symbols -X32 %t32.bc %t64.bc | \
+# RUN:   FileCheck %s -DFILE1=%t32.bc --check-prefixes=BITCODE32 --implicit-check-not={{.}}
----------------
A thought: I wonder if it would be simpler for all positive inptus to be tested in a single set of llvm-nm invocations. In other words, ratehr than having near-identical bitcode, ELF, and XCOFF cases, you could have them all combined into one, e.g.
```
# RUN: llvm-nm --format=just-symbols -X32 %t32.bc %t64.bc %t32.elf %t64.elf %t_xcoff.a | \
# RUN:   FileCheck %s ...
```

Not sure how easy it would be to craft the FileCheck line, so this may not work.


================
Comment at: llvm/test/tools/llvm-nm/option-X.test:53
+  Type:    ET_REL
+  Machine: EM_386
+Sections:
----------------
The Machine key probably isn't needed for this test case. You should be able to omit it.


================
Comment at: llvm/test/tools/llvm-nm/option-X.test:57
+    Type:  SHT_PROGBITS
+    Flags: [SHF_ALLOC, SHF_WRITE]
+Symbols:
----------------
Flags aren't important for this test case. Delete them.


================
Comment at: llvm/test/tools/llvm-nm/option-X.test:59-60
+Symbols:
+  - Name:    global_data_symbol32
+    Binding: STB_GLOBAL
+    Section: .data
----------------
The default binding is Local. This is sufficient for this test case, so delete the explicit binding and rename the symbol.


================
Comment at: llvm/test/tools/llvm-nm/option-X.test:63
+
+--- !ELF
+FileHeader:
----------------
You don't need a second YAML document here. You can use `-D` YAML macros to configure the `Class` field for the specific variety you want. There are plenty of examples in other ELF tests. You don't need a unique symbol name either, though you can if you want (if you want one, I'd reuse the `Class` macro in the symbol name, so you don't need an additional macro).


================
Comment at: llvm/test/tools/llvm-nm/option-X.test:80
+# RUN: yaml2obj --docnum=4 %s -o %t_xcoff64.o
+# RUN: rm -rf %t.a
+# RUN: llvm-ar -q -c %t.a %t_xcoff32.o %t_xcoff64.o
----------------
No need for the `r` in `-rf`, since `%t.a` isn't a directory.


================
Comment at: llvm/test/tools/llvm-nm/option-X.test:125
+
+
+# RUN: not llvm-nm --format=just-symbols -X33 %t_xcoff32.o 2>&1 |  FileCheck %s --check-prefixes=ERR
----------------
Nit: no double blank line.


================
Comment at: llvm/test/tools/llvm-nm/option-X.test:130
+
+--- !XCOFF
+FileHeader:
----------------
Similar comment to the two ELF inputs. I think you can reduce duplication by using a single YAML input for XCOFF and using yaml2obj's `-D` option to customise it accordingly.


================
Comment at: llvm/test/tools/llvm-nm/option-X.test:133-138
+  Flags:             0x0002 
+AuxiliaryHeader:
+  Magic:                 0x10B
+  Version:               0x2
+  TextSectionSize:       0x280
+  DataSectionSize:       0x90
----------------
Do you actually need all these fields? Do any of them have defaults that are sufficient for our needs?


================
Comment at: llvm/test/tools/llvm-nm/option-X.test:140-143
+  - Name:            .text
+    Flags:           [ STYP_TEXT ]
+  - Name:            .data
+    Flags:           [ STYP_DATA ]
----------------
Why do you have two sections? Won't one do, like the ELF case?


================
Comment at: llvm/test/tools/llvm-nm/option-X.test:147-152
+    Type:            0x4000
+    StorageClass:    C_EXT
+    AuxEntries:
+     - Type:                   AUX_CSECT
+       SymbolAlignmentAndType: 0x09
+       StorageMappingClass:    XMC_RW
----------------
Same comment as above: are all of these required fields, or are the defaults sufficient?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118193