Implement --dependent-libraries for GNU output

[jhenderson]

Summary

Previously, the option was only implemented for LLVM output. This fixes https://bugs.llvm.org/show_bug.cgi?id=45695.

The output roughly follows the output style for similar options like --syms/--notes etc, combined with --string-dump output.

[grimar]

llvm/test/tools/llvm-readobj/ELF/dependent-libraries.test

7

Probably it worth to mention that --dependent-libraries is not implemented in GNU at the moment of 14 may 2020.

9

Add --implicit-check-not="DependentLibs" for consistency with below line?

41

These could be 2 lines, seems no need to wrap.

llvm/tools/llvm-readobj/ELFDumper.cpp

729

Can this be a protected method?

I'd also not mix virtual vs non-virtual methods.

5349

S is not used as for variable/condition. Seems that inlining Contents.begin() would look cleaner.

5366

Seems you can replace SecName and SecOffset with a new NameOffset variable.

5383

You do not need to wrap a single line into brackets.

5397

It would be easier to use & here too, as this lambda is not supposed to be used outside of the scope of this method.
In this case using just & reads IMO better in compare with explicit listing of variables.

5399

I'd probably move out these helper functions. It is a bit hard to read them inlined.
Naming them should help for readability.

[jhenderson]

llvm/test/tools/llvm-readobj/ELF/dependent-libraries.test

7

I'm not sure I follow. This change is adding support for GNU.

9

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).

41

Yup, fair. That comes from when I had other options etc in an early version.

llvm/tools/llvm-readobj/ELFDumper.cpp

5366

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.

[jhenderson]

Address review comments.

[grimar]

llvm/test/tools/llvm-readobj/ELF/dependent-libraries.test

7

I mean GNU readelf does not implement this, right? So it is the difference we have atm.
Perhaps worth to mention it in a comment.

[MaskRay]

Looks great! Add [llvm-readobj] to the subject.

[jhenderson]

llvm/test/tools/llvm-readobj/ELF/dependent-libraries.test

7

Right, thanks. I'll add it to the commit message, I think. I could see GNU implementing it to match our output, so consequently the comment in code would go stale.

[grimar]

LGTM

llvm/test/tools/llvm-readobj/ELF/dependent-libraries.test

 ## Check that we can use the --dependent-libraries option
 ## to dump SHT_LLVM_DEPENDENT_LIBRARIES sections.
 
 ## Check how we dump a file that has a single valid SHT_LLVM_DEPENDENT_LIBRARIES
 ## section with multiple entries.
 
 # RUN: yaml2obj --docnum=1 %s -o %t1
-# RUN: llvm-readobj --dependent-libraries %t1 2>&1 | FileCheck %s -DFILE=%t
-
-# CHECK:      DependentLibs [
-# CHECK-NEXT:   foo
-# CHECK-NEXT:   bar
-# CHECK-NEXT:   foo
-# CHECK-NEXT: ]
+# RUN: llvm-readobj --dependent-libraries %t1 | \
+# RUN:   FileCheck %s --check-prefix=LLVM --strict-whitespace --match-full-lines
+# RUN: llvm-readelf --dependent-libraries %t1 | \
+# RUN:   FileCheck %s --check-prefix=GNU --strict-whitespace --match-full-lines --implicit-check-not="Dependent libraries"
+
+#      LLVM:DependentLibs [
+# LLVM-NEXT:  foo
+# LLVM-NEXT:  bar
+# LLVM-NEXT:  foo
+# LLVM-NEXT:]
+
+#      GNU:Dependent libraries section .deplibs at offset 0x40 contains 3 entries:
+# GNU-NEXT:  [     0]  foo
+# GNU-NEXT:  [     4]  bar
+# GNU-NEXT:  [     8]  foo
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_X86_64
 Sections:
   - Name:      .deplibs
     Type:      SHT_LLVM_DEPENDENT_LIBRARIES
     Libraries: [ foo, bar, foo ]
 
 ## Now, check how we dump a mix of valid, empty and invalid SHT_LLVM_DEPENDENT_LIBRARIES sections.
 
 # RUN: yaml2obj --docnum=2 %s -o %t2
-# RUN: llvm-readobj --dependent-libraries %t2 2>&1 | FileCheck %s --check-prefix=MIX -DFILE=%t2
+# RUN: llvm-readobj --dependent-libraries %t2 2>&1 | FileCheck %s --check-prefix=MIX-LLVM -DFILE=%t2
+# RUN: llvm-readelf --dependent-libraries %t2 2>&1 | FileCheck %s --check-prefix=MIX-GNU -DFILE=%t2
 
