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

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

Some minor comments. Thanks for doing this.



================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:110
+  char Padding[4];
+  support::big32_t OffsetToRelEnt;
+};
----------------
32 bit loader section should have no `l_rldoff` field?


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:123
+  support::big64_t OffsetToSymTbl;
+  support::big32_t OffsetToRelEnt;
+  char Padding[4];
----------------
The padding seems not follow XCOFF doc. As I see, there should be 16 byte padding between `OffsetToSymTbl` and `OffsetToRelEnt`?


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/needed-libs.test:7-8
+# CHECK32: NeededLibraries [
+# CHECK32:   ,  libc.a,  shr.o
+# CHECK32: ]
+
----------------
jhenderson wrote:
> Use CHECK-NEXT.
> 
> Also, update your test inputs to have more than one entry.
Would it better to add a header for the contents, like:
```
PATH            BASE            MEMBER     
,            libc.a,            shr.o
,            libunwind.a,            libunwind.so.1
,            libc++abi.a,            libc++abi.so.1
```

And then, maybe we can use the alignment to tell which value is for which field. so we don't need the `,`?

For me, it is a little strange with `,` as the start or end of an import file entry.

We may generate strings like:
```
,    libxxx.a,
,  libunwind.a,  libunwind.so.1
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106643