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

Thu Jul 7 09:05:05 PDT 2022

DiggerLin added inline comments.

================
Comment at: llvm/test/Object/lit.local.cfg:1-7
+# Some tools support -X option and environment variable "OBJECT_MODE" in AIX OS.
+# The -X option will override the environment variable "OBJECT_MODE". if there
+# is no -X option and the environment variable "OBJECT_MODE" specific,  the default
+# behaviours is -X32. In order not to effect existing test cases which should
+# support 32 bits and 64bits by default. Add the environment variable "OBJECT_MODE"
+# to any as default for AIX OS.
+
----------------
jhenderson wrote:
> Delete the blank line between the comment and block of code it refers to.
> 
> I've also fixed a number of grammar errors, typos, etc, and removed all references to the command-line option from the comment, in the suggested edit: the code this comment is supposed to explain is about the environment variable only, and the command-line option is irrelevant to it (users reading this comment, don't need to know about a command-line option to understand why this code exists).
thanks

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:13
+
+## Test default -X option when creating a new archive.
+# RUN: llvm-ar -q -c archive-default.a xcoff32.o elf32.o xcoff64.o elf64.o
----------------
jhenderson wrote:
> You only test the default behaviour for archive creation, but you should also do it for the operations you test, I think.
the code 

```
 if (object::Archive::getDefaultKindForHost() == object::Archive::K_AIXBIG) {
    BitMode = getBitMode(getenv("OBJECT_MODE"));
    if (BitMode == BitModeTy::Unknown)
      BitMode = BitModeTy::Bit32;
  }
```

is dependent of operations , so I think we only need to test  default behavior to -X32 once at here is OK.

and I also using default behavior for the 
## Extract a 64-bit object file with option -X32(or default object mode).

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:14
+## Test default -X option when creating a new archive.
+# RUN: llvm-ar -q -c archive-default.a xcoff32.o elf32.o xcoff64.o elf64.o
+# RUN: llvm-ar -t -Xany archive-default.a | \
----------------
jhenderson wrote:
> Why have you dropped the warning checks from these cases?
I do not think we need to check the warning for each test. I tested the warning in the  line 243~246 (etc), I do not think we need to test warning in each  case.

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:126
+## Test replace/insert a object file from big archive file.
+# RUN: rm -rf *.o
+
----------------
jhenderson wrote:
> Why are you deleting objects here?
we need to delete all *.o before we test extract operation. I can delete the " rm -rf *.o" since I use --output  for the extract operation.

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:128-132
+## xcoff32.o is 64 bits object file here.
+# RUN: yaml2obj --docnum=2 -DFLAG=0x1F7 %s -o xcoff32.o
+
+## Without -X64, -X32_64 or -Xany, nothing changed here,
+## since xcoff32.o is 64 bits object file.
----------------
jhenderson wrote:
> I think you should avoid this confusing situation by using a completely fresh set of objects and an archive created specifically for this test case. You could call the objects 1.o, 2.o etc, to help show the expected order. You should be able to reuse your earlier objects by simply copying them.
here we need a same file xcoff32.o which is a 32 bit member is archive, when we want to  replace with a 64 bit xcoff32.o and without option -X32,  not change anything 

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:133
+## since xcoff32.o is 64 bits object file.
+# RUN: llvm-ar -ru archive-32.a xcoff32.o
+# RUN: llvm-ar -t -Xany archive-32.a | \
----------------
jhenderson wrote:
> I'm not sure I understand the need for the `u` option in these test cases?
yes , we do not need u here.

================
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 | \
----------------
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.

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:21-22
+
+# INFO_64: xcoff64.o is not valid with the current object file mode.
+# INFO_64: elf64.o is not valid with the current object file mode.
+
----------------
jhenderson wrote:
> jhenderson wrote:
> > Should the second of these use the -NEXT suffix?
> Ping this comment? It seems to have been ignored.
I am sorry that I do not get the comment.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127864