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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/test/tools/llvm-ar/option-X.test:181
+## Do not move 64-bit object without options -X64, -X32_64, Xany.
+# RUN: llvm-ar -ma elf32.o archive.a xcoff64.o 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=WARN-XCOFF64
----------------
I change elf64.o --> elf32.o , otherwise it will return "error: insertion point not found"
that is not our test purpose for the scenario.


================
Comment at: llvm/test/tools/llvm-ar/option-X.test:178
+
+## Test move member
+# RUN: cp archive-any.a archive.a
----------------
jhenderson wrote:
> 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
> ```
good catch, thanks.  I add  two more tests for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127864