[llvm-readobj] Allow --hex-dump/--string-dump to dump multiple sections

[MaskRay]

Summary

1. -x foo currently dumps one foo. This change makes it dump all foo.
2. -x foo -x foo currently dumps foo twice. This change makes it dump foo once. In addition, if foo has section index 9, -x foo -x 9 dumps foo once.
3. Give a warning instead of an error if foo does not exist.

The new behaviors match GNU readelf.

Also, print a new line as a separator between two section dumps.
GNU readelf uses two lines, but one seems good enough.

[MaskRay]

Delete an unused include

[grimar]

test/tools/llvm-readobj/hex-dump.test

2

I'd suggest starting the lines from '#'. That will be useful for git history
when we convert all inputs to YAML and inline them here.

Also, you list -x .strtab twice, but -x 9 only once, so may be list it twice too?

5

Can you add check lines showing that .symtab really has index == 9?

5

strtab I mean.

17

In this and below tests you just check that can use section index with -x for
the different targets, right? Please add a comment explaining that.

tools/llvm-readobj/ObjDumper.cpp

40

Map of bool is the same as set I think. Can you use StringSet instead of StringMap<bool>?

88

I'd:

if (First)
  W.startLine() << '\n';
First = false;

123

The same.

tools/llvm-readobj/ObjDumper.h

107

printSectionsAsString, printSectionsAsHex ?

[grimar]

tools/llvm-readobj/ObjDumper.cpp

40

Oh, I misread the code. Please ignore this comment.

[grimar]

tools/llvm-readobj/ObjDumper.cpp

69

I do not think you can do this, because StringMap iteration order is not guaranteed to be deterministic.

70

The same. You should use std::map I think.

[grimar]

tools/llvm-readobj/ObjDumper.cpp

70

Or, if your intention was to keep the insertion order in the outpuit, you probably need MapVector.

[MaskRay]

Address review comments by grimar.

Add two missed llvm-objcopy tests

[MaskRay]

Fix: if (!First)

test/tools/llvm-readobj/hex-dump.test

17

This file was renamed from print-hex.test to be consistent with the option name (also, string-dump.test is not named print-string.test)

tools/llvm-readobj/ObjDumper.cpp

69

Good catch! Changed to std::map.

70

The logic is:

for each section:
  if section name is specified by -x or section index is specified by -x:
    dump()

The insertion order doesn't matter. I'll use std::map for simplicity.

[grimar]

LGTM with a 2 nits/suggestions. But please hold on to see if other reviewers has something to say.

test/tools/llvm-readobj/hex-dump.test

2

"Also, you list -x .strtab twice, but -x 9 only once, so may be list it twice too?"

This part was not addressed. I think you should list -x 9 twice too:

llvm-readobj -x 9 -x 9 -x .strtab -x .strtab

tools/llvm-readobj/ObjDumper.cpp

47

I think we usually wrap the for body in such cases:

for (StringRef Section : Sections) {
  if (!Section.getAsInteger(0, SecIndex))
    SecIndices.emplace(SecIndex, false);
   else
    SecNames.emplace(Section, false);
}

70

What I was thinking about is that in theory we could print the ""could not find section..." warnings in the same order as -x is used in the command line. Though and I also do not think it is really important/useful.

[MaskRay]

Address review comments

[jhenderson]

Not really looked at the code yet, but I noticed a problem with the test, which I'd like addressing.

test/tools/llvm-readobj/hex-dump-multi.s

2

Nit: I'd find it slightly easier to read if this had a blank line after.

test/tools/llvm-readobj/hex-dump.test

2

There is already a "hexdump.test" file. The additional test cases you are adding should be added to that (or that should be deleted/renamed to hex-dump.test).

[MaskRay]

Merge hexdump.test into hex-dump.test, i.e. print-hex.test hexdump.test -> hex-dump.test

[jhenderson]

test/tools/llvm-readobj/hex-dump.test

30

Unnecessary extra line here?

tools/llvm-readobj/ObjDumper.cpp

38

Could Sections be an ArrayRef?

40

I may be being dumb, but why can't we use a StringMap? I'm not seeing where the ordering makes a difference. Or is it just that the output order might change arbitrarily?

