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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/lit.cfg.py:482-488
+# Some tools support an environment variable "OBJECT_MODE" on AIX OS, which
+# controls the kind of objects they will support. If there is no "OBJECT_MODE" environment
+# variable specified, 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
+# "OBJECT_MODE" to 'any' by default on AIX OS.
+
----------------
Delete the blank line at the end of this comment. Also, some of the line wrapping is a bit off, so I recommend reflowing it.


================
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
----------------
jhenderson wrote:
> Nit: there's a double space here.
Not addressed.


================
Comment at: llvm/test/tools/llvm-ar/option-X.test:178
+
+## Test move member
+# RUN: cp archive-any.a archive.a
----------------
DiggerLin wrote:
> jhenderson wrote:
> > 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).
> in original,  the test scenario look like 
> 
>  # RUN: llvm-ar -ma elf64.o archive.a xcoff64.o 2>&1 | \
>  # RUN:   FileCheck %s --check-prefix=MOVE_ERR
>  # MOVE_ERR: llvm-ar: error: xcoff64.o: No such file or directory
> 
> it because we run "rm -rf *.o" before the test scenario in original.
> so when run llvm-ar -ma elf64.o archive.a xcoff64.o ,it will return 
> "xcoff64.o: No such file or directory" 
> 
> but current we do not run  "rm -rf *.o" before the  
>   #RUN: llvm-ar -ma elf64.o archive.a xcoff64.o
> there is xcoff64.o in the directory.
> it return warning: 'xcoff64.o' is not valid with the current object file mode
> 
> I think we only to test the -X option work with -ma , so we need to  xcoff64.o in the directory.
> 
> and from the logic for the llvm-ar.cpp it will check object file in the command line whether it exist first if we want to move the object file.  it is logic is not new in the patch, so I do not think we need to test it.
Thanks for the explanation - I didn't realise that the missing file was the one being added, which is indeed not something to do with this patch.

What happens if you try to move after a file with a bitness that doesn't match the object mode? That probably needs testing. I think there are two interesting cases: a) if there is only one object with the relevant name, and b) if there are two objects with the same name, but different bitness, where only the second one matches the object mode. The second case is unnecessary, if the first case's behaviour is that the object is moved to the right place anyway, even though the referenced object doesn't have the right object mode.

Outline example - update names of files as appropriate etc:
```
# Case 1:
llvm-ar rc -Xany archive.a xcoff64.o elf32.o elf64.o
llvm-ar ma -X64 elf32.o archive.a xcoff64.o

# Case 2, if case 1 is "elf32.o is treated as not present". First elf.o is 32-bits, second is 64.
llvm-ar rc -Xany archive.a xcoff64.o elf.o elf.o
llvm-ar ma -X64 elf.o archive.a xcoff64.o
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127864