[PATCH] D106643: [llvm-readobj][XCOFF] Add support for `--needed-libs` option.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

You'll need an XCOFF developer to review the object file code.



================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:758
+// This function returns the import file table. Each entry in the import file
+// table consists of: path_name'\0'base_name'\0'archive_member_name'\0'
+Expected<StringRef> XCOFFObjectFile::getImportFileTable() const {
----------------
This more closely represents the literal string you'd specify.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/needed-libs.test:1
+# RUN: llvm-readobj --needed-libs %p/Inputs/needed-libs-32.o | \
+# RUN:   FileCheck %s --check-prefix=CHECK32
----------------
Add simple comment to the start of the test.

Also add test cases where there are no entries in the table (it is empty), and no table at all (assuming neither case is explicitly forbidden by the spec).


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/needed-libs.test:2
+# RUN: llvm-readobj --needed-libs %p/Inputs/needed-libs-32.o | \
+# RUN:   FileCheck %s --check-prefix=CHECK32
+# RUN: llvm-readobj --needed-libs %p/Inputs/needed-libs-64.o | \
----------------
For both FileCheck invocations, I'd add `--strict-whitespace --match-full-lines` to ensure you are capturing the whole NeedLibraries output.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/needed-libs.test:7-8
+# CHECK32: NeededLibraries [
+# CHECK32:   ,  libc.a,  shr.o
+# CHECK32: ]
+
----------------
Use CHECK-NEXT.

Also, update your test inputs to have more than one entry.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/needed-libs.test:11
+# CHECK64: NeededLibraries [
+# CHECK64:   ,  libc.a,  shr_64.o
+# CHECK64: ]
----------------
a) The leading comma without any content looks wrong
b) If you craft your inputs differently, you could have the same output for both forms and avoid the need for two different checks.
c) Where are the spaces after the commas coming from in this output? The  code doesn't seem to add them currently.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:482
+  if (!ImportFilesOrError) {
+    reportUniqueWarning(ImportFilesOrError.takeError());
+    return;
----------------
Test case required.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:488-499
+  uint32_t NumOfString = 1;
+
+  while (CurrentStr < TableEnd) {
+    // The first import file ID (the entry contains 3 strings) is a default
+    // LIBPATH value to be used by the system loader. Do not dump it here.
+    if (NumOfString > 3)
+      // Use comma to separate path_name, base_name, and archive_member_name
----------------
Using a `for` loop seems like the right thing to do here. Also:
1) Don't use `uint32_t` for a count, use `size_t`.
2) Why are you omitting the LIBPATH value? It seems like you should have some method to dump this.
3) Index from 0 or people are going to think you have an off-by-one error.
4) No need to use comments to describe what the code clearly does.
5) Add a space after the comma?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106643