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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-nm.rst:131
+
+ Specifies the type of XCOFF object file, ELF object file, IR object file input from command
+   line or from big archive files and llvm-nm should examine. The mode must be one of the following:
----------------
Use imperative sentences like other options


================
Comment at: llvm/docs/CommandGuide/llvm-nm.rst:132
+ Specifies the type of XCOFF object file, ELF object file, IR object file input from command
+   line or from big archive files and llvm-nm should examine. The mode must be one of the following:
+   32
----------------
Align


================
Comment at: llvm/docs/CommandGuide/llvm-nm.rst:133
+   line or from big archive files and llvm-nm should examine. The mode must be one of the following:
+   32
+         Processes only 32-bit object files.
----------------
Can you run `ninja docs-llvm-html` and inspect whether the .rst renders properly?

Ensure sphinx-build exists. Configure your build with `-DLLVM_ENABLE_SPHINX=ON`.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/option-X.test:12
+# BIT32:     00000000 D var32
+# BIT32-NOT: 0000000000000000 D var64
+
----------------
If you use `--implicit-check-not={{.}}`, the `NOT` pattern should be unneeded.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/option-X.test:25
+# BOTH:        [[FILE32]]:
+# BOTH-NEXT:   00000000 D var32
+# BOTH:        [[FILE64]]:
----------------
It's worth testing the intermediate lines between the `[[FILE32]]:` stanza and the `[[FILE64]]:` stanza, to ensure the formatting is intended.

I didn't verify:

```
# BOTH:        [[FILE32]]:
# BOTH-NEXT:   00000000 D var32
# BOTH-EMPTY:
# BOTH-NEXT:   [[FILE64]]:
# BOTH-NEXT:   0000000000000000 D var64
```



================
Comment at: llvm/test/tools/llvm-nm/XCOFF/option-X.test:37
+
+# RUN: llvm-nm -X64 %t32_xcoff.o | FileCheck %s --allow-empty --implicit-check-not={{.}}
+# RUN: llvm-nm -X32 %t64_xcoff.o | FileCheck %s --allow-empty --implicit-check-not={{.}}
----------------
replace --allow-empty usage with `count 0` (there is zero line, i.e. empty)


================
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
----------------
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/`


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:87
 enum OutputFormatTy { bsd, sysv, posix, darwin, just_symbols };
+enum class BitModeTy { Bit32, Bit64, Bit32_64, Any };
 } // namespace
----------------
If Any is not that different from Bit32_64, just remove Any.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118193