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

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 30 00:38:59 PDT 2022


jhenderson 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.
+
----------------
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).


================
Comment at: llvm/test/Object/lit.local.cfg:1-2
+if 'system-aix' in config.available_features:
+    config.environment['OBJECT_MODE'] = 'any'
----------------
jhenderson wrote:
> I'd add a comment explaining this variable briefly.
> 
> I also think it would make sense to move this into the top-level lit config, rather than a subdirectory, because it impacts all uses of llvm-ar. llvm-ar is commonly used to create inputs for tests at runtime, and these could appear in virtually any directory within llvm/test (not to mention possibly even other subprojects, such as lld, but I'm less concerned there).
> I also think it would make sense to move this into the top-level lit config, rather than a subdirectory, because it impacts all uses of llvm-ar. llvm-ar is commonly used to create inputs for tests at runtime, and these could appear in virtually any directory within llvm/test (not to mention possibly even other subprojects, such as lld, but I'm less concerned there).

You've ignored this. Please stop ignoring parts of my comments without explaining why at least.


================
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
----------------
You only test the default behaviour for archive creation, but you should also do it for the operations you test, I think.


================
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 | \
----------------
Why have you dropped the warning checks from these cases?


================
Comment at: llvm/test/tools/llvm-ar/option-X.test:56-57
+
+# RUN: env OBJECT_MODE=any llvm-ar -q -c archive-envany.a xcoff32.o elf32.o xcoff64.o elf64.o
+# RUN: llvm-ar -t -Xany archive-envany.a | \
+# RUN:   FileCheck %s --check-prefixes=OBJ32_64
----------------
Nit: In the -X cases, the "any" case comes before the 32_64 case. I don't mind the order, but it should be the same in all cases, for ease of review and maintenance.


================
Comment at: llvm/test/tools/llvm-ar/option-X.test:71
+## Test -X option for extract operation.
+# RUN: rm -rf *.o
+# RUN: llvm-ar -x -X32 archive-any.a
----------------
Rather than repeatedly delete the objects, I think you should run each of these cases in a different directory (use mkdir and cd as needed), or use the new --output option added very recently (see rGbf223e43fe68) to specify a different directory each time.


================
Comment at: llvm/test/tools/llvm-ar/option-X.test:73
+# RUN: llvm-ar -x -X32 archive-any.a
+# RUN: test -f xcoff32.o
+# RUN: test -f elf32.o
----------------
The more common way to test for existence of a file is `ls xcoff32.o`. That being said, if you keep the original objects as described above, you could use `cmp xcoff32.o ../xcoff32.o` (or equivalent) instead for the cases where the file exists (you'd still want `not ls` for the missing cases).


================
Comment at: llvm/test/tools/llvm-ar/option-X.test:99
+
+## Extract a 64bit object file with option -X32
+# RUN: rm -rf *.o
----------------
You should also test the 32-bit object using -X64 case.


================
Comment at: llvm/test/tools/llvm-ar/option-X.test:109
+
+## Test deleting a object file from big archive file.
+# RUN: cp archive-any.a archive.a
----------------



================
Comment at: llvm/test/tools/llvm-ar/option-X.test:125
+ 
+## Test replace/insert a object file from big archive file.
+# RUN: rm -rf *.o
----------------



================
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
+
----------------
Why are you deleting objects here?


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


================
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 | \
----------------
I'm not sure I understand the need for the `u` option in these test cases?


================
Comment at: llvm/test/tools/llvm-ar/option-X.test:139-141
+## With option -X64, -X32_64 or -Xany, 
+## the old 32bit xcoff32.o is still in the archive 
+## and a new 64bits object file xcoff32.o is added into the archive. 
----------------



================
Comment at: llvm/test/tools/llvm-ar/option-X.test:148
+
+## Test move mumber
+# RUN: cp archive-any.a archive.a
----------------



================
Comment at: llvm/test/tools/llvm-ar/option-X.test:150
+# RUN: cp archive-any.a archive.a
+## Not move 64bits object without -X64
+# RUN: not llvm-ar -ma elf64.o archive.a xcoff64.o 2>&1| \
----------------



================
Comment at: llvm/test/tools/llvm-ar/option-X.test:152
+# RUN: not llvm-ar -ma elf64.o archive.a xcoff64.o 2>&1| \
+# RUN:   FileCheck %s --check-prefixes=MOVE_ERR
+
----------------
Prefer `-` to `_` in check prefixes.


================
Comment at: llvm/test/tools/llvm-ar/option-X.test:154
+
+# MOVE_ERR: llvm-ar: error: xcoff64.o: No such file or directory
+
----------------
This should use the `%errc_ENOENT` lit substitution.


================
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 | \
----------------
You need 32-bit equivalents of these test cases, and also to show the behaviour with the other -X options.


================
Comment at: llvm/test/tools/llvm-ar/option-X.test:191-192
+
+# RUN: llvm-as -o t32.bc %p/Inputs/bitcode-sym32.ll
+# RUN: llvm-as -o t64.bc %p/Inputs/bitcode-sym64.ll
+# RUN: yaml2obj --docnum=3 %s -o macho32.o
----------------
Why not just 32.bc and 64.bc?


================
Comment at: llvm/test/tools/llvm-ar/option-X.test:200
+
+## Test -X option for object file bit code,mach-O,coff,wasm
+
----------------
DiggerLin wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > I think you're trying to say "Test -X option with other output formats." right? You don't need to list them out specifically (the test case is clear enough).
> > > 
> > > Is there a reason you've got ELF in the earlier test cases, rather than down here? I think it would make the earlier tests simpler to use just one file format in the earlier test cases (probably XCOFF) and then just this simple test for the other ones (including ELF).
> > > 
> > > If you go down this route, make sure you don't lose test coverage that shows that llvm-ar warns for every object it doesn't process, not just the first one. This could be achieved by checking the warnings in this test case, I believe.
> > > Is there a reason you've got ELF in the earlier test cases, rather than down here? I think it would make the earlier tests simpler to use just one file format in the earlier test cases (probably XCOFF) and then just this simple test for the other ones (including ELF).
> > 
> > there is no special reason to got the  ELF in the earlier test cases, but it is no harm to ELF in the earlier test case. And if only letting the earlier test cases only test  xcoff32.o and xcoff64.o, I think it is too simple, so I put the earlier  test cases test on the xcoff32.o elf32.o xcoff64.o elf64.o
> > 
> > 
> > ```
> > If you go down this route, make sure you don't lose test coverage that shows that llvm-ar warns for every object it doesn't process, not just the first one. This could be achieved by checking the warnings in this test case, I believe
> > ```
> > when we checking the warning functionality of  "is not valid with the current object file mode" , I think we only need to check once, we do not need to check everywhere ? is it correct?
> And if only letting the earlier test cases only test xcoff32.o and xcoff64.o, I think it is too simple to test especially when testing move operation.
Okay.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:811
+  if (!isValidInBitMode(NM)) {
+    warn(FileName + " is not valid with the current object file mode");
+    return;
----------------
You should quote FileName, so that it is clear if there's a space in the name.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:926-930
+          // If a new member is not a valid object for the bit mode, add the old
+          // member back.
+          WithColor::warning()
+              << *MemberI
+              << " is not valid with the current object file mode\n";
----------------
jhenderson wrote:
> We should probably move this code into a separate function, so that the message is not repeated in three different cases. That function would then call the proposed generic print warning function suggested above.
This bit hasn't been addressed. I'm suggesting a new function like this:
```
static void warnInvalidObjectForFileMode(Twine Name) {
  warn(Name + " is not valid with the current object file mode");
}
```

To avoid repeating that string in multiple places.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127864



More information about the llvm-commits mailing list