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

Wed Feb 2 00:48:17 PST 2022

jhenderson added a comment.

> Added a new option "-X" to specify,which type of object file should be examined.
> For example,
>
> 1. "llvm-nm -X64 archive.a" only deals with the 64-bit object files in archive.a, ignoring all 32-bit object files.

I'm not sure what this second sentence is actually saying. Please fix it.

> 2. "llvm-nm -X32 xcoffobj32.o xcoffobj64.o " only deal with the 32bit , 64bit object files input from command line

================
Comment at: llvm/docs/CommandGuide/llvm-nm.rst:131
+
+ Specify the type of XCOFF object file, ELF object file, IR object file input
+ from command line or from big archive files that llvm-nm should examine. The
----------------

================
Comment at: llvm/docs/CommandGuide/llvm-nm.rst:132
+ Specify the type of XCOFF object file, ELF object file, IR object file input
+ from command line or from big archive files that llvm-nm should examine. The
+ mode must be one of the following:
----------------
Use the proper-noun form to make it stand out that this is a specific file format and doesn't simply mean "large regular archives".

================
Comment at: llvm/docs/CommandGuide/llvm-nm.rst:135-141
+         Processes only 32-bit object files.
+   64
+         Processes only 64-bit object files.
+   32_64
+         Processes both 32-bit and 64-bit object files. 
+   any
+         Processes all of the supported object files.
----------------
In all cases: "Processes" -> "Process"

Also "all of" -> "all"

================
Comment at: llvm/test/tools/llvm-nm/XCOFF/option-X.test:1
+## Test the option "-X" of llvm-nm for xcoff object file.
+## The option specifies the type of object file llvm-nm should examine. 
----------------
This comment can be simplified, since you are a) inside the llvm-nm testsuite, and b) inside the XCOFF subdirectory.

================
Comment at: llvm/test/tools/llvm-nm/XCOFF/option-X.test:4-5
+
+# RUN: yaml2obj --docnum=1 %s -o %t32_xcoff.o
+# RUN: yaml2obj --docnum=2 %s -o %t64_xcoff.o
+# RUN: rm -rf %t.a
----------------
Here and for all references, delete `_xcoff` from the file names: we're in the XCOFF subdirectory, so there's no need to provide that disambiguation.

================
Comment at: llvm/test/tools/llvm-nm/XCOFF/option-X.test:10-12
+# RUN: llvm-nm -X32 %t.a | FileCheck --check-prefixes=BIT32,ARC32 %s
+# BIT32:     00000000 D var32
+# ARC32-NOT: 0000000000000000 D var64
----------------
You should be able to do this here too. You also will need to add an extra check for a filename, I believe (which will also help show that only the desired object is dumped). Prefer the catch-all `--implicit-check-not={{.}}` to a pattern that could go stale if the test is modified slightly

================
Comment at: llvm/test/tools/llvm-nm/XCOFF/option-X.test:14-17
+# RUN: llvm-nm -X64 %t64_xcoff.o | FileCheck --check-prefixes=BIT64 %s --implicit-check-not={{.}}
+# RUN: llvm-nm -X64 %t.a | FileCheck --check-prefixes=BIT64,ARC64 %s
+# ARC64-NOT:     00000000 D var32
+# BIT64:         0000000000000000 D var64
----------------
Same sort of comments as above.

================
Comment at: llvm/test/tools/llvm-nm/XCOFF/option-X.test:19-21
+# RUN: llvm-nm %t32_xcoff.o %t64_xcoff.o | FileCheck --check-prefixes=BOTH %s -DFILE32=%t32_xcoff.o -DFILE64=%t64_xcoff.o --match-full-lines
+# RUN: llvm-nm -X32_64 %t32_xcoff.o %t64_xcoff.o | FileCheck --check-prefixes=BOTH %s -DFILE32=%t32_xcoff.o -DFILE64=%t64_xcoff.o --match-full-lines
+# RUN: llvm-nm -Xany %t32_xcoff.o %t64_xcoff.o | FileCheck --check-prefixes=BOTH %s -DFILE32=%t32_xcoff.o -DFILE64=%t64_xcoff.o --match-full-lines
----------------
These lines are all very long. Split them over multiple lines as shown.

================
Comment at: llvm/test/tools/llvm-nm/option-X-elf-bitcode.test:30-34
+# RUN: llvm-nm --format=just-symbols -X32 %t32.bc %t64.bc | FileCheck %s -DFILE1=%t32.bc --check-prefixes=BITCODE32 --implicit-check-not={{.}}
+# RUN: llvm-nm --format=just-symbols -X64 %t32.bc %t64.bc | FileCheck %s -DFILE2=%t64.bc --check-prefixes=BITCODE64 --implicit-check-not={{.}}
+# RUN: llvm-nm --format=just-symbols %t32.bc %t64.bc | FileCheck %s -DFILE1=%t32.bc -DFILE2=%t64.bc --check-prefixes=BITCODE32,BITCODE64
+# RUN: llvm-nm --format=just-symbols -X32_64 %t32.bc %t64.bc | FileCheck %s -DFILE1=%t32.bc -DFILE2=%t64.bc --check-prefixes=BITCODE32,BITCODE64
+# RUN: llvm-nm --format=just-symbols -Xany %t32.bc %t64.bc | FileCheck %s -DFILE1=%t32.bc -DFILE2=%t64.bc --check-prefixes=BITCODE32,BITCODE64
----------------
Split all these lines into two lines each. Same below.

================
Comment at: llvm/test/tools/llvm-nm/option-X-elf-bitcode.test:6
+
+# RUN: clang -emit-llvm -m32 -o %t32.bc -c %t32.c
+# RUN: clang -emit-llvm -m64 -o %t64.bc -c %t64.c
----------------
DiggerLin wrote:
> MaskRay wrote:
> > Because of project layering, llvm tests cannot use clang. You need to use `llc` to generate the test.
> > 
> > Consider `split-file`. There are quite few tests in `llvm/test/tools/`
> thanks
You've marked as done, but you haven't used `split-file` to provide multiple inputs within the file instead of using `echo`. Please adderss this point.

================
Comment at: llvm/tools/llvm-nm/Opts.td:16

+defm bit_mode : Eq<"bit-mode", "Specifies the type of object file llvm-nm should examine. The bit-mode must be one of : 32, 64 ,any(default)">;
 def debug_syms : FF<"debug-syms", "Show all symbols, even debugger only">;
----------------
MaskRay wrote:
> Ensure the space separators are consistently used: `32, 64, (default) any` or `32, 64, any (default)`
> 
> The leading message can be "(AIX specific) Specify the type of object file to examine". Note that other messages are imperative sentences and don't use the third-person singular. The tool is called `llvm-nm`, so it is unneeded to repeat `llvm-nm` in individual option messages.
Marked as done, but not addressed?

================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1813
+static bool shouldDump(SymbolicFile &Obj) {
+  // -X option only work for XCOFF object file, ELF object file, IR object file
+  // input from command line or archive files.
----------------
Obvious question: why does it only work for those kinds of files, and not COFF or Mach-O files (for example)?

================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:2405
+  else
+    error("--X value should be one of: 32, 64,32_64, (default) any");
+
----------------

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118193