[lldb][COFF] Rewrite ParseSymtab to list both export and symbol tables

[alvinhochun]

Summary

This reimplements ObjectFilePECOFF::ParseSymtab to replace the manual
data extraction with what COFFObjectFile already provides. Also use
SymTab::AddSymbol instead of resizing the SymTab then assigning each
elements afterwards.

Previously, ParseSymTab loads symbols from both the COFF symbol table
and the export table, but if there are any entries in the export table,
it overwrites all the symbols already loaded from the COFF symbol table.
Due to the change to use AddSymbols, this no longer happens, and so the
SymTab now contains all symbols from both tables as expected.

The export symbols are now ordered by ordinal, instead of by the name
table order.

In its current state, it is possible for symbols in the COFF symbol
table to be duplicated by those in the export table. This behaviour will
be modified in a separate change.

[labath]

I don't know how coff symbol tables work, but this seems reasonable to me.

lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml

11

Even though it does not look like that, this will match an entry with any flags (because FileCheck does a substring match). The simplest solution would be to match the preceding UserID field as well.

35–76

Could this be reduced? Maybe by removing these zero fields?

[alvinhochun]

Updated test

[alvinhochun]

Thanks for reviewing. The COFF symbol table seems to be used by MinGW binaries in conjunction with DWARF debugging symbols. Some symbols (those added by the linker in particular) are not included in the DWARF symbol. I understand that it is not used by MSVC, in which all debugging symbols are stored in PDB format.

[mstorsjo]

This looks good to me, nothing to add from my PoV. Nice to have this condensed to a small enough rewrite, with few enough functional changes to wrap one's head around.

[alvinhochun]

Updated the test to include more symbols used in later changes

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h

typedef struct coff_opt_header {
     uint64_t heap_reserve_size = 0;
     uint64_t heap_commit_size = 0;
     uint32_t loader_flags = 0;
     //    uint32_t  num_data_dir_entries;
     std::vector<data_directory>
         data_dirs; // will contain num_data_dir_entries entries
   } coff_opt_header_t;
 
-  enum coff_data_dir_type {
-    coff_data_dir_export_table = 0,
-    coff_data_dir_import_table = 1,
-    coff_data_dir_exception_table = 3
-  };
-
   typedef struct section_header {
     char name[8] = {};
     uint32_t vmsize = 0;  // Virtual Size
     uint32_t vmaddr = 0;  // Virtual Addr
     uint32_t size = 0;    // File size
     uint32_t offset = 0;  // File offset
     uint32_t reloff = 0;  // Offset to relocations
     uint32_t lineoff = 0; // Offset to line table entries
     uint16_t nreloc = 0;  // Number of relocation entries
     uint16_t nline = 0;   // Number of line table entries
     uint32_t flags = 0;
   } section_header_t;
 
-  typedef struct coff_symbol {
-    char name[8] = {};
-    uint32_t value = 0;
-    uint16_t sect = 0;
-    uint16_t type = 0;
-    uint8_t storage = 0;
-    uint8_t naux = 0;
-  } coff_symbol_t;
-
-  typedef struct export_directory_entry {
-    uint32_t characteristics = 0;
-    uint32_t time_date_stamp = 0;
-    uint16_t major_version = 0;
-    uint16_t minor_version = 0;
-    uint32_t name = 0;
-    uint32_t base = 0;
-    uint32_t number_of_functions = 0;
-    uint32_t number_of_names = 0;
-    uint32_t address_of_functions = 0;
-    uint32_t address_of_names = 0;
-    uint32_t address_of_name_ordinals = 0;
-  } export_directory_entry;
-
   static bool ParseDOSHeader(lldb_private::DataExtractor &data,
                              dos_header_t &dos_header);
   static bool ParseCOFFHeader(lldb_private::DataExtractor &data,
                               lldb::offset_t *offset_ptr,
                               coff_header_t &coff_header);
   bool ParseCOFFOptionalHeader(lldb::offset_t *offset_ptr);
   bool ParseSectionHeaders(uint32_t offset);
 
static lldb::SectionType GetSectionType(llvm::StringRef sect_name,
                                           const section_header_t &sect);
 
   typedef std::vector<section_header_t> SectionHeaderColl;
   typedef SectionHeaderColl::iterator SectionHeaderCollIter;
   typedef SectionHeaderColl::const_iterator SectionHeaderCollConstIter;
 
 private:
   bool CreateBinary();