44

I don't think we want to accept negative numbers as section indices here. Can you change the type of SecIndex to an unsigned type? I think that will cause this to become an unsigned lookup.

Then again, using a signed type is probably harmless, since we'll warn later, it just is confusing looking at the code as is.

97–107

Is the continue necessary? I think error() ends the program...

108–109

Maybe NameIt and IndexIt instead of It and It2 might be a little clearer.

114

ArrayRef?

145

ArrayRef?

[grimar]

tools/llvm-readobj/ObjDumper.cpp

40

Or is it just that the output order might change arbitrarily?

Yes, and tests can become flaky because of that.

[MaskRay]

Address review comments

[jhenderson]

LGTM.

tools/llvm-readobj/ObjDumper.cpp

52

Interesting. I was not expecting error() to do nothing if passed a success! I don't mind this being left as is, but there is precedent elsewhere in this file to do:

if (...)
  error(...);

See printSectionsAsString for such an example.

[MaskRay]

tools/llvm-readobj/ObjDumper.cpp

52

I'll leave it as is:)

The error(...) pattern is much more common, especially in coff/macho dumpers..

[MaskRay]

Consistently use error(...)

test/tools/llvm-readobj/hex-dump-multi.s

+# REQUIRES: x86-registered-target
+# RUN: llvm-mc -filetype=obj -triple x86_64 %s -o %t.o
+# RUN: llvm-readobj -x .a -x .b %t.o | FileCheck %s
+# RUN: llvm-readelf -x .a -x .b %t.o | FileCheck %s
+
+# CHECK:      Hex dump of section '.a':
+# CHECK-NEXT: 0x00000000 00
+# CHECK-EMPTY:
+# CHECK-NEXT: Hex dump of section '.b':
+# CHECK-NEXT: 0x00000000 01
+# CHECK-EMPTY:
+# CHECK-NEXT: Hex dump of section '.a':
+# CHECK-NEXT: 0x00000000 00
+
+.section .a,"a",@progbits,unique,0
+.byte 0
+.section .b,"a",@progbits
+.byte 1
+.section .a,"a",@progbits,unique,1
+.byte 0

test/tools/llvm-readobj/hex-dump.test

