[llvm-readobj][XCOFF] Add support for printing the String Table.

[Esme]

Summary

This patch adds string table dumping to llvm-readobj. Currently I only implement it for XCOFF, other files can also implement the feature if needed.

[jhenderson]

Please add the new option to the llvm-readobj documentation.

llvm/lib/Object/XCOFFObjectFile.cpp

190

I'd make this return a map of offsets to strings. If you look at the --string-dump output of llvm-readelf, you'll see it includes the string offsets in there, and I think it would be good to use the same output format for this option.

192–193

I'd inline NumberOfSymbols into the loop initialization list, since it's not needed otherwise.

llvm/tools/llvm-readobj/ObjDumper.h

108–109

I'd keep the comment, but name it printStringTable() so that you don't need to rename the function if and when this gets implemented for other file formats - there's nothing fundamentally XCOFF-specific about this feature after all.

llvm/tools/llvm-readobj/XCOFFDumper.cpp

462–463

It would be preferable to print the error as a warning and skip dumping, so that a user who requests to dump the string table and other data can still get the other data. This is the general pattern we follow elsewhere in llvm-readobj, at least for newer code (possibly it hasn't been adopted in XCOFF, but I'm trying to encourage it).

464

StringRef is designed to be passed by value, not reference.

llvm/tools/llvm-readobj/llvm-readobj.cpp

299–302

Let's drop the "xcoff" from the name and description. Perhaps add in the help message "Currently implemented for XCOFF only." instead. That way, when the option is adopted for other formats, we don't need multiple options or to force users to change the switch name.

606

No need for this outer-if statement, since the virtual function has a do-nothing base version.

[Esme]

Addressed comments.

[Higuoxing]

llvm/lib/Object/XCOFFObjectFile.cpp

190

Since we are modifying lib/Object/XCOFFObjectFile.cpp and this function may return an error, I think we need to write unittest for this function. What do you think, @jhenderson ?

[jhenderson]

llvm/include/llvm/Object/XCOFFObjectFile.h

19

I don't think this has been clang-formatted. Include lists should be in alphabetical order.

llvm/lib/Object/XCOFFObjectFile.cpp

190

A gtest unit test is always preferable for libraries, in my opinion, but I don't know if the XCOFF stuff has extensive unit testing, and where there's a tool that handles this error, it's probably sufficient to test it in the tool lit tests.

llvm/tools/llvm-readobj/XCOFFDumper.cpp

464

Always use reportUniqueWarning in preference to reportWarning. The current reportWarning implementation may mean the same warning is reported repeatedly, and we are trying to migrate to reportUniqueWarning instead.

[Esme]

Addressed comments.

[jhenderson]

llvm/tools/llvm-readobj/XCOFFDumper.cpp

463–464

You need to add a test case that will exercise this warning. You also need to return after the warning, or you'll attempt to print the table contents when you don't have a valid map!

[Esme]

llvm/tools/llvm-readobj/XCOFFDumper.cpp

463–464

Well, I'm unable to construct a lit test to exercise this warning. The warning occurs only if the function XCOFFObjectFile::getStringTableEntry(uint32_t Offset) returns an error. I don't think this should happen unless yaml2obj writes the wrong data for symbol name offset or the string table.

[jhenderson]

llvm/tools/llvm-readobj/XCOFFDumper.cpp

463–464

One of yaml2obj's intentions is to be able to craft broken inputs so that these things can be tested. That suggests to me you need to extend yaml2obj a little more, to allow you to force writing a broken value.

That being said, your comment made me think that the approach in getStringTable is wrong. That should be able to dump the entire string table, by just iterating over null terminated strings from whatever offset indicates the string table start, rather than dumping the individual string entries as derived from symbol information. What if, for example, the string table had strings that didn't correspond to symbol names? I'd expect this option to print them.

[Esme]

1. Address comments.
2. Use MapVector instead of DenseMap to make string table in order.

[jhenderson]

I just noticed that the code for --string-dump is generic, rather than ELF specific. It loops over the provided sections, and retrieves some details, but the main body of the loop should do exactly what you want to do. Perhaps it would be worth splitting out the while loop from ObjDumper::printSectionsAsString into a separate function and simply reusing that? This would ensure the XCOFF behaviour and other object behaviour is broadly equivalent.

llvm/lib/Object/XCOFFObjectFile.cpp

192–193

Is this an actual case that can happen? Under what circumstances? Perhaps those circumstances deserve a comment.

[Esme]

Address comment.

[jhenderson]

llvm/test/tools/yaml2obj/XCOFF/basic-doc64.yaml

137

