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

Mon Jun 27 00:34:53 PDT 2022

jhenderson added inline comments.

================
Comment at: llvm/test/Object/lit.local.cfg:1-2
+if 'system-aix' in config.available_features:
+    config.environment['OBJECT_MODE'] = 'any'
----------------
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).

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:23
+## Test -X32 option when creating a new archive. 
+# RUN: rm -f archive32.a
+# RUN: llvm-ar -q -c -X 32 archive32.a xcoff32.o elf32.o xcoff64.o elf64.o 2>&1
----------------
I said this once already - don't delete archives that don't exist! The directory was deleted and recreated at the start of the test. Use a unique name for each test case too, e.g. don't reuse `archive.a` in multiple test cases, instead use `create-default.a`, `create-32.a`, `create-64.a` etc (with names that describe the test case). This way you a) don't have to rename the test, and b) can inspect the archive for each test case after running it without having to do strange things like delete parts of the test.

This applies throughout the test.

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:24
+# RUN: rm -f archive32.a
+# RUN: llvm-ar -q -c -X 32 archive32.a xcoff32.o elf32.o xcoff64.o elf64.o 2>&1
+# RUN: llvm-ar -t -Xany archive32.a | \
----------------
Did you mean to run `FileCheck` on this line? The `2>&1` implies you either a copy-paste error or that you meant to check for a warning.

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:28
+
+## Test -X option will override the envionment "OBJECT_MODE"
+# RUN: rm -f archive.a
----------------

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:37
+# RUN: llvm-ar -q -c -X 64 archive.a xcoff32.o elf32.o xcoff64.o elf64.o 2>&1 | \
+# RUN:  FileCheck %s  --check-prefixes=WARN-32
+# RUN: llvm-ar -t -Xany archive.a | \
----------------
You've still got a double space between `%s` and the `--check-prefixes` option here. Please remove it from here and all other cases.

In general, when I make a comment, please make sure you've addressed all occurrences of the issue I raised, rather than just the one, as it will speed up the review and save us both time.

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:179
+
+#OBJ32:          xcoff32.o 
+#OBJ32-NEXT:     elf32.o
----------------
Please remember to include a space between your `#` and the check directive.

================
Comment at: llvm/test/tools/llvm-ar/option-X.test:200
+
+## Test -X option for object file bit code,mach-O,coff,wasm
+
----------------
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.

================
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:
> Should the second of these use the -NEXT suffix?
Ping this comment? It seems to have been ignored.

================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:670-671
+    return true;
+  LLVMContext Context;
+  Context.setOpaquePointers(true);
+  Expected<std::unique_ptr<Binary>> BinOrErr = getAsBinary(Member, &Context);
----------------
I'm not sure I follow the need for this bit of code? Has something else been rebased that requires it, and if so, could you explain why it's correct to use it the way you've done (i.e. with the `setOpaquePointers(true)`, please.

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

================
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;
----------------
jhenderson wrote:
> 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.
llvm-ar.cpp already prints one other warning in this file (see https://github.com/llvm/llvm-project/blob/ca2933f3f88a09eb485146eef776b7345e3d87d3/llvm/tools/llvm-ar/llvm-ar.cpp#L1021). To ensure consistency, we should factor all the call sites into a function that prints the warning. Note that the other case includes the toolname, for example, which this doesn't.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127864