[PATCH] D127864: [llvm-ar] Add object mode option -X for AIX

Mon Jul 11 01:13:56 PDT 2022

jhenderson added inline comments.

================
Comment at: llvm/test/lit.local.cfg:1
+# Some tools support -an environment variable "OBJECT_MODE" on AIX OS, which
+# controls the kind of objects they will support. If there is no environment
----------------
I guess I should have been more specific - top-level lit configuration goes in llvm/test/lit.cfg.py, not in a lit.local.cfg in the top level.

================
Comment at: llvm/test/lit.local.cfg:2-5
+# controls the kind of objects they will support. If there is no environment
+# variable "OBJECT_MODE" specific, the default behaviour is to support 32-bit
+# objects only. In order to not affect most test cases which expect to support
+# 32-bit and 64-bit objects by default, set the environment variable
----------------

================
Comment at: llvm/test/lit.local.cfg:7
+# "OBJECT_MODE" to 'any' by default on AIX OS.
+
+if 'system-aix' in config.available_features:
----------------
Delete this blank line (as requested before - the logic is still the same).

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:77
+## Test -X option for print operation.
+# RUN: llvm-ar -t  archive-any.a | \
+# RUN:   FileCheck %s --check-prefixes=OBJ32
----------------
Nit: there's a double space here.

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:94-95
+# RUN: llvm-ar --output=32 -x -X32 archive-any.a
+# RUN: cmp -s xcoff32.o 32/xcoff32.o
+# RUN: cmp -s elf32.o 32/elf32.o
+# RUN: not ls 32/coff64.o
----------------
I'm not sure why you've added the `-s` option to the `cmp` commands. We want the output, so that if the test fails, we'll get told where the problem is in the test log.

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:117
+
+## Extract a 64-bit object file with option -X32(or default object mode).
+# RUN: not llvm-ar --output=err64 -x archive-any.a xcoff64.o 2>&1 | \
----------------

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:126
+
+## Extract a 32-bit object file with option -X64
+# RUN: not llvm-ar --output=err32 -x -X64 archive-any.a xcoff32.o 2>&1 | \
----------------

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:153
+
+## xcoff.o is 64-bit object file here.
+# RUN: yaml2obj --docnum=2 -DFLAG=0x1F7 %s -o xcoff.o
----------------

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:157
+## Without -X64, -X32_64 or -Xany, nothing changed here,
+## since xcoff.o is 64-bit object file in command line, but
+## the xcoff.o member in archive-rep.a is a 32-bit object file.
----------------
I already suggested this fix once...

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:162
+# RUN:   FileCheck %s --check-prefixes=REP
+# RUN: llvm-nm -Xany --format=just-symbols archive-rep.a | \
+# RUN:   FileCheck %s --check-prefixes=SYM32
----------------
This is the canonical option for dumping the archive symbol table. If that's not what you meant to do, could you explain what you are trying to check with this line, please?

Same below.

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:178
+
+## Test move member
+# RUN: cp archive-any.a archive.a
----------------
Comments end with a ".".

Previously you had a test case that showed the if you tried to perform an operation on an object which didn't match the -X option, you'd get a "file not found" error. Whilst I accept that it probably follows the same code paths as other cases already tested, I think it's an important enough behaviour that you should have a specific test case for it somewhere (you don't need to test both 32 and 64 bit variations for that case though, I think).

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:180
+# RUN: cp archive-any.a archive.a
+## Not move 64-bit object without options -X64, -X32_64, Xany
+# RUN: llvm-ar -ma elf64.o archive.a xcoff64.o 2>&1| \
----------------
Not sure why you ignored parts of my original edit...

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:181
+## Not move 64-bit object without options -X64, -X32_64, Xany
+# RUN: llvm-ar -ma elf64.o archive.a xcoff64.o 2>&1| \
+# RUN:   FileCheck %s --check-prefix=WARN-XCOFF64
----------------
Ditto below.

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:187
+
+## Not move 32-bit object with -X64
+# RUN: llvm-ar -ma -X64 elf32.o archive.a xcoff32.o 2>&1| \
----------------

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:159
+
+# RUN: llvm-ar -ma -X64 elf64.o archive.a xcoff64.o
+# RUN: llvm-ar -t -Xany archive.a | \
----------------
DiggerLin wrote:
> jhenderson wrote:
> > You need 32-bit equivalents of these test cases, and also to show the behaviour with the other -X options.
> I added a -X32 of the test case , but I do not think I need to add additional -X32_64 -Xany for the "-ma" ,since we use the same 
> function 
> ```
> static bool isValidInBitMode(Binary &Bin) 
> ```
> for operation,
> I have test -X32-64 and -Xany in other  operation case.
If the code changed to use a different function for the move case, then there would not be test coverage for it, hence why you should test all four cases, I think.

================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:711
+static void warnInvalidObjectForFileMode(Twine Name) {
+  warn(Name + " is not valid with the current object file mode");
+}
----------------
`Name` still isn't quoted in the output. Please fix (and don't mark comments as "Done" if you haven't done them).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127864