+Both 9 and .strtab refer to .strtab. Test we dump the section only once.
+RUN: llvm-readobj -x 9 -x .strtab -x .strtab %p/Inputs/trivial.obj.elf-x86-64 2>&1 | \
+RUN:   FileCheck %s --check-prefix=ELF
+RUN: llvm-readobj -x 9 -x .strtab -x 10 -x not_exist \
+RUN:   %p/Inputs/trivial.obj.elf-x86-64 2>&1 | FileCheck %s --check-prefixes=ELF-WARN,ELF
+
+ELF-WARN: warning: could not find section 'not_exist'
+ELF-WARN: warning: could not find section 10
+ELF:      Hex dump of section '.strtab':
+ELF-NEXT: 0x00000000 00747269 7669616c 2e6c6c00 6d61696e .trivial.ll.main
+ELF-NEXT: 0x00000010 002e4c2e 73747200 70757473 00536f6d ..L.str.puts.Som
+ELF-NEXT: 0x00000020 654f7468 65724675 6e637469 6f6e005f eOtherFunction._
+ELF-NEXT: 0x00000030 474c4f42 414c5f4f 46465345 545f5441 GLOBAL_OFFSET_TA
+ELF-NEXT: 0x00000040 424c455f 00                         BLE_.
+ELF-NOT:  {{.}}
+
+RUN: llvm-readobj -x 1 %p/Inputs/trivial.obj.coff-x86-64 \
+RUN:     | FileCheck %s --check-prefix COFF
+
+COFF: 0x00000000 4883ec28 488d0d00 000000e8 00000000 H..(H...........
+COFF: 0x00000010 e8000000 0031c048 83c428c3          .....1.H..(.
+
+RUN: llvm-readobj -x 1 %p/Inputs/trivial.obj.macho-x86-64 \
+RUN:     | FileCheck %s --check-prefix MACHO
+
+MACHO: 0x00000000 50488d3d 00000000 e8000000 00e80000 PH.=............
+MACHO: 0x00000010 000031c0 5ac3                       ..1.Z.
+
+RUN: llvm-readobj -x 1 %p/Inputs/trivial.obj.wasm \
+RUN:     | FileCheck %s --check-prefix WASM
+
+WASM: 0x00000000 04600001 7f60017f 017f6000 0060017f .`...`....`..`..
+WASM: 0x00000010 00                                  .

test/tools/llvm-readobj/print-hex.test

-RUN: llvm-readobj -x .strtab %p/Inputs/trivial.obj.elf-x86-64 \
-RUN:     | FileCheck %s --check-prefix ELF
-
-ELF: 0x00000000 00747269 7669616c 2e6c6c00 6d61696e .trivial.ll.main
-ELF: 0x00000010 002e4c2e 73747200 70757473 00536f6d ..L.str.puts.Som
-ELF: 0x00000020 654f7468 65724675 6e637469 6f6e005f eOtherFunction._
-ELF: 0x00000030 474c4f42 414c5f4f 46465345 545f5441 GLOBAL_OFFSET_TA
-ELF: 0x00000040 424c455f 00                         BLE_.
-
-RUN: llvm-readobj -x 1 %p/Inputs/trivial.obj.coff-x86-64 \
-RUN:     | FileCheck %s --check-prefix COFF
-
-COFF: 0x00000000 4883ec28 488d0d00 000000e8 00000000 H..(H...........
-COFF: 0x00000010 e8000000 0031c048 83c428c3          .....1.H..(.
-
-RUN: llvm-readobj -x 1 %p/Inputs/trivial.obj.macho-x86-64 \
-RUN:     | FileCheck %s --check-prefix MACHO
-
-MACHO: 0x00000000 50488d3d 00000000 e8000000 00e80000 PH.=............
-MACHO: 0x00000010 000031c0 5ac3                       ..1.Z.
-
-RUN: llvm-readobj -x 1 %p/Inputs/trivial.obj.wasm \
-RUN:     | FileCheck %s --check-prefix WASM
-
-WASM: 0x00000000 04600001 7f60017f 017f6000 0060017f .`...`....`..`..
-WASM: 0x00000010 00                                  .

test/tools/llvm-readobj/string-dump-multi.s

+# REQUIRES: x86-registered-target
+# RUN: llvm-mc -filetype=obj -triple x86_64 %s -o %t.o
+# RUN: llvm-readobj -p .a -p .b %t.o | FileCheck %s
+# RUN: llvm-readelf -p .a -p .b %t.o | FileCheck %s
+
+# CHECK:      String dump of section '.a':
+# CHECK-NEXT: [     0] a
+# CHECK-EMPTY:
+# CHECK-NEXT: String dump of section '.b':
+# CHECK-NEXT: [     0] b
+# CHECK-EMPTY:
+# CHECK-NEXT: String dump of section '.a':
+# CHECK-NEXT: [     0] a
+
+.section .a,"a",@progbits,unique,0
+.asciz "a"
+.section .b,"a",@progbits
+.asciz "b"
+.section .a,"a",@progbits,unique,1
+.asciz "a"

test/tools/llvm-readobj/string-dump.test

 # Check the dumping of a section as strings.
 # RUN: yaml2obj %s > %t
 
 # RUN: llvm-readobj --string-dump=.strings \
 # RUN:   --string-dump=.not_null_terminated %t > %t.readobj.out
 # RUN: FileCheck %s --input-file=%t.readobj.out
 
 # Also test the different ways --string-dump can be specified, i.e. as a short
 # flag (-p), with different prefix modes (-p .foo, -p=.foo, -p.foo), and with
 # the value being a index section number instead of a section name.
 # RUN: llvm-readobj -p=.strings -p=.not_null_terminated %t > %t.readobj.1
 # RUN: llvm-readobj -p.strings -p.not_null_terminated %t > %t.readobj.2
 # RUN: llvm-readobj --string-dump=1 --string-dump=2 %t > %t.readobj.3
-# RUN: llvm-readobj -p1 -p2 %t > %t.readobj.4
+# RUN: llvm-readobj -p1 -p1 -p2 %t > %t.readobj.4
 # RUN: llvm-readobj -p=1 -p=2 %t > %t.readobj.5
 
 # RUN: cmp %t.readobj.out %t.readobj.1
 # RUN: cmp %t.readobj.out %t.readobj.2
 # RUN: cmp %t.readobj.out %t.readobj.3
 # RUN: cmp %t.readobj.out %t.readobj.4
 # RUN: cmp %t.readobj.out %t.readobj.5
 

 # RUN: cmp %t.readelf.out %t.readelf.5
 # RUN: cmp %t.readelf.out %t.readelf.6
 
 # CHECK:      String dump of section '.strings':
 # CHECK-NEXT: [ 0] here
 # CHECK-NEXT: [ 5] are
 # CHECK-NEXT: [ 9] some
 # CHECK-NEXT: [ e] strings
+# CHECK-EMPTY:
 # CHECK-NEXT: String dump of section '.not_null_terminated':
 # CHECK-NEXT: [ 0] no
 # CHECK-NEXT: [ 3] null{{$}}
 # CHECK-NOT:  {{.}}
 
-# RUN: not llvm-readobj --string-dump=does_not_exist %t 2>&1 | FileCheck %s --check-prefix=ERR1
-# RUN: not llvm-readobj --string-dump=42 %t 2>&1 | FileCheck %s --check-prefix=ERR2
+# RUN: llvm-readobj --string-dump=does_not_exist %t 2>&1 | FileCheck %s --check-prefix=WARN1
+# RUN: llvm-readobj --string-dump=42 %t 2>&1 | FileCheck %s --check-prefix=WARN2
 
-# RUN: not llvm-readelf --string-dump=does_not_exist %t 2>&1 | FileCheck %s --check-prefix=ERR1
-# RUN: not llvm-readelf --string-dump=42 %t 2>&1 | FileCheck %s --check-prefix=ERR2
+# RUN: llvm-readelf --string-dump=does_not_exist %t 2>&1 | FileCheck %s --check-prefix=WARN1
+# RUN: llvm-readelf --string-dump=42 %t 2>&1 | FileCheck %s --check-prefix=WARN2
 
-# ERR1: error: could not find section 'does_not_exist'
-# ERR2: error: could not find section '42'
+# WARN1: warning: could not find section 'does_not_exist'
+# WARN2: warning: could not find section 42
 
 --- !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 Sections:
   - Name: .strings
     Type: SHT_PROGBITS
     Content: 686572650061726500736f6d6500737472696e677300
   - Name: .not_null_terminated
     Type: SHT_PROGBITS
     Content: 6e6f006e756c6c

tools/llvm-readobj/ObjDumper.h

public:
   virtual void printMachOVersionMin() { }
   virtual void printMachODysymtab() { }
   virtual void printMachOSegment() { }
   virtual void printMachOIndirectSymbols() { }
   virtual void printMachOLinkerOptions() { }
 
   virtual void printStackMap() const = 0;
 
-  void printSectionAsString(const object::ObjectFile *Obj, StringRef SecName);
-  void printSectionAsHex(const object::ObjectFile *Obj, StringRef SecName);
+  void printSectionAsString(const object::ObjectFile *Obj,
+                            const std::vector<std::string> &Sections);
+  void printSectionAsHex(const object::ObjectFile *Obj,
+                         const std::vector<std::string> &Sections);
 
 protected:
   ScopedPrinter &W;
 
 private:
   virtual void printSymbols() {}
   virtual void printDynamicSymbols() {}
   virtual void printProgramHeaders() {}

tools/llvm-readobj/ObjDumper.cpp

 #include "ObjDumper.h"
 #include "Error.h"
 #include "llvm-readobj.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/raw_ostream.h"
+#include <unordered_map>
 
 namespace llvm {
 
 ObjDumper::ObjDumper(ScopedPrinter &Writer) : W(Writer) {}
 
 ObjDumper::~ObjDumper() {
 }
 
 static void printAsPrintable(raw_ostream &W, const uint8_t *Start, size_t Len) {
   for (size_t i = 0; i < Len; i++)
     W << (isPrint(Start[i]) ? static_cast<char>(Start[i]) : '.');
 }
 
-static Expected<object::SectionRef>
-getSecNameOrIndexAsSecRef(const object::ObjectFile *Obj, StringRef SecName) {
-  char *StrPtr;
-  long SectionIndex = strtol(SecName.data(), &StrPtr, 10);
-  long SecIndex;
-  if (Obj->isELF())
-    SecIndex = 0;
+static std::vector<object::SectionRef>
+getSectionRefsByNameOrIndex(const object::ObjectFile *Obj,
+                            const std::vector<std::string> &Sections) {
+  std::vector<object::SectionRef> Ret;
+  StringMap<bool> SecNames;
+  std::unordered_map<int, bool> SecIndices;
+  int SecIndex;
+  for (StringRef Section : Sections)
+    if (!Section.getAsInteger(0, SecIndex))
+      SecIndices.emplace(SecIndex, false);
     else
-    SecIndex = 1;
-  for (object::SectionRef SecRef : Obj->sections()) {
-    if (*StrPtr) {
-      StringRef SectionName;
-
-      if (std::error_code E = SecRef.getName(SectionName))
-        return errorCodeToError(E);
-
-      if (SectionName == SecName)
-        return SecRef;
-    } else if (SecIndex == SectionIndex)
-      return SecRef;
+      SecNames.try_emplace(Section, false);
 
+  SecIndex = Obj->isELF() ? 0 : 1;
+  for (object::SectionRef SecRef : Obj->sections()) {
+    StringRef SecName;
+    if (std::error_code EC = SecRef.getName(SecName)) {
+      error(EC);
+      continue;
+    }
+    auto It = SecNames.find(SecName);
+    if (It != SecNames.end())
+      It->second = true;
+    auto It2 = SecIndices.find(SecIndex);
+    if (It2 != SecIndices.end())
+      It2->second = true;
+    if (It != SecNames.end() || It2 != SecIndices.end())
+      Ret.push_back(SecRef);
     SecIndex++;
   }
-  return make_error<StringError>(
-      formatv("could not find section '{0}'", SecName),
-      object::object_error::parse_failed);
+
+  for (StringMapEntry<bool> &S : SecNames)
+    if (!S.second)
+      reportWarning(formatv("could not find section '{0}'", S.first()).str());
+  for (std::pair<int, bool> S : SecIndices)
+    if (!S.second)
+      reportWarning(formatv("could not find section {0}", S.first).str());
+
+  return Ret;
 }
 
 void ObjDumper::printSectionAsString(const object::ObjectFile *Obj,
-                                     StringRef SecName) {
-  Expected<object::SectionRef> SectionRefOrError =
-      getSecNameOrIndexAsSecRef(Obj, SecName);
-  if (!SectionRefOrError)
-    error(std::move(SectionRefOrError));
-  object::SectionRef Section = *SectionRefOrError;
+                                     const std::vector<std::string> &Sections) {
+  bool First = true;
+  for (object::SectionRef Section :
+       getSectionRefsByNameOrIndex(Obj, Sections)) {
     StringRef SectionName;
-
     if (std::error_code E = Section.getName(SectionName))
       error(E);
+    if (First)
+      First = false;
+    else
+      W.startLine() << '\n';
     W.startLine() << "String dump of section '" << SectionName << "':\n";
 
     StringRef SectionContent = unwrapOrError(Section.getContents());
 
     const uint8_t *SecContent = SectionContent.bytes_begin();
     const uint8_t *CurrentWord = SecContent;
     const uint8_t *SecEnd = SectionContent.bytes_end();
 
     while (CurrentWord <= SecEnd) {
       size_t WordSize = strnlen(reinterpret_cast<const char *>(CurrentWord),
                                 SecEnd - CurrentWord);
       if (!WordSize) {
         CurrentWord++;
         continue;
       }
       W.startLine() << format("[%6tx] ", CurrentWord - SecContent);
       printAsPrintable(W.startLine(), CurrentWord, WordSize);
       W.startLine() << '\n';
       CurrentWord += WordSize + 1;
     }
   }
+}
 
 void ObjDumper::printSectionAsHex(const object::ObjectFile *Obj,
-                                  StringRef SecName) {
-  Expected<object::SectionRef> SectionRefOrError =
-      getSecNameOrIndexAsSecRef(Obj, SecName);
-  if (!SectionRefOrError)
-    error(std::move(SectionRefOrError));
-  object::SectionRef Section = *SectionRefOrError;
+                                  const std::vector<std::string> &Sections) {
+  bool First = true;
+  for (object::SectionRef Section :
+       getSectionRefsByNameOrIndex(Obj, Sections)) {
     StringRef SectionName;
-
     if (std::error_code E = Section.getName(SectionName))
       error(E);
+    if (First)
+      First = false;
+    else
+      W.startLine() << '\n';
     W.startLine() << "Hex dump of section '" << SectionName << "':\n";
 
     StringRef SectionContent = unwrapOrError(Section.getContents());
     const uint8_t *SecContent = SectionContent.bytes_begin();
     const uint8_t *SecEnd = SecContent + SectionContent.size();
 
     for (const uint8_t *SecPtr = SecContent; SecPtr < SecEnd; SecPtr += 16) {
       const uint8_t *TmpSecPtr = SecPtr;
       uint8_t i;
       uint8_t k;
 
       W.startLine() << format_hex(Section.getAddress() + (SecPtr - SecContent),
                                   10);
       W.startLine() << ' ';
       for (i = 0; TmpSecPtr < SecEnd && i < 4; ++i) {
         for (k = 0; TmpSecPtr < SecEnd && k < 4; k++, TmpSecPtr++) {
           uint8_t Val = *(reinterpret_cast<const uint8_t *>(TmpSecPtr));
           W.startLine() << format_hex_no_prefix(Val, 2);
         }
         W.startLine() << ' ';
       }
 
       // We need to print the correct amount of spaces to match the format.
       // We are adding the (4 - i) last rows that are 8 characters each.
       // Then, the (4 - i) spaces that are in between the rows.
       // Least, if we cut in a middle of a row, we add the remaining characters,
       // which is (8 - (k * 2)).
       if (i < 4)
         W.startLine() << format("%*c", (4 - i) * 8 + (4 - i) + (8 - (k * 2)),
                                 ' ');
 
       TmpSecPtr = SecPtr;
       for (i = 0; TmpSecPtr + i < SecEnd && i < 16; ++i)
-      W.startLine() << (isPrint(TmpSecPtr[i]) ? static_cast<char>(TmpSecPtr[i])
-                                              : '.');
+        W.startLine() << (isPrint(TmpSecPtr[i])
+                              ? static_cast<char>(TmpSecPtr[i])
 
       W.startLine() << '\n';
     }
   }
+}
 
 } // namespace llvm

tools/llvm-readobj/llvm-readobj.cpp

if (opts::UnwindInfo)
     Dumper->printUnwindInfo();
   if (opts::DynamicTable)
     Dumper->printDynamicTable();
   if (opts::NeededLibraries)
     Dumper->printNeededLibraries();
   if (opts::ProgramHeaders || opts::SectionMapping == cl::BOU_TRUE)
     Dumper->printProgramHeaders(opts::ProgramHeaders, opts::SectionMapping);
   if (!opts::StringDump.empty())
-    llvm::for_each(opts::StringDump, [&Dumper, Obj](StringRef SectionName) {
-      Dumper->printSectionAsString(Obj, SectionName);
-    });
+    Dumper->printSectionAsString(Obj, opts::StringDump);
   if (!opts::HexDump.empty())
-    llvm::for_each(opts::HexDump, [&Dumper, Obj](StringRef SectionName) {
-      Dumper->printSectionAsHex(Obj, SectionName);
-    });
+    Dumper->printSectionAsHex(Obj, opts::HexDump);
   if (opts::HashTable)
     Dumper->printHashTable();
   if (opts::GnuHashTable)
     Dumper->printGnuHashTable();
   if (opts::VersionInfo)
     Dumper->printVersionInfo();
   if (Obj->isELF()) {
     if (opts::ELFLinkerOptions)