[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/XCOFF/option-X.test:9-15
+# RUN: llvm-nm -X32 %t32.o | FileCheck --check-prefixes=BIT32 %s --implicit-check-not={{.}}
+# BIT32:     00000000 D var32
+
+# RUN: llvm-nm -X32 %t.a | \
+# RUN:   FileCheck --check-prefixes=ARC32 %s -DFILE=%basename_t --implicit-check-not={{.}}
+# ARC32:      [[FILE]].tmp32.o:
+# ARC32-NEXT: 00000000 D var32
----------------
Possible simplification, as it removes the duplicate `00000000 D var32`.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/option-X.test:20-23
+# RUN: llvm-nm -X64 %t.a | \
+# RUN:   FileCheck --check-prefixes=ARC64 -DFILE=%basename_t %s --implicit-check-not={{.}}
+# ARC64:              [[FILE]].tmp64.o:
+# ARC64-NEXT:         0000000000000000 D var64
----------------



================
Comment at: llvm/test/tools/llvm-nm/XCOFF/option-X.test:45-48
+# ARCHIVE: [[FILE]].tmp32.o:
+# ARCHIVE-NEXT: 00000000 D var32{{[[:space:]]}}
+# ARCHIVE-NEXT: [[FILE]].tmp64.o:
+# ARCHIVE-NEXT: 0000000000000000 D var64
----------------
Indent to align.

I'd also change `ARCHIVE` to `ARCBOTH` or something equivalent, so that it is consistent with the 32-bit and 64-bit cases.


================
Comment at: llvm/test/tools/llvm-nm/option-X-elf-bitcode.test:1
+## Test the option "-X" of llvm-nm for bitcode and elf object file.
+
----------------
Nit: the file format should be all caps.

It's not clear to me why this test contains test cases for two formats (ELF and bitcode), but not all formats supported by this option. In other words, could you please explain why your XCOFF case is in a separate test file to these two (and why you've kept ELF and bitcode in the same one)?


================
Comment at: llvm/test/tools/llvm-nm/option-X-elf-bitcode.test:3-4
+
+# RUN: llvm-as -o %t32.bc %p/Inputs/bitcode-sym32.ll
+# RUN: llvm-as -o %t64.bc %p/Inputs/bitcode-sym64.ll
+
----------------
These files don't exist. Did you forget to add them?


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1762-1763
+static bool shouldDump(SymbolicFile &Obj) {
+  // The -X option only works for XCOFF object file, ELF object file, IR object file
+  // input from command line or archive files.
+  if (!isa<XCOFFObjectFile>(Obj) && !isa<ELFObjectFileBase>(Obj) &&
----------------
I think you can simplify this comment, and also clarify that the option isn't fundamentally impossible with other formats, just isn't implemented.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1764-1766
+  if (!isa<XCOFFObjectFile>(Obj) && !isa<ELFObjectFileBase>(Obj) &&
+      !isa<IRObjectFile>(Obj))
+    return true;
----------------
It's fine not to add support for COFF and Mach-O in this patch, but something to consider is what might happen if a user attempted to use that option with an object in that format. The option would be silently ignored, but it wouldn't be clear why to them. I'd consider adding a warning here, if the option has been specified, but the input object is not a supported format.

Alternatively, just add support for all supported formats up-front.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:2353
+  else
+    error("--X value should be one of: 32, 64, 32_64, (default) any");
+
----------------
Please add a  test-case for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118193