+  void AppendFromCOFFSymbolTable(lldb_private::SectionList *sect_list,
+                                 lldb_private::Symtab &symtab);
+  void AppendFromExportTable(lldb_private::SectionList *sect_list,
+                             lldb_private::Symtab &symtab);
 
   dos_header_t m_dos_header;
   coff_header_t m_coff_header;
   coff_opt_header_t m_coff_header_opt;
   SectionHeaderColl m_sect_headers;
   lldb::addr_t m_image_base;
   lldb_private::Address m_entry_point_address;
   llvm::Optional<lldb_private::FileSpecList> m_deps_filespec;
   std::unique_ptr<llvm::object::COFFObjectFile> m_binary;
   lldb_private::UUID m_uuid;
 };
 
 #endif // LLDB_SOURCE_PLUGINS_OBJECTFILE_PECOFF_OBJECTFILEPECOFF_H

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/Timer.h"
 #include "lldb/Utility/UUID.h"
-#include "llvm/BinaryFormat/COFF.h"
 
+#include "llvm/BinaryFormat/COFF.h"
 #include "llvm/Object/COFFImportFile.h"
 #include "llvm/Support/CRC.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/FormatAdapters.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/MemoryBuffer.h"
 
 #define IMAGE_DOS_SIGNATURE 0x5A4D    // MZ
 #define IMAGE_NT_SIGNATURE 0x00004550 // PE00
 #define OPT_HEADER_MAGIC_PE32 0x010b
 #define OPT_HEADER_MAGIC_PE32_PLUS 0x020b
 
