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

[Esme Yi via Phabricator via llvm-commits]

Esme added inline comments.


================
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
----------------
jhenderson wrote:
> 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).
According to the spec, no entries in the table is the same as no table at all, because a table consists only of entries.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/needed-libs.test:11
+# CHECK64: NeededLibraries [
+# CHECK64:   ,  libc.a,  shr_64.o
+# CHECK64: ]
----------------
jhenderson wrote:
> 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.
a)  Because there is no path name in some import ID entries. Should I not print the comma for an empty name? The output format references the output of the system `objdump` in AIX.
```
$ objdump -P loader 1.o
...
Import files:
  0: /home/esme/workspace/build/bin/../lib/powerpc-ibm-aix7.2.0.0:/home/esme/workspace/build/bin/../xlmass/latest/lib/:/home/esme/workspace/build/bin/../lib/:/usr/lib:/lib,,
  1: ,libc.a,shr.o
```
c) Spaces are automatically generated by ScopedPrinter when using `startLine()<<`.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:482
+  if (!ImportFilesOrError) {
+    reportUniqueWarning(ImportFilesOrError.takeError());
+    return;
----------------
jhenderson wrote:
> Test case required.
I will add cases to test wrong paths after the import file table can be customized in yaml2obj.


================
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
----------------
jhenderson wrote:
> 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?
Thanks!
2. What I omitted here is `/home/esme/workspace/build/bin/../lib/powerpc-ibm-aix7.2.0.0:/home/esme/workspace/build/bin/../xlmass/latest/lib/:/home/esme/workspace/build/bin/../lib/:/usr/lib:/lib\0\0\0`, which contains a default LIBPATH name without file name or archive member name, and I think we don't need to dump it in `needed libs`? Btw, ELF doesn't print paths under `--needed-libs` either.

5. `W.startLine() <<` will generate 2 spaces in the case.

```
  void printIndent() {
    OS << Prefix;
    for (int i = 0; i < IndentLevel; ++i)
      OS << "  ";
  }
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106643