[PATCH] D132494: [AIX] llvm-nm support environment "OBJECT_MODE" for option -X on AIX OS

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-nm.rst:144-150
+   In AIX OS, the default is to process 32-bit object files (ignore 64-bit objects). The mode can also
+  be set with the OBJECT_MODE environment variable. For example, OBJECT_MODE=64 causes ar to
+  process any 64-bit objects and ignore 32-bit objects. The -X flag overrides the OBJECT_MODE
+  variable.
+
+   In no AIX OS, default is to process all the supported object files. And do not support the
+  OBJECT_MODE environment variable.
----------------
Some suggested rewording.

Please reflow all lines to an 80 character width.


================
Comment at: llvm/test/tools/llvm-nm/option-X-AIX.test:22
+
+## Test -X option override environment variable "OBJECT_MODE".
+# RUN: env OBJECT_MODE=any llvm-nm -X32 --format=just-symbols %t32.bc %t64.bc | \
----------------



================
Comment at: llvm/test/tools/llvm-nm/option-X-AIX.test:27-29
+# RUN: env OBJECT_MODE=any llvm-nm -X32_64 --format=just-symbols %t32.bc %t64.bc | \
+# RUN:   FileCheck %s -DFILE1=%t32.bc -DFILE2=%t64.bc --check-prefixes=BITCODE32,BITCODE64
+# RUN: env OBJECT_MODE=any llvm-nm -Xany --format=just-symbols %t32.bc %t64.bc | \
----------------
You should specify a different `OBJECT_MODE` value here instead of `any`, since otherwise you aren't actually showing the overriding behaviour. Perhaps use `32` for these two cases?


================
Comment at: llvm/test/tools/llvm-nm/option-X-AIX.test:42-43
+
+# RUN: yaml2obj -DFLAG=0x1DF %s -o %t_xcoff32.o
+# RUN: yaml2obj -DFLAG=0x1F7 %s -o %t_xcoff64.o
+# RUN: rm -rf %t.a
----------------
Is there a reason you're using bitcode files for the first set of tests and XCOFF files here? It seems like you could just use one set of files for all the testing.


================
Comment at: llvm/test/tools/llvm-nm/option-X-AIX.test:47
+
+# RUN: llvm-nm --format=just-symbols %t_xcoff32.o | \
+# RUN:   FileCheck --check-prefixes=XCOFF32 %s --implicit-check-not={{.}}
----------------
Like you have above, please add comments describing the test cases.


================
Comment at: llvm/test/tools/llvm-nm/option-X-AIX.test:67-70
+# RUN: env OBJECT_MODE=32_64 llvm-nm --format=just-symbols %t_xcoff32.o %t_xcoff64.o | \
+# RUN:   FileCheck --check-prefixes=BOTH %s -DFILE32=%t_xcoff32.o -DFILE64=%t_xcoff64.o --match-full-lines
+# RUN: env OBJECT_MODE=any llvm-nm --format=just-symbols %t_xcoff32.o %t_xcoff64.o | \
+# RUN:   FileCheck --check-prefixes=BOTH %s -DFILE32=%t_xcoff32.o -DFILE64=%t_xcoff64.o --match-full-lines
----------------
In the first half of your test, with the bitcode files and no archive, you share the same set of prefixes for your 32 + 64 cases with your "both" cases. I don't see why this half of your test should be any different. I think the first half of the test flows quite smoothly, so I suggest reorganising this half accordingly.


================
Comment at: llvm/test/tools/llvm-nm/option-X-Non-AIX.test:3
+
+## Test the default "-X" option in non AIX OS.
+
----------------



================
Comment at: llvm/test/tools/llvm-nm/option-X-Non-AIX.test:24-25
+
+# RUN: llvm-nm --format=just-symbols %t_elf32.o %t_elf64.o | \
+# RUN:   FileCheck %s -DFILE32=%t_elf32.o -DFILE64=%t_elf64.o --check-prefixes=ELF32,ELF64
+
----------------
There's no need to have the ELF, bitcode, XCOFF and archive cases in separate llvm-nm invocations - they can all be one llvm-nm call.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:2380
+  if (HostTriple.isOSAIX()) {
+    auto getBitMode = [](const char *RawBitMode) {
+      return StringSwitch<BitModeTy>(RawBitMode)
----------------
The lambda doesn't add anything, since it's only called once.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132494