[lldb][COFF] Match symbols from COFF symbol table to export symbols

[alvinhochun]

Summary

If a symbol is the same as an export symbol, mark it as 'Additional' to
prevent the duplicated symbol from being repeated in some commands (e.g.
disas -n func). If the RVA is the same but exported with a different
name, only synchronize the symbol types.

Depends on D134265

[mstorsjo]

This looks mostly reasonable to me, a couple discussion points only.

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

817

As a curious question - since D134265 we get better quality for the symbol types set from the get go - what are the other cases you foresee where this will make a difference? I don't mind if there isn't any difference though, syncing the types from the symbol table which is more expressible probably is good anyway. Just wondering about the actual utility of it.

828

This condition had me puzzled for a moment, until I realized that you're synchronizing symbol type in the other direction here. Is it worth clarifying this in the comment?

(Also, I presume the test case doesn't really trigger this case?)

[labath]

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

893

Can you have multiple symbols pointing to the same address? Make this should use stable_sort instead?

[alvinhochun]

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

817

Given what limited info the existing implementations (LLD and GNU ld) write to the symbol table, we probably won't get any better type info from just the symbol table alone. Though I was thinking if we can use a bit of additional heuristics we can assign more specific symbol types.

For example, perhaps if we can guess that a particular code symbol may be an import thunk from the jump instruction it contains, then we can set the symbol as 'eSymbolTypeTrampoline'. It seems the breakpoint command will skip trampoline symbols, so if you, say, set a breakpoint on memcpy it will only break on the real function but not the import thunk. Not sure how feasible it is to implement though.

828

Hmm, I can try to improve the comment.

The aliasInt symbol should trigger this case. Perhaps it will be clearer if I update the previous patch to include these symbols, so the difference in output can be seen in this patch.

893

Yes, it can happen. The ordering shouldn't affect what AppendFromCOFFSymbolTable does but I suppose stable_sort does make it more deterministic to avoid future issues down the line.

[alvinhochun]

Updated test based on parent and addressed review comments.

[mstorsjo]

No further objections/comments from me on this!

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

828

Oh, ok, I had missed that MapSymbolType returns code or invalid - I had expected it to return code or data. I guess this is fine then. (Changing that function, or changing the logic here to look at section types is out of scope here anyway, and I don’t know if the difference between data and invalid matters?)

[alvinhochun]

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

828

Data vs Invalid does seem to have an effect on whether a symbol can be printed as data or have its address taken. I can't print or dump any static/globals when the type is Invalid. I think the DWARF debug info should have at least some information about these symbols, but I have no idea how that works.

I wonder if I should change MapSymbolType to return Data for anything that is not code?

[mstorsjo]

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

828

I think that might be a good idea - but that's clearly a separate change from this one too. Thanks!

[labath]

Seems fine to me from a general perspective. I think others have already checked the windows parts.

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

893

Yeah, it's better to avoid having this kind of nondeterministic behavior that can differ from run to run. LLDB is not so string about it as clang/llvm, but it's still a good idea.

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

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();
+  typedef std::vector<std::pair<uint32_t, uint32_t>> rva_symbol_list_t;
   void AppendFromCOFFSymbolTable(lldb_private::SectionList *sect_list,
-                                 lldb_private::Symtab &symtab);
-  void AppendFromExportTable(lldb_private::SectionList *sect_list,
+                                 lldb_private::Symtab &symtab,
+                                 const rva_symbol_list_t &sorted_exports);
+  rva_symbol_list_t 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

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();
-  AppendFromExportTable(sect_list, symtab);
-  AppendFromCOFFSymbolTable(sect_list, symtab);
+  rva_symbol_list_t sorted_exports = AppendFromExportTable(sect_list, symtab);
+  AppendFromCOFFSymbolTable(sect_list, symtab, sorted_exports);
 }
 