Does this test pass? Why is there a second "." in this name, if so?

What's at bytes 0-2 in the string table?

llvm/test/tools/yaml2obj/XCOFF/long-symbol-name.yaml

27

Ditto.

[Esme]

llvm/test/tools/yaml2obj/XCOFF/basic-doc64.yaml

137

Yes, the test does pass. The offset is 3, but the real offset is 4 in the Symbol entry. It is not a bug of yaml2obj or this patch, since the issue occurs when check the StringTable of a compiled binary object. This should be a bug during parsing the StringTable, and I will post another patch to fix it before the patch.
There are '\0' at bytes 0-2. Here is an example of dumping the string data for every bytes offset of a StringTable for a compiled binary file. Where the correct result should be [ 4] alongname

StringTable {
-every bytes
0: 
1: 
2: 
3: alongname
4: alongname
5: longname
6: ongname
7: ngname
8: gname
9: name
10: ame
11: me
12: e
13: 
-Result
  [     3]   .alongname  
}

[jhenderson]

LGTM, pending the bug fix (but see my inline comment - feel free to land this first if it makes testing the fix easier).

llvm/test/tools/yaml2obj/XCOFF/basic-doc64.yaml

137

Okay, thanks. Please post the other patch, then rebase this one on it. Alternatively, I don't object to you landing this one, with FIXME notes added to the test describing the problem, so that you can use the --string-table option to show the bug fix is correct.

llvm/docs/CommandGuide/llvm-readobj.rst

 
  Display contents of the stackmap section.
 
 .. option:: --string-dump=<section[,section,...]>, -p
 
  Display the specified section(s) as a list of strings. ``section`` may be a
  section index or section name.
 
+.. option:: --string-table
+ 
+ Display contents of the string table.
+
 .. option:: --symbols, --syms, -t
 
  Display the symbol table.
 
 .. option:: --unwind, -u
 
  Display unwind information.
 

llvm/include/llvm/Object/XCOFFObjectFile.h

 //===- XCOFFObjectFile.h - XCOFF object file implementation -----*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file declares the XCOFFObjectFile class.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_OBJECT_XCOFFOBJECTFILE_H
 #define LLVM_OBJECT_XCOFFOBJECTFILE_H
 
