[PATCH] D79939: Implement --dependent-libraries for GNU output

[James Henderson via Phabricator via llvm-commits]

jhenderson marked 11 inline comments as done.
jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dependent-libraries.test:7
 
 # RUN: yaml2obj --docnum=1 %s -o %t1
+# RUN: llvm-readobj --dependent-libraries %t1 | \
----------------
grimar wrote:
> Probably it worth to mention that `--dependent-libraries` is not implemented in GNU at the moment of 14 may 2020.
I'm not sure I follow. This change is adding support for GNU.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dependent-libraries.test:9
+# RUN: llvm-readobj --dependent-libraries %t1 | \
+# RUN:   FileCheck %s --check-prefix=LLVM --strict-whitespace --match-full-lines
+# RUN: llvm-readelf --dependent-libraries %t1 | \
----------------
grimar wrote:
> Add `--implicit-check-not="DependentLibs"` for consistency with below line?
I can add it, but I don't think it's that useful. In the GNU mode, the header is printed for each SHT_LLVM_DEPEDENT_LIBRARIES section, so checking it doesn't appear other than when we expect helps show that the header is not printed for other section types, whereas for LLVM it is only ever printed once (and the -NEXT lines help ensure that other sections aren't interepreted as dependent library sections).


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dependent-libraries.test:41
+# RUN: llvm-readelf --dependent-libraries %t2 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=MIX-GNU -DFILE=%t2
+
----------------
grimar wrote:
> These could be 2 lines, seems no need to wrap.
Yup, fair. That comes from when I had other options etc in an early version.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5347
+  StringRef SecName;
+  uint64_t SecOffset;
+  auto PrintSection = [&]() {
----------------
grimar wrote:
> Seems you can replace `SecName` and `SecOffset` with a new `NameOffset` variable.
Fair enough. `NameOffset` was intended for library names and offsets within the section, but I think it's fine sharing the struct in that way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79939