[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
 
+   On AIX OS, the default is to process 32-bit object files only  and to ignore
+  64-bit objects. The can be changed by setting the OBJECT_MODE environment
----------------



================
Comment at: llvm/docs/CommandGuide/llvm-nm.rst:150-151
+
+   On other operating systems, the default is to process all object files.
+  The OBJECT_MODE environment variable is not supported.
+
----------------
Slight tweak from my earlier suggestion, on second reading.


================
Comment at: llvm/test/tools/llvm-nm/option-X-AIX.test:62
+
+## environment variable "OBJECT_MODE" on archive.
+# RUN: llvm-nm --format=just-symbols %t.a | \
----------------



================
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
----------------
DiggerLin wrote:
> jhenderson wrote:
> > 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.
> there is no reason to using bitcode files for the first set of tests.  I chose the bit code and XCOFF just because the format are used in AIX OS.  other object file format are not used in AIX OS. 
I guess my point is that you don't need both XCOFF and bitcode inputs. You could just have one file format for this test's purposes.


================
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
----------------
jhenderson wrote:
> 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.
I don't think you fully understood my comment. In the first half of the test (where you are testing bitcode), you have only two prefixes, one for 32-bit and one for 64-bit files. The case where you expect both to be output is then tested using --check-prefixes with both prefixes specified. However, for archives, you have a total of 5 different prefixes: `ARC32`, `ARC64`, `XCOFF32`, `XCOFF64` and `BOTH`. I think you can drop the `BOTH` case by simply specifying `--check-prefixes=ARC32,ARC64,XCOFF32,XCOFF64`.


================
Comment at: llvm/test/tools/llvm-nm/option-X-Non-AIX.test:1
+# REQUIRES: !system-aix
+
----------------
I've taken another look at this test and am wondering what the test gives us above what regular llvm-nm tests give us?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132494