if (const char *name = m_data.GetCStr(&string_file_offset))
       return name;
     return "";
   }
   return hdr_name;
 }
 
 void ObjectFilePECOFF::ParseSymtab(Symtab &symtab) {
   SectionList *sect_list = GetSectionList();
-  const uint32_t num_syms = m_coff_header.nsyms;
-  if (m_file && num_syms > 0 && m_coff_header.symoff > 0) {
-    const uint32_t symbol_size = 18;
-    const size_t symbol_data_size = num_syms * symbol_size;
-    // Include the 4-byte string table size at the end of the symbols
-    DataExtractor symtab_data =
-        ReadImageData(m_coff_header.symoff, symbol_data_size + 4);
-    lldb::offset_t offset = symbol_data_size;
-    const uint32_t strtab_size = symtab_data.GetU32(&offset);
-    if (strtab_size > 0) {
-      DataExtractor strtab_data = ReadImageData(
-          m_coff_header.symoff + symbol_data_size, strtab_size);
-
-      offset = 0;
-      std::string symbol_name;
-      Symbol *symbols = symtab.Resize(num_syms);
-      for (uint32_t i = 0; i < num_syms; ++i) {
-        coff_symbol_t symbol;
-        const uint32_t symbol_offset = offset;
-        const char *symbol_name_cstr = nullptr;
-        // If the first 4 bytes of the symbol string are zero, then they
-        // are followed by a 4-byte string table offset. Else these
-        // 8 bytes contain the symbol name
-        if (symtab_data.GetU32(&offset) == 0) {
-          // Long string that doesn't fit into the symbol table name, so
-          // now we must read the 4 byte string table offset
-          uint32_t strtab_offset = symtab_data.GetU32(&offset);
-          symbol_name_cstr = strtab_data.PeekCStr(strtab_offset);
-          symbol_name.assign(symbol_name_cstr);
-        } else {
-          // Short string that fits into the symbol table name which is 8
-          // bytes
-          offset += sizeof(symbol.name) - 4; // Skip remaining
-          symbol_name_cstr = symtab_data.PeekCStr(symbol_offset);
-          if (symbol_name_cstr == nullptr)
-            break;
-          symbol_name.assign(symbol_name_cstr, sizeof(symbol.name));
-        }
-        symbol.value = symtab_data.GetU32(&offset);
-        symbol.sect = symtab_data.GetU16(&offset);
-        symbol.type = symtab_data.GetU16(&offset);
-        symbol.storage = symtab_data.GetU8(&offset);
-        symbol.naux = symtab_data.GetU8(&offset);
-        symbols[i].GetMangled().SetValue(ConstString(symbol_name.c_str()));
-        if ((int16_t)symbol.sect >= 1) {
-          Address symbol_addr(sect_list->FindSectionByID(symbol.sect),
-                              symbol.value);
-          symbols[i].GetAddressRef() = symbol_addr;
-          symbols[i].SetType(MapSymbolType(symbol.type));
-        }
+  AppendFromExportTable(sect_list, symtab);
+  AppendFromCOFFSymbolTable(sect_list, symtab);
 
-        if (symbol.naux > 0) {
-          i += symbol.naux;
-          offset += symbol.naux * symbol_size;
-        }
-      }
-    }
-  }
+void ObjectFilePECOFF::AppendFromCOFFSymbolTable(SectionList *sect_list,
+                                                 Symtab &symtab) {
+  const uint32_t num_syms = m_binary->getNumberOfSymbols();
+  if (num_syms == 0)
+    return;
+  // Check that this is not a bigobj; we do not support bigobj.
+  if (m_binary->getSymbolTableEntrySize() !=
+      sizeof(llvm::object::coff_symbol16))
+    return;
+
+  Log *log = GetLog(LLDBLog::Object);
+  symtab.Reserve(symtab.GetNumSymbols() + num_syms);
+  for (const auto &sym_ref : m_binary->symbols()) {
+    const auto coff_sym_ref = m_binary->getCOFFSymbol(sym_ref);
+    auto name_or_error = sym_ref.getName();
+    if (auto err = name_or_error.takeError()) {
+      LLDB_LOG(log,
+               "ObjectFilePECOFF::AppendFromCOFFSymbolTable - failed to get "
+               "symbol table entry name: {0}",
+               llvm::fmt_consume(std::move(err)));
+      continue;
+    const llvm::StringRef sym_name = *name_or_error;
+    Symbol symbol;
+    symbol.GetMangled().SetValue(ConstString(sym_name));
+    int16_t section_number =
+        static_cast<int16_t>(coff_sym_ref.getSectionNumber());
+    if (section_number >= 1) {
+      symbol.GetAddressRef() = Address(
+          sect_list->FindSectionByID(section_number), coff_sym_ref.getValue());
+      symbol.SetType(MapSymbolType(coff_sym_ref.getType()));
+    }
+    symtab.AddSymbol(symbol);
 
-  // Read export header
-  if (coff_data_dir_export_table < m_coff_header_opt.data_dirs.size() &&
-      m_coff_header_opt.data_dirs[coff_data_dir_export_table].vmsize > 0 &&
-      m_coff_header_opt.data_dirs[coff_data_dir_export_table].vmaddr > 0) {
-    export_directory_entry export_table;
-    uint32_t data_start =
-        m_coff_header_opt.data_dirs[coff_data_dir_export_table].vmaddr;
-
-    DataExtractor symtab_data = ReadImageDataByRVA(
-        data_start, m_coff_header_opt.data_dirs[0].vmsize);
-    lldb::offset_t offset = 0;
+void ObjectFilePECOFF::AppendFromExportTable(SectionList *sect_list,
+                                             Symtab &symtab) {
+  const auto *export_table = m_binary->getExportTable();
+  if (!export_table)
+    return;
+  const uint32_t num_syms = export_table->AddressTableEntries;
+  if (num_syms == 0)
+    return;
 
-    // Read export_table header
-    export_table.characteristics = symtab_data.GetU32(&offset);
-    export_table.time_date_stamp = symtab_data.GetU32(&offset);
-    export_table.major_version = symtab_data.GetU16(&offset);
-    export_table.minor_version = symtab_data.GetU16(&offset);
-    export_table.name = symtab_data.GetU32(&offset);
-    export_table.base = symtab_data.GetU32(&offset);
-    export_table.number_of_functions = symtab_data.GetU32(&offset);
-    export_table.number_of_names = symtab_data.GetU32(&offset);
-    export_table.address_of_functions = symtab_data.GetU32(&offset);
-    export_table.address_of_names = symtab_data.GetU32(&offset);
-    export_table.address_of_name_ordinals = symtab_data.GetU32(&offset);
-
-    bool has_ordinal = export_table.address_of_name_ordinals != 0;
-
-    lldb::offset_t name_offset = export_table.address_of_names - data_start;
-    lldb::offset_t name_ordinal_offset =
-        export_table.address_of_name_ordinals - data_start;
-
-    Symbol *symbols = symtab.Resize(export_table.number_of_names);
-
-    std::string symbol_name;
-
-    // Read each export table entry
-    for (size_t i = 0; i < export_table.number_of_names; ++i) {
-      uint32_t name_ordinal =
-          has_ordinal ? symtab_data.GetU16(&name_ordinal_offset) : i;
-      uint32_t name_address = symtab_data.GetU32(&name_offset);
-
-      const char *symbol_name_cstr =
-          symtab_data.PeekCStr(name_address - data_start);
-      symbol_name.assign(symbol_name_cstr);
-
-      lldb::offset_t function_offset = export_table.address_of_functions -
-                                        data_start +
-                                        sizeof(uint32_t) * name_ordinal;
-      uint32_t function_rva = symtab_data.GetU32(&function_offset);
-
-      Address symbol_addr(m_coff_header_opt.image_base + function_rva,
-                          sect_list);
-      symbols[i].GetMangled().SetValue(ConstString(symbol_name.c_str()));
-      symbols[i].GetAddressRef() = symbol_addr;
-      symbols[i].SetType(lldb::eSymbolTypeCode);
-      symbols[i].SetDebug(true);
+  Log *log = GetLog(LLDBLog::Object);
+  symtab.Reserve(symtab.GetNumSymbols() + num_syms);
+  // Read each export table entry, ordered by ordinal instead of by name.
+  for (const auto &entry : m_binary->export_directories()) {
+    llvm::StringRef sym_name;
+    if (auto err = entry.getSymbolName(sym_name)) {
+      LLDB_LOG(log,
+               "ObjectFilePECOFF::AppendFromExportTable - failed to get export "
+               "table entry name: {0}",
+               llvm::fmt_consume(std::move(err)));
+      continue;
+    }
+    Symbol symbol;
+    // Note: symbol name may be empty if it is only exported by ordinal.
+    symbol.GetMangled().SetValue(ConstString(sym_name));
+
+    uint32_t function_rva;
+    if (auto err = entry.getExportRVA(function_rva)) {
+      LLDB_LOG(log,
+               "ObjectFilePECOFF::AppendFromExportTable - failed to get "
+               "address of export entry '{0}': {1}",
+               sym_name, llvm::fmt_consume(std::move(err)));
+      continue;
     }
+    symbol.GetAddressRef() =
+        Address(m_coff_header_opt.image_base + function_rva, sect_list);
+    symbol.SetType(lldb::eSymbolTypeCode);
+    symbol.SetDebug(true);
+    symtab.AddSymbol(symbol);
   }
 }
 
 std::unique_ptr<CallFrameInfo> ObjectFilePECOFF::CreateCallFrameInfo() {
-  if (coff_data_dir_exception_table >= m_coff_header_opt.data_dirs.size())
+  if (llvm::COFF::EXCEPTION_TABLE >= m_coff_header_opt.data_dirs.size())
     return {};
 
   data_directory data_dir_exception =
-      m_coff_header_opt.data_dirs[coff_data_dir_exception_table];
+      m_coff_header_opt.data_dirs[llvm::COFF::EXCEPTION_TABLE];
   if (!data_dir_exception.vmaddr)
     return {};
 
   if (m_coff_header.machine != llvm::COFF::IMAGE_FILE_MACHINE_AMD64)
     return {};
 
   return std::make_unique<PECallFrameInfo>(*this, data_dir_exception.vmaddr,
                                            data_dir_exception.vmsize);

lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml

+# RUN: yaml2obj %s -o %t
+# RUN: lldb-test symbols %t | FileCheck %s
+
+# Checks that the symtab contains both symbols from the export table and the
+# COFF symbol table.
+
+# CHECK:          UserID DSX Type    File Address/Value {{.*}} Size            Flags           Name
+# CHECK-NEXT:     ------
+# CHECK-NEXT: 4294967295 D   Code    0x0000000180001020        0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFnAlias
+# CHECK-NEXT: 4294967295 D   Code    0x0000000180001010        0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
+# CHECK-NEXT: 4294967295 D   Code    0x0000000180003000        0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
+# CHECK-NEXT: 4294967295 D   Code    0x0000000180003004        0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportIntAlias
+# CHECK-NEXT: 4294967295     Code    0x0000000180001000        0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
+# CHECK-NEXT: 4294967295     Code    0x0000000180001010        0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
+# CHECK-NEXT: 4294967295     Code    0x0000000180001020        0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasFunc
+# CHECK-NEXT: 4294967295     Invalid 0x0000000180003000        0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
+# CHECK-NEXT: 4294967295     Invalid 0x0000000180003004        0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasInt
+# CHECK-NEXT: 4294967295     Invalid 0x0000000180003008        0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} internalInt
+# CHECK-EMPTY:
+
+# Test file generated with:
+#   clang -O2 --target=x86_64-windows-msvc test.c -nostdlib -c -o test.obj
+#   lld-link -debug:symtab -dll -out:test.dll -entry:entry -export:exportFnAlias=aliasFunc -export:exportIntAlias=aliasInt test.obj
+# test.c:
+#   __declspec(dllexport) int exportInt;
+#   int aliasInt;
+#   int internalInt;
+#   void entry(void) {}
+#   __declspec(dllexport) void exportFunc(void) {}
+#   void aliasFunc(void) {}
+
+--- !COFF
+OptionalHeader:
+  AddressOfEntryPoint: 4096
+  ImageBase:       6442450944
+  SectionAlignment: 4096
+  FileAlignment:   512
+  MajorOperatingSystemVersion: 6
+  MinorOperatingSystemVersion: 0
+  MajorImageVersion: 0
+  MinorImageVersion: 0
+  MajorSubsystemVersion: 6
+  MinorSubsystemVersion: 0
+  Subsystem:       IMAGE_SUBSYSTEM_WINDOWS_GUI
+  DLLCharacteristics: [ IMAGE_DLL_CHARACTERISTICS_HIGH_ENTROPY_VA, IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE, IMAGE_DLL_CHARACTERISTICS_NX_COMPAT ]
+  SizeOfStackReserve: 1048576
+  SizeOfStackCommit: 4096
+  SizeOfHeapReserve: 1048576
+  SizeOfHeapCommit: 4096
+  ExportTable:
+    RelativeVirtualAddress: 8192
+    Size:            156
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [ IMAGE_FILE_EXECUTABLE_IMAGE, IMAGE_FILE_LARGE_ADDRESS_AWARE, IMAGE_FILE_DLL ]
+sections:
+  - Name:            .text
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    VirtualAddress:  4096
+    VirtualSize:     33
+    SectionData:     C36666666666662E0F1F840000000000C36666666666662E0F1F840000000000C3
+  - Name:            .rdata
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ ]
+    VirtualAddress:  8192
+    VirtualSize:     156
+    SectionData:     0000000000000000000000002820000001000000040000000400000042200000522000006220000073796D626F6C732D6578706F7274732E632E746D702E646C6C00201000001010000000300000043000006A20000078200000832000008D20000000000100020003006578706F7274466E416C696173006578706F727446756E63006578706F7274496E74006578706F7274496E74416C69617300
+  - Name:            .data
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+    VirtualAddress:  12288
+    VirtualSize:     12
+    SectionData:     ''
+symbols:
+  - Name:            entry
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_FUNCTION
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            exportFunc
+    Value:           16
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_FUNCTION
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            aliasFunc
+    Value:           32
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_FUNCTION
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            exportInt
+    Value:           0
+    SectionNumber:   3
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            aliasInt
+    Value:           4
+    SectionNumber:   3
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            internalInt
+    Value:           8
+    SectionNumber:   3
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+...

llvm/include/llvm/Object/COFF.h

public:
   uint32_t getNumberOfSymbols() const {
     if (!SymbolTable16 && !SymbolTable32)
       return 0;
     return getRawNumberOfSymbols();
   }
 
   uint32_t getStringTableSize() const { return StringTableSize; }
 
+  const export_directory_table_entry *getExportTable() const {
+    return ExportDirectory;
+  }
+
   const coff_load_configuration32 *getLoadConfig32() const {
     assert(!is64());
     return reinterpret_cast<const coff_load_configuration32 *>(LoadConfig);
   }
 
   const coff_load_configuration64 *getLoadConfig64() const {
     assert(is64());
     return reinterpret_cast<const coff_load_configuration64 *>(LoadConfig);