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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin marked 12 inline comments as done.
DiggerLin added inline comments.


================
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
----------------
jhenderson wrote:
> 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.
there are two version of xcoff.o , one is 32-bit, other one is 64-bit,

the 32-bit xcoff.o has symbol var_0x1DF
the 64-bit xcoff.o has symbol var+0x1F7.

I want to check whether the only a 32-bit xcoff.o in the archive or both 32-bit and 64-bit xcoff.o in the archive, I need to check the symbol.


================
Comment at: llvm/test/tools/llvm-ar/option-X.test:178
+
+## Test move member
+# RUN: cp archive-any.a archive.a
----------------
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.


================
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| \
----------------
jhenderson wrote:
> Not sure why you ignored parts of my original edit...
sorry for the miss some of comment even I  go the comment one by one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127864