[PATCH] D127864: [llvm-ar] Add object mode option -X for AIX
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 21 01:56:41 PDT 2022
jhenderson added a subscriber: gbreynoo.
jhenderson added a comment.
I'm going to leave my review there, but haven't finished the test. Please address my comments and then I'll finish reviewing the test case.
================
Comment at: llvm/test/Object/X86/archive-symbol-table.s:1
+# RUN: export "OBJECT_MODE=any"
# RUN: llvm-mc %s -o %t.o -filetype=obj -triple=x86_64-pc-linux
----------------
I don't like the need for this in every single llvm-ar test. I would prefer it if this variable is set by default in the lit harness, and then just explicitly unset in the test(s) where we want to test the AIX behaviour.
================
Comment at: llvm/test/tools/llvm-ar/invalid-option-X.test:2
+# REQUIRES: !system-aix
+## Test not support "-X" option in no AIX OS
+
----------------
"Test that the -X option is not supported on non-AIX OS."
================
Comment at: llvm/test/tools/llvm-ar/invalid-option-X.test:5
+# RUN: not llvm-ar -q -X32 %t.a xcoff32.o 2>&1 | \
+# RUN: FileCheck %s --check-prefixes=ERR
+# ERR: error: -X32 option not supported on non AIX OS
----------------
No need for a check prefix other than the default CHECK when you only have one test case in the file.
================
Comment at: llvm/test/tools/llvm-ar/invalid-option-X.test:6
+# RUN: FileCheck %s --check-prefixes=ERR
+# ERR: error: -X32 option not supported on non AIX OS
----------------
Nit: there's a trailing space at the end of this line. Please delete it.
================
Comment at: llvm/test/tools/llvm-ar/option-X.test:3
+## Test the "-X" option.
+## The option specifies the type of object file to be operate in llvm-ar.
+
----------------
================
Comment at: llvm/test/tools/llvm-ar/option-X.test:5-7
+# RUN: rm -rf %t
+# RUN: mkdir %t
+# RUN: cd %t
----------------
================
Comment at: llvm/test/tools/llvm-ar/option-X.test:14
+
+## Test -X option for the create a new archive operation
+# RUN: rm -f %t.a
----------------
That being said, this first test case doesn't test the new option at all. It just uses the default behaviour.
================
Comment at: llvm/test/tools/llvm-ar/option-X.test:15
+## Test -X option for the create a new archive operation
+# RUN: rm -f %t.a
+# RUN: llvm-ar -q -c %t.a xcoff32.o elf32.o xcoff64.o elf64.o 2>&1 | \
----------------
No need for this: you haven't created the file yet, and you deleted the directory you are working in.
================
Comment at: llvm/test/tools/llvm-ar/option-X.test:16
+# RUN: rm -f %t.a
+# RUN: llvm-ar -q -c %t.a xcoff32.o elf32.o xcoff64.o elf64.o 2>&1 | \
+# RUN: FileCheck %s --check-prefixes=INFO_64
----------------
No need for `%t` when you cd'ed into a directory - just use a name specific to the test case, in this case something like `default-create.a` (i.e. it's the default behaviour for creating a new archive). Same sort of thing goes for the other test cases too.
Your new code checks for 32 versus 64 for each of a number of different formats, not just XCOFF and ELF. You should test each of them in at least one test case.
================
Comment at: llvm/test/tools/llvm-ar/option-X.test:17-19
+# RUN: FileCheck %s --check-prefixes=INFO_64
+# RUN: llvm-ar -t -Xany %t.a | \
+# RUN: FileCheck %s --check-prefixes=OBJ32
----------------
I'd also recommend using INFO-64 i.e. a hyphen instead of an underscore.
Same comments and edits apply throughout.
================
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.
+
----------------
Should the second of these use the -NEXT suffix?
================
Comment at: llvm/test/tools/llvm-ar/option-X.test:30
+# RUN: rm -f %t.a
+# RUN: export "OBJECT_MODE=64"
+# RUN: llvm-ar -q -c -X32 %t.a xcoff32.o elf32.o xcoff64.o elf64.o
----------------
Add comments explaining what each test case is doing. E.g. here it is saying that the -X option overrides the environment variable.
Additionally, the norm for setting environment variables is to use the `env` command at the start of the RUN line you want to modify, e.g.
```
# RUN: env OBJECT_MODE=64 llvm-ar ...
```
================
Comment at: llvm/test/tools/llvm-ar/option-X.test:32
+# RUN: llvm-ar -q -c -X32 %t.a xcoff32.o elf32.o xcoff64.o elf64.o
+# unset OBJECT_MODE
+# RUN: llvm-ar -t -Xany %t.a | \
----------------
This line isn't a RUN line so won't do anything, but regardless, see above.
================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:633
+ return MachO->is64Bit();
+ return cast<ELFObjectFileBase>(Obj).getBytesInAddress() == 8;
+}
----------------
There's a real risk that if a new object file format is added that this will result in an assertion/crash. It's probably better to do a dyn_cast for ELFObjectFileBase, like the other formats and then report an error (e.g. "unsupported file format" if it is none of the known formats.
================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:666-667
+ Expected<std::unique_ptr<Binary>> BinOrErr = getAsBinary(Member);
+ // In AIX "ar", If there is member which is not a object file, it also always
+ // be consider as valid bit mode.
+ if (!BinOrErr) {
----------------
I think this phrasing is a little cleaner.
================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:689
+ // Check whether the object mode is permit to output.
+ if (!isValidInBitMode(C))
----------------
================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:699
continue;
+
if (CountParam && ++MemberCount[Name] != CountParam)
----------------
Delete this unrelated change.
================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:802
+ if (!isValidInBitMode(NM)) {
+ outs() << FileName << " is not valid with the current object file mode.\n";
+ return;
----------------
This should probably be printed as a warning (and follow the coding standards for warnings and errors).
Same goes with the other areas reporting this kind of message.
================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:915
+ case IA_AddNewMember: {
+ NewArchiveMember NM = getArchiveMember(*MemberI);
+ if (isValidInBitMode(NM))
----------------
@gbreynoo, can you take a look at this area of the change in particular, please, and see if it is the best we can do given how llvm-ar is structured? I'm very rusty on the code logic, so don't feel comfortable saying whether this is the best approach or coming up with a better one.
================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:919
+ else {
+ // If a new member is not a valid object mode, add old member back.
+ outs() << *MemberI
----------------
================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1338-1339
+ if (strncmp(*ArgIt, "-X", 2) == 0) {
+ if (object::Archive::getDefaultKindForHost() ==
+ object::Archive::K_AIXBIG) {
+ Match = *(*ArgIt + 2) != '\0' ? *ArgIt + 2 : *(++ArgIt);
----------------
DiggerLin wrote:
> jhenderson wrote:
> > What's the motivation for making this AIX OS only?
> I think to make -X for all the non AIX OS before, but in the AIX, The default is to process 32-bit object files (ignore 64-bit objects). but for other non AIX OS, the default is to process both 32 and 64 bits object file. it maybe caused problem for the script which llvm-ar.
>
> if you think it is better to let -X work for non AIX OS, I can make
> default for AIX is 32bit, for non AIX OS, the default is any.
Having thought about it, I think -X should be AIX only, as it's less likely to cause a problem with another short option appearing in GNU ar and causing a name clash.
================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1343
+ if (BitMode == BitModeTy::Unknown)
+ fail(std::string("Invalid Bit Mode: ") + Match);
+ continue;
----------------
jhenderson wrote:
> This message does not conform to the LLVM coding standard for error messages. Please fix it.
Use `Twine` rather than `std::string` here to perform the concatenation. Convert to `std::string` only once the whole thing is concatenated (if needed).
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