-# MIX:      DependentLibs [
-# MIX-NEXT:  warning: '[[FILE]]': SHT_LLVM_DEPENDENT_LIBRARIES section at index 1 is broken: the content is not null-terminated
-# MIX-NEXT:  abc
-# MIX-NEXT:  warning: '[[FILE]]': SHT_LLVM_DEPENDENT_LIBRARIES section at index 4 is broken: section [index 4] has a sh_offset (0xffff0000) + sh_size (0x4) that is greater than the file size (0x2c0)
-# MIX-NEXT:  bar
-# MIX-NEXT:  xxx
-# MIX-NEXT: ]
+# MIX-LLVM:      DependentLibs [
+# MIX-LLVM-NEXT:   warning: '[[FILE]]': SHT_LLVM_DEPENDENT_LIBRARIES section at index 1 is broken: the content is not null-terminated
+# MIX-LLVM-NEXT:   abc
+# MIX-LLVM-NEXT:   warning: '[[FILE]]': SHT_LLVM_DEPENDENT_LIBRARIES section at index 4 is broken: section [index 4] has a sh_offset (0xffff0000) + sh_size (0x4) that is greater than the file size (0x308)
+# MIX-LLVM-NEXT:   bar
+# MIX-LLVM-NEXT:   xxx
+# MIX-LLVM-NEXT:   baz
+# MIX-LLVM-NEXT: ]
+
+# MIX-GNU:      warning: '[[FILE]]': SHT_LLVM_DEPENDENT_LIBRARIES section at index 1 is broken: the content is not null-terminated
+# MIX-GNU-NEXT: Dependent libraries section .deplibs.nonul at offset 0x40 contains 0 entries:
+# MIX-GNU-EMPTY:
+# MIX-GNU-NEXT: Dependent libraries section .deplibs.single at offset 0x43 contains 1 entries:
+# MIX-GNU-NEXT:   [     0]  abc
+# MIX-GNU-EMPTY:
+# MIX-GNU-NEXT: Dependent libraries section .deplibs.empty at offset 0x47 contains 0 entries:
+# MIX-GNU-EMPTY:
+# MIX-GNU-NEXT: warning: '[[FILE]]': SHT_LLVM_DEPENDENT_LIBRARIES section at index 4 is broken: section [index 4] has a sh_offset (0xffff0000) + sh_size (0x4) that is greater than the file size (0x308)
+# MIX-GNU-NEXT: Dependent libraries section .deplibs.broken.shoffset at offset 0xffff0000 contains 0 entries:
+# MIX-GNU-EMPTY:
+# MIX-GNU-NEXT: Dependent libraries section .deplibs.multiple at offset 0x4b contains 2 entries:
+# MIX-GNU-NEXT:   [     0]  bar
+# MIX-GNU-NEXT:   [     4]  xxx
+# MIX-GNU-EMPTY:
+# MIX-GNU-NEXT: warning: '[[FILE]]': cannot get section name of SHT_LLVM_DEPENDENT_LIBRARIES section: a section [index 6] has an invalid sh_name (0x10000) offset which goes past the end of the section name string table
+# MIX-GNU-NEXT: Dependent libraries section <?> at offset 0x53 contains 1 entries:
+# MIX-GNU-NEXT:   [     0]  baz
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_X86_64
 Sections:
## Case 4: test we report a warning when the section offset is invalid.
   - Name:      .deplibs.broken.shoffset
     Type:      SHT_LLVM_DEPENDENT_LIBRARIES
     Libraries: [ yyy ]
     ShOffset:  0xffff0000
 ## Case 5: test we can dump all entries from a valid section that has more than one entry.
   - Name:      .deplibs.multiple
     Type:      SHT_LLVM_DEPENDENT_LIBRARIES
     Libraries: [ bar, xxx ]
+## Case 6: test we report a warning in GNU mode if the section name can't be read.
+  - ShName:    0x10000
+    Type:      SHT_LLVM_DEPENDENT_LIBRARIES
+    Libraries: [ baz ]
+
+## Show the output when there are no dependent library sections.
+# RUN: yaml2obj --docnum=3 %s -o %t3
+# RUN: llvm-readobj --dependent-libraries %t3 2>&1 | FileCheck %s --check-prefix=NONE
+# RUN: llvm-readelf --dependent-libraries %t3 2>&1 | FileCheck %s --allow-empty --implicit-check-not={{.}}
 