-void ObjectFilePECOFF::AppendFromCOFFSymbolTable(SectionList *sect_list,
-                                                 Symtab &symtab) {
+static bool RVASymbolListCompareRVA(const std::pair<uint32_t, uint32_t> &a,
+                                    const std::pair<uint32_t, uint32_t> &b) {
+  return a.first < b.first;
+}
+
+void ObjectFilePECOFF::AppendFromCOFFSymbolTable(
+    SectionList *sect_list, Symtab &symtab,
+    const ObjectFilePECOFF::rva_symbol_list_t &sorted_exports) {
   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;
 
for (const auto &sym_ref : m_binary->symbols()) {
     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()));
+      const auto symbol_type = MapSymbolType(coff_sym_ref.getType());
+      symbol.SetType(symbol_type);
+
+      // Check for duplicate of exported symbols:
+      const uint32_t symbol_rva = symbol.GetAddressRef().GetFileAddress() -
+                                  m_coff_header_opt.image_base;
+      const auto &first_match = std::lower_bound(
+          sorted_exports.begin(), sorted_exports.end(),
+          std::make_pair(symbol_rva, 0), RVASymbolListCompareRVA);
+      for (auto it = first_match;
+           it != sorted_exports.end() && it->first == symbol_rva; ++it) {
+        Symbol *exported = symtab.SymbolAtIndex(it->second);
+        if (symbol_type != lldb::eSymbolTypeInvalid)
+          exported->SetType(symbol_type);
+        if (exported->GetMangled() == symbol.GetMangled()) {
+          symbol.SetExternal(true);
+          // We don't want the symbol to be duplicated (e.g. when running
+          // `disas -n func`), but we also don't want to erase this entry (to
+          // preserve the original symbol order), so we mark it as additional.
+          symbol.SetType(lldb::eSymbolTypeAdditional);
+        } else {
+          // It is possible for a symbol to be exported in a different name
+          // from its original. In this case keep both entries so lookup using
+          // either names will work. If this symbol has an invalid type, replace
+          // it with the type from the export symbol.
+          if (symbol.GetType() == lldb::eSymbolTypeInvalid)
+            symbol.SetType(exported->GetType());
+        }
+      }
     }
     symtab.AddSymbol(symbol);
   }
 }
 
-void ObjectFilePECOFF::AppendFromExportTable(SectionList *sect_list,
-                                             Symtab &symtab) {
+ObjectFilePECOFF::rva_symbol_list_t
+ObjectFilePECOFF::AppendFromExportTable(SectionList *sect_list,
   const auto *export_table = m_binary->getExportTable();
   if (!export_table)
-    return;
+    return {};
   const uint32_t num_syms = export_table->AddressTableEntries;
   if (num_syms == 0)
-    return;
+    return {};
 
   Log *log = GetLog(LLDBLog::Object);
+  rva_symbol_list_t export_list;
   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}",
for (const auto &entry : m_binary->export_directories()) {
 
     // An exported symbol may be either code or data. Guess by checking whether
     // the section containing the symbol is executable.
     symbol.SetType(lldb::eSymbolTypeData);
     if (auto section_sp = symbol.GetAddressRef().GetSection())
       if (section_sp->GetPermissions() & ePermissionsExecutable)
         symbol.SetType(lldb::eSymbolTypeCode);
     symbol.SetExternal(true);
-    symtab.AddSymbol(symbol);
+    uint32_t idx = symtab.AddSymbol(symbol);
+    export_list.push_back(std::make_pair(function_rva, idx));
   }
+  std::stable_sort(export_list.begin(), export_list.end(),
+                   RVASymbolListCompareRVA);
+  return export_list;
 }
 
 std::unique_ptr<CallFrameInfo> ObjectFilePECOFF::CreateCallFrameInfo() {
   if (llvm::COFF::EXCEPTION_TABLE >= m_coff_header_opt.data_dirs.size())
     return {};
 
   data_directory data_dir_exception =
       m_coff_header_opt.data_dirs[llvm::COFF::EXCEPTION_TABLE];

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:          UserID DSX Type       File Address/Value {{.*}} Size            Flags           Name
 # CHECK-NEXT:     ------
-# CHECK-NEXT:          1   X Code    0x0000000180001020        0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFnAlias
-# CHECK-NEXT:          2   X Code    0x0000000180001010        0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
-# CHECK-NEXT:          3   X Data    0x0000000180003000        0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
-# CHECK-NEXT:          4   X Data    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-NEXT:          1   X Code       0x0000000180001020        0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFnAlias
+# CHECK-NEXT:          2   X Code       0x0000000180001010        0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
+# CHECK-NEXT:          3   X Data       0x0000000180003000        0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
+# CHECK-NEXT:          4   X Data       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   X Additional 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   X Additional 0x0000000180003000        0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
+# CHECK-NEXT: 4294967295     Data       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;