Set symbol types for function symbols loaded from PE/COFF

[amccarth]

Summary

This fixes the regression of several tests on Windows after rL258621.

The root problem is that ObjectFilePECOFF was not setting type information for the symbols, and the new CL rejects symbols without type information, breaking functionality like thread step-over.

The fix sets the type information for functions (and creates a TODO for other types).

Along the way, I fixed some typos and formatting that made the code I was debugging harder to understand.

In the long run, we should consider replacing most of ObjectFilePECOFF with the COFF parsing code from LLVM.

[clayborg]

Looks fine.

[zturner]

include/lldb/Core/RangeMap.h

1230 ↗

(On Diff #45925)

If you're making cleanup changes anyway, how about a ranged based for.

include/lldb/Symbol/Symtab.h

84 ↗

(On Diff #45925)

Did you use clang-format? If you had this would have a return type on a new line (although obviously you don't want that here since it woudl be inconsistent with the rest of the file).

I guess you would need to clang-format it, then don't take the change to this line. (Which maybe is exactly what you did, just checking)

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

183 ↗

(On Diff #45925)

Instead of returning eSymbolTypeInvalid here, how about eSymbolTypeData? If you look in llvm/Support/COFF.h all of the non function types are data.

[amccarth]

include/lldb/Core/RangeMap.h

1230 ↗

(On Diff #45925)

I had thought about it, especially since it violates the LLVM style guide to call end() on every iteration, but I was trying not to venture too far from the point of the patch. Done now.

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

183 ↗

(On Diff #45925)

I'm not sure about that. Some of the symbols are clearly sections (.text, etc.). Those have 0 for the COFF type, which COFF.h says means "No type information or unknown base type." If .text has a "valid" symbol type, then it will be found (instead of, say, "_main"), and I'm not sure if the unwinding/stepping would work right.

[zturner]

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

183 ↗

(On Diff #45925)

How about:

if (coff_symbol_type == 0)
    return lldb::eSymbolTypeInvalid;
if (coff_symbol_type >> llvm::COFF::SCT_COMPLEX_TYPE_SHIFT) == llvm::COFF::IMAGE_SYM_DTYPE_FUNCTION)
    return lldb::eSymbolTypeCode;
return lldb::eSymbolTypeData;

I feel like we can at least do a little better than always returning eSymbolTypeInvalid if it's not a function without too much work.

[amccarth]

Mostly clang-format.

[amccarth]

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

195 ↗

(On Diff #45933)

After discussion with a COFF expert, it seems wise to postpone trying to do anything more with this type mapping.

lldb/trunk/include/lldb/Core/RangeMap.h

#endif
 
                 if (pos != end && pos->Contains(addr))
                     return std::distance (begin, pos);
             }
             return UINT32_MAX;
         }
 
         uint32_t
-        FindEntryIndexesThatContains (B addr, std::vector<uint32_t> &indexes) const
+        FindEntryIndexesThatContain(B addr, std::vector<uint32_t> &indexes) const
         {
 #ifdef ASSERT_RANGEMAP_ARE_SORTED
             assert (IsSorted());
 #endif
 
             if (!m_entries.empty())
             {
                 typename Collection::const_iterator pos;
-                for(pos = m_entries.begin(); pos != m_entries.end(); pos++)
+                for (const auto &entry : m_entries)
                 {
-                    if (pos->Contains(addr))
-                        indexes.push_back (pos->data);
+                    if (entry.Contains(addr))
+                        indexes.push_back(entry.data);
                 }
             }
             return indexes.size() ;
         }
         
         Entry *
         FindEntryThatContains (B addr)
         {

lldb/trunk/include/lldb/Symbol/Symtab.h

const   Symbol *    GetParent (Symbol *symbol) const;
             uint32_t    AppendSymbolIndexesMatchingRegExAndType (const RegularExpression &regex, lldb::SymbolType symbol_type, std::vector<uint32_t>& indexes);
             uint32_t    AppendSymbolIndexesMatchingRegExAndType (const RegularExpression &regex, lldb::SymbolType symbol_type, Debug symbol_debug_type, Visibility symbol_visibility, std::vector<uint32_t>& indexes);
             size_t      FindAllSymbolsWithNameAndType (const ConstString &name, lldb::SymbolType symbol_type, std::vector<uint32_t>& symbol_indexes);
             size_t      FindAllSymbolsWithNameAndType (const ConstString &name, lldb::SymbolType symbol_type, Debug symbol_debug_type, Visibility symbol_visibility, std::vector<uint32_t>& symbol_indexes);
             size_t      FindAllSymbolsMatchingRexExAndType (const RegularExpression &regex, lldb::SymbolType symbol_type, Debug symbol_debug_type, Visibility symbol_visibility, std::vector<uint32_t>& symbol_indexes);
             Symbol *    FindFirstSymbolWithNameAndType (const ConstString &name, lldb::SymbolType symbol_type, Debug symbol_debug_type, Visibility symbol_visibility);
             Symbol *    FindSymbolContainingFileAddress (lldb::addr_t file_addr, const uint32_t* indexes, uint32_t num_indexes);
             Symbol *    FindSymbolContainingFileAddress (lldb::addr_t file_addr);
-            void        ForEachSymbolContainingFileAddresss (lldb::addr_t file_addr, std::function <bool(Symbol *)> const &callback);
+            void        ForEachSymbolContainingFileAddress(lldb::addr_t file_addr, std::function<bool(Symbol *)> const &callback);
             size_t      FindFunctionSymbols (const ConstString &name, uint32_t name_type_mask, SymbolContextList& sc_list);
             void        CalculateSymbolSizes ();
 
             void        SortSymbolIndexesByValue (std::vector<uint32_t>& indexes, bool remove_duplicates) const;
 
     static  void        DumpSymbolHeader (Stream *s);
 
     

lldb/trunk/source/Core/Module.cpp

if (section_sp && section_sp->GetModule().get() == this)
 
         // Resolve the symbol if requested, but don't re-look it up if we've already found it.
         if (resolve_scope & eSymbolContextSymbol && !(resolved_flags & eSymbolContextSymbol))
         {
             Symtab *symtab = sym_vendor->GetSymtab();
             if (symtab && so_addr.IsSectionOffset())
             {
                 Symbol *matching_symbol = nullptr;
-                symtab->ForEachSymbolContainingFileAddresss (so_addr.GetFileAddress(), [&matching_symbol](Symbol *symbol) -> bool {
+
+                symtab->ForEachSymbolContainingFileAddress(so_addr.GetFileAddress(),
+                                                           [&matching_symbol](Symbol *symbol) -> bool {
                                                                if (symbol->GetType() != eSymbolTypeInvalid)
                                                                {
                                                                    matching_symbol = symbol;
                                                                    return false; // Stop iterating
                                                                }
                                                                return true; // Keep iterating
                                                            });
                 sc.symbol = matching_symbol;
                 if (!sc.symbol &&
                     resolve_scope & eSymbolContextFunction && !(resolved_flags & eSymbolContextFunction))
                 {
                     bool verify_unique = false; // No need to check again since ResolveSymbolContext failed to find a symbol at this address.
                     if (ObjectFile *obj_file = sc.module_sp->GetObjectFile())
                         sc.symbol = obj_file->ResolveSymbolForAddress(so_addr, verify_unique);
                 }

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

public:
 
     static bool
     SaveCore (const lldb::ProcessSP &process_sp,
               const lldb_private::FileSpec &outfile,
               lldb_private::Error &error);
 
     static bool
     MagicBytesMatch (lldb::DataBufferSP& data_sp);
-    
+    static lldb::SymbolType
+    MapSymbolType(uint16_t coff_symbol_type);
+
     bool
     ParseHeader() override;
     
     bool
     SetLoadAddress(lldb_private::Target &target, lldb::addr_t value, bool value_is_offset) override;
 
     lldb::ByteOrder
     GetByteOrder() const override;
     
     bool
     IsExecutable() const override;
     
     uint32_t
     GetAddressByteSize() const override;
-    
 //    virtual lldb_private::AddressClass
 //    GetAddressClass (lldb::addr_t file_addr);
 
     lldb_private::Symtab *
     GetSymtab() override;
     
     bool
     IsStripped() override;

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

 ObjectFilePECOFF::MagicBytesMatch (DataBufferSP& data_sp)
 {
     DataExtractor data(data_sp, eByteOrderLittle, 4);
     lldb::offset_t offset = 0;
     uint16_t magic = data.GetU16 (&offset);
     return magic == IMAGE_DOS_SIGNATURE;
 }
 
+lldb::SymbolType
+ObjectFilePECOFF::MapSymbolType(uint16_t coff_symbol_type)
+{
+    // TODO:  We need to complete this mapping of COFF symbol types to LLDB ones.
+    // For now, here's a hack to make sure our function have types.
+    const auto complex_type = coff_symbol_type >> llvm::COFF::SCT_COMPLEX_TYPE_SHIFT;
+    if (complex_type == llvm::COFF::IMAGE_SYM_DTYPE_FUNCTION)
+    {
+        return lldb::eSymbolTypeCode;
+    }
+    return lldb::eSymbolTypeInvalid;
+}
 
 ObjectFilePECOFF::ObjectFilePECOFF (const lldb::ModuleSP &module_sp, 
                                     DataBufferSP& data_sp,
                                     lldb::offset_t data_offset,
                                     const FileSpec* file, 
                                     lldb::offset_t file_offset,
                                     lldb::offset_t length) :
     ObjectFile (module_sp, file, file_offset, length, data_sp, data_offset),
if (module_sp)
             Mutex::Locker symtab_locker (m_symtab_ap->GetMutex());
             
             const uint32_t num_syms = m_coff_header.nsyms;
 
             if (num_syms > 0 && m_coff_header.symoff > 0)
             {
                 const uint32_t symbol_size = 18;
                 const uint32_t addr_byte_size = GetAddressByteSize ();
-                const size_t symbol_data_size = num_syms * symbol_size; 
-                // Include the 4 bytes string table size at the end of the symbols
+                const size_t symbol_data_size = num_syms * symbol_size;
+                // Include the 4-byte string table size at the end of the symbols
                 DataBufferSP symtab_data_sp(m_file.ReadFileContents (m_coff_header.symoff, symbol_data_size + 4));
                 DataExtractor symtab_data (symtab_data_sp, GetByteOrder(), addr_byte_size);
                 lldb::offset_t offset = symbol_data_size;
                 const uint32_t strtab_size = symtab_data.GetU32 (&offset);
                 DataBufferSP strtab_data_sp(m_file.ReadFileContents (m_coff_header.symoff + symbol_data_size, strtab_size));
                 DataExtractor strtab_data (strtab_data_sp, GetByteOrder(), addr_byte_size);
 
                 // First 4 bytes should be zeroed after strtab_size has been read,
                 // because it is used as offset 0 to encode a NULL string.
                 uint32_t* strtab_data_start = (uint32_t*)strtab_data_sp->GetBytes();
                 strtab_data_start[0] = 0;
 
                 offset = 0;
                 std::string symbol_name;
                 Symbol *symbols = m_symtab_ap->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 = NULL;
-                    // If the first 4 bytes of the symbol string are zero, then we
-                    // it is followed by a 4 byte string table offset. Else these
+                    // 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);
if (module_sp)
                     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->GetSectionAtIndex(symbol.sect-1), symbol.value);
                         symbols[i].GetAddressRef() = symbol_addr;
+                        symbols[i].SetType(MapSymbolType(symbol.type));
                     }
 
                     if (symbol.naux > 0)
                     {
                         i += symbol.naux;
                         offset += symbol_size;
                     }
                 }

lldb/trunk/source/Symbol/Symtab.cpp

if (entry)
         Symbol* symbol = SymbolAtIndex(entry->data);
         if (symbol->ContainsFileAddress(file_addr))
             return symbol;
     }
     return nullptr;
 }
 
 void
-Symtab::ForEachSymbolContainingFileAddresss (addr_t file_addr, std::function <bool(Symbol *)> const &callback)
+Symtab::ForEachSymbolContainingFileAddress(addr_t file_addr, std::function<bool(Symbol *)> const &callback)
 {
     Mutex::Locker locker (m_mutex);
 
     if (!m_file_addr_to_index_computed)
         InitAddressIndexes();
 
     std::vector<uint32_t> all_addr_indexes;
 
     // Get all symbols with file_addr
-    const size_t addr_match_count = m_file_addr_to_index.FindEntryIndexesThatContains(file_addr, all_addr_indexes);
+    const size_t addr_match_count = m_file_addr_to_index.FindEntryIndexesThatContain(file_addr, all_addr_indexes);
 
-    for (size_t i=0; i<addr_match_count; ++i)
+    for (size_t i = 0; i < addr_match_count; ++i)
     {
         if (!callback(SymbolAtIndex(all_addr_indexes[i])))
         break;
     }
 }
 
 void
 Symtab::SymbolIndicesToSymbolContextList (std::vector<uint32_t> &symbol_indexes, SymbolContextList &sc_list)