-## llvm-readelf doesn't support --dependent-libraries yet.
-# RUN: llvm-readelf --dependent-libraries %t1 2>&1 | FileCheck %s --check-prefix=READELF
+# NONE:      DependentLibs [
+# NONE-NEXT: ]
 
-# READELF: printDependentLibs not implemented!
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64

llvm/tools/llvm-readobj/ELFDumper.cpp

public:
   virtual void printFileHeaders(const ELFFile<ELFT> *Obj) = 0;
   virtual void printGroupSections(const ELFFile<ELFT> *Obj) = 0;
   virtual void printRelocations(const ELFFile<ELFT> *Obj) = 0;
   virtual void printSectionHeaders(const ELFFile<ELFT> *Obj) = 0;
   virtual void printSymbols(const ELFFile<ELFT> *Obj, bool PrintSymbols,
                             bool PrintDynamicSymbols) = 0;
   virtual void printHashSymbols(const ELFFile<ELFT> *Obj) {}
   virtual void printDependentLibs(const ELFFile<ELFT> *Obj) = 0;
   virtual void printDynamic(const ELFFile<ELFT> *Obj) {}
   virtual void printDynamicRelocations(const ELFFile<ELFT> *Obj) = 0;
   virtual void printSymtabMessage(const ELFFile<ELFT> *Obj, StringRef Name,
                                   size_t Offset, bool NonVisibilityBitsUsed) {}
   virtual void printSymbol(const ELFFile<ELFT> *Obj, const Elf_Sym *Symbol,
                            const Elf_Sym *FirstSym, StringRef StrTable,
                            bool IsDynamic, bool NonVisibilityBitsUsed) = 0;
   virtual void printProgramHeaders(const ELFFile<ELFT> *Obj,
                                    bool PrintProgramHeaders,
void printStackSize(const ELFObjectFile<ELFT> *Obj, RelocationRef Rel,
                       const RelocationResolver &Resolver, DataExtractor Data);
   virtual void printStackSizeEntry(uint64_t Size, StringRef FuncName) = 0;
   virtual void printMipsGOT(const MipsGOTParser<ELFT> &Parser) = 0;
   virtual void printMipsPLT(const MipsGOTParser<ELFT> &Parser) = 0;
   virtual void printMipsABIFlags(const ELFObjectFile<ELFT> *Obj) = 0;
   const ELFDumper<ELFT> *dumper() const { return Dumper; }
 
 protected:
+  void printDependentLibsHelper(
+      const ELFFile<ELFT> *Obj,
+      function_ref<void(const Elf_Shdr &)> OnSectionStart,
+      function_ref<void(StringRef, uint64_t)> OnSectionEntry);
+
   void reportUniqueWarning(Error Err) const;
   StringRef FileName;
 
 private:
   const ELFDumper<ELFT> *Dumper;
 };
 
 template <typename ELFT> class GNUStyle : public DumpStyle<ELFT> {

 }
 
 template <class ELFT>
 void GNUStyle<ELFT>::printELFLinkerOptions(const ELFFile<ELFT> *Obj) {
   OS << "printELFLinkerOptions not implemented!\n";
 }
 
 template <class ELFT>
