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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin marked 12 inline comments as done.
DiggerLin 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={{.}}
----------------
jhenderson wrote:
> 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.
I think a separate test , it is easy to understand,  and if someone want to add a new object format support support in some day, for example, MachO support, they just need to add one new test line, do not need to change current test line. 


================
Comment at: llvm/test/tools/llvm-nm/option-X.test:63
+
+--- !ELF
+FileHeader:
----------------
jhenderson wrote:
> 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).
thanks.


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


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


================
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
----------------
jhenderson wrote:
> Same comment as above: are all of these required fields, or are the defaults sufficient?
thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118193