+#include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/BinaryFormat/XCOFF.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/Endian.h"
 #include <limits>
 
 namespace llvm {
 namespace object {
 
 struct XCOFFFileHeader32 {
public:
   getLogicalNumberOfRelocationEntries(const XCOFFSectionHeader32 &Sec) const;
 
   Expected<ArrayRef<XCOFFRelocation32>>
   relocations(const XCOFFSectionHeader32 &) const;
 
   // This function returns string table entry.
   Expected<StringRef> getStringTableEntry(uint32_t Offset) const;
 
+  // This function returns the string table.
+  MapVector<uint64_t, StringRef> getStringTable() const; 
+
   const XCOFF::SymbolAuxType *getSymbolAuxType(uintptr_t AuxEntryAddress) const;
 
   static uintptr_t getAdvancedSymbolEntryAddress(uintptr_t CurrentAddress,
                                                  uint32_t Distance);
 
   static bool classof(const Binary *B) { return B->isXCOFF(); }
 }; // XCOFFObjectFile
 

llvm/lib/Object/XCOFFObjectFile.cpp

XCOFFObjectFile::getStringTableEntry(uint32_t Offset) const {
 
   if (StringTable.Data != nullptr && StringTable.Size > Offset)
     return (StringTable.Data + Offset);
 
   return make_error<GenericBinaryError>("Bad offset for string table entry",
                                         object_error::parse_failed);
 }
 
+MapVector<uint64_t, StringRef> XCOFFObjectFile::getStringTable() const {
+  MapVector<uint64_t, StringRef> StrTableMap;
+  if (StringTable.Data == nullptr)
+    return StrTableMap;
+  const char *StringTablePtr = StringTable.Data;
+  for (uint32_t I = 1; I < StringTable.Size; ++I) {
+    if (StringTablePtr[I - 1] == '\0') {
+      StringRef StringTableEntry = StringTablePtr + I;
+      if (StringTableEntry.size())
+        StrTableMap[I] = StringTableEntry;
+    }
+  }
+  return StrTableMap;
+}
+
 Expected<StringRef>
 XCOFFObjectFile::getCFileName(const XCOFFFileAuxEnt *CFileEntPtr) const {
   if (CFileEntPtr->NameInStrTbl.Magic != XCOFFSymbolRef::NAME_IN_STR_TBL_MAGIC)
     return generateXCOFFFixedNameStringRef(CFileEntPtr->Name);
   return getStringTableEntry(CFileEntPtr->NameInStrTbl.Offset);
 }
 
 Expected<StringRef> XCOFFObjectFile::getSymbolName(DataRefImpl Symb) const {

llvm/test/tools/yaml2obj/XCOFF/basic-doc64.yaml

 # RUN: yaml2obj %s -o %t
-# RUN: llvm-readobj --headers --symbols %t | FileCheck %s --check-prefix=CHECK64
+# RUN: llvm-readobj --headers --symbols --string-table %t | \
+# RUN:   FileCheck %s --check-prefix=CHECK64
 
 --- !XCOFF
 FileHeader:
   MagicNumber:     0x01F7
 Sections:
   - Name:            .text
     Flags:           [ STYP_TEXT ]
   - Name:            .data

 # CHECK64-NEXT:     Name: .text
 # CHECK64-NEXT:     Value (RelocatableAddress): 0x0
 # CHECK64-NEXT:     Section: .text
 # CHECK64-NEXT:     Type: 0x0
 # CHECK64-NEXT:     StorageClass: C_HIDEXT (0x6B)
 # CHECK64-NEXT:     NumberOfAuxEntries: 0
 # CHECK64-NEXT:   }
 # CHECK64-NEXT: ]
+# CHECK64-NEXT: StringTable {
+# CHECK64-NEXT:   [     3]   .text
+# CHECK64-NEXT:   [     a]   .abs
+# CHECK64-NEXT:   [     f]   .undef
+# CHECK64-NEXT:   [    16]   .file
+# CHECK64-NEXT: }

llvm/test/tools/yaml2obj/XCOFF/long-symbol-name.yaml

 ## Test that the string table works well for long symbol names.
-## TODO: Dump the raw string table and check the contents.
 # RUN: yaml2obj %s -o %t
-# RUN: llvm-readobj --symbols %t | FileCheck %s
+# RUN: llvm-readobj --symbols --string-table %t | FileCheck %s
 
 # CHECK:      AddressSize: 32bit
 # CHECK-NEXT: Symbols [
 # CHECK-NEXT:   Symbol {
 # CHECK-NEXT:     Index: 0
 # CHECK-NEXT:     Name: .symname
 # CHECK-NEXT:     Value: 0x0
 # CHECK-NEXT:     Section: N_UNDEF
 # CHECK-NEXT:     Type: 0x0
 # CHECK-NEXT:     StorageClass: C_NULL (0x0)
 # CHECK-NEXT:     NumberOfAuxEntries: 0
 # CHECK-NEXT:   }
 # CHECK-NEXT:   Symbol {
 # CHECK-NEXT:     Index: 1
 # CHECK-NEXT:     Name: .longname
 # CHECK-NEXT:     Value: 0x0
 # CHECK-NEXT:     Section: N_UNDEF
 # CHECK-NEXT:     Type: 0x0
 # CHECK-NEXT:     StorageClass: C_NULL (0x0)
 # CHECK-NEXT:     NumberOfAuxEntries: 0
 # CHECK-NEXT:   }
 # CHECK-NEXT: ]
+# CHECK-NEXT: StringTable {
+# CHECK-NEXT:   [     3]   .longname
+# CHECK-NEXT: }
 
 --- !XCOFF
 FileHeader:
   MagicNumber: 0x1DF
 Symbols:
   - Name: .symname
   - Name: .longname

llvm/tools/llvm-readobj/ObjDumper.h

public:
   // Only implemented for MachO.
   virtual void printMachODataInCode() { }
   virtual void printMachOVersionMin() { }
   virtual void printMachODysymtab() { }
   virtual void printMachOSegment() { }
   virtual void printMachOIndirectSymbols() { }
   virtual void printMachOLinkerOptions() { }
 
+  // Currently only implemented for XCOFF.
+  virtual void printStringTable() { }
+
   virtual void printStackMap() const = 0;
 
   void printSectionsAsString(const object::ObjectFile &Obj,
                              ArrayRef<std::string> Sections);
   void printSectionsAsHex(const object::ObjectFile &Obj,
                           ArrayRef<std::string> Sections);
 
   std::function<Error(const Twine &Msg)> WarningHandler;

llvm/tools/llvm-readobj/XCOFFDumper.cpp

public:
   void printFileHeaders() override;
   void printSectionHeaders() override;
   void printRelocations() override;
   void printSymbols() override;
   void printDynamicSymbols() override;
   void printUnwindInfo() override;
   void printStackMap() const override;
   void printNeededLibraries() override;
+  void printStringTable() override;
 
 private:
   template <typename T> void printSectionHeaders(ArrayRef<T> Sections);
   template <typename T> void printGenericSectionHeader(T &Sec) const;
   template <typename T> void printOverflowSectionHeader(T &Sec) const;
   void printFileAuxEnt(const XCOFFFileAuxEnt *AuxEntPtr);
   void printCsectAuxEnt(XCOFFCsectAuxRef AuxEntRef);
   void printSectAuxEntForStat(const XCOFFSectAuxEntForStat *AuxEntPtr);

 }
 
 void XCOFFDumper::printSymbols() {
   ListScope Group(W, "Symbols");
   for (const SymbolRef &S : Obj.symbols())
     printSymbol(S);
 }
 
+void XCOFFDumper::printStringTable() {
+  DictScope DS(W, "StringTable");
+  MapVector<uint64_t, StringRef> StrTableMap = Obj.getStringTable();
+  for (const auto &EntryPair : StrTableMap) {
+    W.startLine() << format("[%6tx] ", EntryPair.first);
+    W.startLine() << EntryPair.second;
+    W.startLine() << '\n';
+  }
+}
+
 void XCOFFDumper::printDynamicSymbols() {
   llvm_unreachable("Unimplemented functionality for XCOFFDumper");
 }
 
 void XCOFFDumper::printUnwindInfo() {
   llvm_unreachable("Unimplemented functionality for XCOFFDumper");
 }
 

llvm/tools/llvm-readobj/llvm-readobj.cpp

namespace opts {
   cl::list<std::string> StringDump(
       "string-dump", cl::value_desc("number|name"),
       cl::desc("Display the specified section(s) as a list of strings"),
       cl::ZeroOrMore);
   cl::alias StringDumpShort("p", cl::desc("Alias for --string-dump"),
                             cl::aliasopt(StringDump), cl::Prefix,
                             cl::NotHidden);
 
+  // --string-table
+  cl::opt<bool>
+      StringTable("string-table",
+                  cl::desc("Display the string table (only for XCOFF now)"));
+
   // --hex-dump, -x
   cl::list<std::string>
       HexDump("hex-dump", cl::value_desc("number|name"),
               cl::desc("Display the specified section(s) as hexadecimal bytes"),
               cl::ZeroOrMore);
   cl::alias HexDumpShort("x", cl::desc("Alias for --hex-dump"),
                          cl::aliasopt(HexDump), cl::Prefix, cl::NotHidden);
 
namespace opts {
   cl::opt<bool> COFFResources("coff-resources",
                               cl::desc("Display the PE/COFF .rsrc section"));
 
   // --coff-load-config
   cl::opt<bool>
   COFFLoadConfig("coff-load-config",
                  cl::desc("Display the PE/COFF load config"));
 
   // --elf-linker-options
   cl::opt<bool>
   ELFLinkerOptions("elf-linker-options",
                    cl::desc("Display the ELF .linker-options section"));
 
   // --macho-data-in-code
   cl::opt<bool>
   MachODataInCode("macho-data-in-code",
                   cl::desc("Display MachO Data in Code command"));
 
   // --macho-indirect-symbols
   cl::opt<bool>
static void dumpObject(ObjectFile &Obj, ScopedPrinter &Writer,
   if (!opts::HexDump.empty())
     Dumper->printSectionsAsHex(Obj, opts::HexDump);
   if (opts::HashTable)
     Dumper->printHashTable();
   if (opts::GnuHashTable)
     Dumper->printGnuHashTable();
   if (opts::VersionInfo)
     Dumper->printVersionInfo();
+  if (opts::StringTable)
+    Dumper->printStringTable();
   if (Obj.isELF()) {
     if (opts::DependentLibraries)
       Dumper->printDependentLibs();
     if (opts::ELFLinkerOptions)
       Dumper->printELFLinkerOptions();
     if (opts::ArchSpecificInfo)
       Dumper->printArchSpecificInfo();
     if (opts::SectionGroups)
if (opts::MachOLinkerOptions)
       Dumper->printMachOLinkerOptions();
     if (opts::MachOSegment)
       Dumper->printMachOSegment();
     if (opts::MachOVersionMin)
       Dumper->printMachOVersionMin();
     if (opts::MachODysymtab)
       Dumper->printMachODysymtab();
   }
   if (opts::PrintStackMap)
     Dumper->printStackMap();
   if (opts::PrintStackSizes)
     Dumper->printStackSizes();
 }
 
 /// Dumps each object file in \a Arc;
 static void dumpArchive(const Archive *Arc, ScopedPrinter &Writer) {
   Error Err = Error::success();