+void DumpStyle<ELFT>::printDependentLibsHelper(
+    const ELFFile<ELFT> *Obj,
+    function_ref<void(const Elf_Shdr &)> OnSectionStart,
+    function_ref<void(StringRef, uint64_t)> OnLibEntry) {
+  auto Warn = [this](unsigned SecNdx, StringRef Msg) {
+    this->reportUniqueWarning(
+        createError("SHT_LLVM_DEPENDENT_LIBRARIES section at index " +
+                    Twine(SecNdx) + " is broken: " + Msg));
+  };
+
+  unsigned I = -1;
+  for (const Elf_Shdr &Shdr : unwrapOrError(this->FileName, Obj->sections())) {
+    ++I;
+    if (Shdr.sh_type != ELF::SHT_LLVM_DEPENDENT_LIBRARIES)
+      continue;
+
+    OnSectionStart(Shdr);
+
+    Expected<ArrayRef<uint8_t>> ContentsOrErr = Obj->getSectionContents(&Shdr);
+    if (!ContentsOrErr) {
+      Warn(I, toString(ContentsOrErr.takeError()));
+      continue;
+    }
+
+    ArrayRef<uint8_t> Contents = *ContentsOrErr;
+    if (!Contents.empty() && Contents.back() != 0) {
+      Warn(I, "the content is not null-terminated");
+      continue;
+    }
+
+    for (const uint8_t *I = Contents.begin(), *E = Contents.end(); I < E;) {
+      StringRef Lib((const char *)I);
+      OnLibEntry(Lib, I - Contents.begin());
+      I += Lib.size() + 1;
+    }
+  }
+}
+
+template <class ELFT>
 void GNUStyle<ELFT>::printDependentLibs(const ELFFile<ELFT> *Obj) {
-  OS << "printDependentLibs not implemented!\n";
+  bool SectionStarted = false;
+  struct NameOffset {
+    StringRef Name;
+    uint64_t Offset;
+  };
+  std::vector<NameOffset> SecEntries;
+  NameOffset Current;
+  auto PrintSection = [&]() {
+    OS << "Dependent libraries section " << Current.Name << " at offset "
+       << format_hex(Current.Offset, 1) << " contains " << SecEntries.size()
+       << " entries:\n";
+    for (NameOffset Entry : SecEntries)
+      OS << "  [" << format("%6tx", Entry.Offset) << "]  " << Entry.Name
+         << "\n";
+    OS << "\n";
+    SecEntries.clear();
+  };
+
+  auto OnSectionStart = [&](const Elf_Shdr &Shdr) {
+    if (SectionStarted)
+      PrintSection();
+    SectionStarted = true;
+    Current.Offset = Shdr.sh_offset;
+    Expected<StringRef> Name = Obj->getSectionName(&Shdr);
+    if (!Name) {
+      Current.Name = "<?>";
+      this->reportUniqueWarning(
+          createError("cannot get section name of "
+                      "SHT_LLVM_DEPENDENT_LIBRARIES section: " +
+                      toString(Name.takeError())));
+    } else {
+      Current.Name = *Name;
+    }
+  };
+  auto OnLibEntry = [&](StringRef Lib, uint64_t Offset) {
+    SecEntries.push_back(NameOffset{Lib, Offset});
+  };
+
+  printDependentLibsHelper(Obj, OnSectionStart, OnLibEntry);
+  if (SectionStarted)
+    PrintSection();
 }
 
 // Used for printing section names in places where possible errors can be
 // ignored.
 static StringRef getSectionName(const SectionRef &Sec) {
   Expected<StringRef> NameOrErr = Sec.getName();
   if (NameOrErr)
     return *NameOrErr;
for (const Elf_Shdr &Shdr : unwrapOrError(this->FileName, Obj->sections())) {
     for (size_t I = 0; I < Strings.size(); I += 2)
       W.printString(Strings[I], Strings[I + 1]);
   }
 }
 
 template <class ELFT>
 void LLVMStyle<ELFT>::printDependentLibs(const ELFFile<ELFT> *Obj) {
   ListScope L(W, "DependentLibs");
-
-  auto Warn = [this](unsigned SecNdx, StringRef Msg) {
-    this->reportUniqueWarning(
-        createError("SHT_LLVM_DEPENDENT_LIBRARIES section at index " +
-                    Twine(SecNdx) + " is broken: " + Msg));
-  };
-
-  unsigned I = -1;
-  for (const Elf_Shdr &Shdr : unwrapOrError(this->FileName, Obj->sections())) {
-    ++I;
-    if (Shdr.sh_type != ELF::SHT_LLVM_DEPENDENT_LIBRARIES)
-      continue;
-
-    Expected<ArrayRef<uint8_t>> ContentsOrErr = Obj->getSectionContents(&Shdr);
-    if (!ContentsOrErr) {
-      Warn(I, toString(ContentsOrErr.takeError()));
-      continue;
-    }
-
-    ArrayRef<uint8_t> Contents = *ContentsOrErr;
-    if (!Contents.empty() && Contents.back() != 0) {
-      Warn(I, "the content is not null-terminated");
-      continue;
-    }
-
-    for (const uint8_t *I = Contents.begin(), *E = Contents.end(); I < E;) {
-      StringRef Lib((const char *)I);
-      W.printString(Lib);
-      I += Lib.size() + 1;
-    }
-  }
+  printDependentLibsHelper(
+      Obj, [](const Elf_Shdr &) {},
+      [this](StringRef Lib, uint64_t) { W.printString(Lib); });
 }
 
 template <class ELFT>
 void LLVMStyle<ELFT>::printStackSizes(const ELFObjectFile<ELFT> *Obj) {
   ListScope L(W, "StackSizes");
   if (Obj->isRelocatableObject())
     this->printRelocatableStackSizes(Obj, []() {});
   else