https://github.com/DavidSpickett created https://github.com/llvm/llvm-project/pull/91603
Code in https://github.com/llvm/llvm-project/pull/90622 exposed a situation where a symbol table may be loaded via a second copy of the same object. This resulted in the first copy having its address class map updated, but not the second. And it was that second one we used for symbol lookups, since it was found by a plugin and we assume those to be more specific. (the plugins returning the same file may itself be a bug) I tried fixing this by returning the address map changes from the symbol table parsing. Which works but it's awkward to make sure both object's maps get updated. Instead, this change moves the address class map into the symbol table, which is shared between the objects already. ObjectFileELF::GetAddressClass was already checking whether the symbol table existed anyway, so we are ok to use it. >From 9187bbfc99a16803c56a3cf679695f511354d9c9 Mon Sep 17 00:00:00 2001 From: David Spickett <david.spick...@linaro.org> Date: Thu, 9 May 2024 14:37:30 +0000 Subject: [PATCH] [lldb][ELF] Move address class map into the symbol table Code in https://github.com/llvm/llvm-project/pull/90622 exposed a situation where a symbol table may be loaded via a second copy of the same object. This resulted in the first copy having its address class map updated, but not the second. And it was that second one we used for symbol lookups, since it was found by a plugin and we assume those to be more specific. (the plugins returning the same file may itself be a bug) I tried fixing this by returning the address map changes from the symbol table parsing. Which works but it's awkward to make sure both object's maps get updated. Instead, this change moves the address class map into the symbol table, which is shared between the objects already. ObjectFileELF::GetAddressClass was already checking whether the symbol table existed anyway, so we are ok to use it. --- lldb/include/lldb/Symbol/Symtab.h | 15 ++++++ .../Plugins/ObjectFile/ELF/ObjectFileELF.cpp | 48 ++++++++----------- .../Plugins/ObjectFile/ELF/ObjectFileELF.h | 6 --- lldb/source/Symbol/Symtab.cpp | 19 ++++++++ 4 files changed, 54 insertions(+), 34 deletions(-) diff --git a/lldb/include/lldb/Symbol/Symtab.h b/lldb/include/lldb/Symbol/Symtab.h index 57627d2dde7d2..331716c3bac41 100644 --- a/lldb/include/lldb/Symbol/Symtab.h +++ b/lldb/include/lldb/Symbol/Symtab.h @@ -23,6 +23,8 @@ class Symtab { public: typedef std::vector<uint32_t> IndexCollection; typedef UniqueCStringMap<uint32_t> NameToIndexMap; + typedef std::map<lldb::addr_t, lldb_private::AddressClass> + FileAddressToAddressClassMap; enum Debug { eDebugNo, // Not a debug symbol @@ -239,6 +241,16 @@ class Symtab { } /// \} + /// Set the address class for the given address. + void SetAddressClass(lldb::addr_t addr, + lldb_private::AddressClass addr_class); + + /// Lookup the address class of the given address. + /// + /// \return + /// The address' class, if it is known, otherwise AddressClass::eCode. + lldb_private::AddressClass GetAddressClass(lldb::addr_t addr); + protected: typedef std::vector<Symbol> collection; typedef collection::iterator iterator; @@ -274,6 +286,9 @@ class Symtab { collection m_symbols; FileRangeToIndexMap m_file_addr_to_index; + /// The address class for each symbol in the elf file + FileAddressToAddressClassMap m_address_class_map; + /// Maps function names to symbol indices (grouped by FunctionNameTypes) std::map<lldb::FunctionNameType, UniqueCStringMap<uint32_t>> m_name_to_symbol_indices; diff --git a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp index 1646ee9aa34a6..9bfd05fddb4b0 100644 --- a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp +++ b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp @@ -769,17 +769,7 @@ AddressClass ObjectFileELF::GetAddressClass(addr_t file_addr) { if (res != AddressClass::eCode) return res; - auto ub = m_address_class_map.upper_bound(file_addr); - if (ub == m_address_class_map.begin()) { - // No entry in the address class map before the address. Return default - // address class for an address in a code section. - return AddressClass::eCode; - } - - // Move iterator to the address class entry preceding address - --ub; - - return ub->second; + return symtab->GetAddressClass(file_addr); } size_t ObjectFileELF::SectionIndex(const SectionHeaderCollIter &I) { @@ -2213,18 +2203,18 @@ unsigned ObjectFileELF::ParseSymbols(Symtab *symtab, user_id_t start_id, switch (mapping_symbol) { case 'a': // $a[.<any>]* - marks an ARM instruction sequence - m_address_class_map[symbol.st_value] = AddressClass::eCode; + symtab->SetAddressClass(symbol.st_value, AddressClass::eCode); break; case 'b': case 't': // $b[.<any>]* - marks a THUMB BL instruction sequence // $t[.<any>]* - marks a THUMB instruction sequence - m_address_class_map[symbol.st_value] = - AddressClass::eCodeAlternateISA; + symtab->SetAddressClass(symbol.st_value, + AddressClass::eCodeAlternateISA); break; case 'd': // $d[.<any>]* - marks a data item sequence (e.g. lit pool) - m_address_class_map[symbol.st_value] = AddressClass::eData; + symtab->SetAddressClass(symbol.st_value, AddressClass::eData); break; } } @@ -2238,11 +2228,11 @@ unsigned ObjectFileELF::ParseSymbols(Symtab *symtab, user_id_t start_id, switch (mapping_symbol) { case 'x': // $x[.<any>]* - marks an A64 instruction sequence - m_address_class_map[symbol.st_value] = AddressClass::eCode; + symtab->SetAddressClass(symbol.st_value, AddressClass::eCode); break; case 'd': // $d[.<any>]* - marks a data item sequence (e.g. lit pool) - m_address_class_map[symbol.st_value] = AddressClass::eData; + symtab->SetAddressClass(symbol.st_value, AddressClass::eData); break; } } @@ -2260,11 +2250,11 @@ unsigned ObjectFileELF::ParseSymbols(Symtab *symtab, user_id_t start_id, // conjunction with symbol.st_value to produce the final // symbol_value that we store in the symtab. symbol_value_offset = -1; - m_address_class_map[symbol.st_value ^ 1] = - AddressClass::eCodeAlternateISA; + symtab->SetAddressClass(symbol.st_value ^ 1, + AddressClass::eCodeAlternateISA); } else { // This address is ARM - m_address_class_map[symbol.st_value] = AddressClass::eCode; + symtab->SetAddressClass(symbol.st_value, AddressClass::eCode); } } } @@ -2285,17 +2275,19 @@ unsigned ObjectFileELF::ParseSymbols(Symtab *symtab, user_id_t start_id, */ if (arch.IsMIPS()) { if (IS_MICROMIPS(symbol.st_other)) - m_address_class_map[symbol.st_value] = AddressClass::eCodeAlternateISA; + symtab->SetAddressClass(symbol.st_value, + AddressClass::eCodeAlternateISA); else if ((symbol.st_value & 1) && (symbol_type == eSymbolTypeCode)) { symbol.st_value = symbol.st_value & (~1ull); - m_address_class_map[symbol.st_value] = AddressClass::eCodeAlternateISA; + symtab->SetAddressClass(symbol.st_value, + AddressClass::eCodeAlternateISA); } else { if (symbol_type == eSymbolTypeCode) - m_address_class_map[symbol.st_value] = AddressClass::eCode; + symtab->SetAddressClass(symbol.st_value, AddressClass::eCode); else if (symbol_type == eSymbolTypeData) - m_address_class_map[symbol.st_value] = AddressClass::eData; + symtab->SetAddressClass(symbol.st_value, AddressClass::eData); else - m_address_class_map[symbol.st_value] = AddressClass::eUnknown; + symtab->SetAddressClass(symbol.st_value, AddressClass::eUnknown); } } } @@ -3060,10 +3052,10 @@ void ObjectFileELF::ParseSymtab(Symtab &lldb_symtab) { if (arch.GetMachine() == llvm::Triple::arm && (entry_point_file_addr & 1)) { symbol.GetAddressRef().SetOffset(entry_point_addr.GetOffset() ^ 1); - m_address_class_map[entry_point_file_addr ^ 1] = - AddressClass::eCodeAlternateISA; + lldb_symtab.SetAddressClass(entry_point_file_addr ^ 1, + AddressClass::eCodeAlternateISA); } else { - m_address_class_map[entry_point_file_addr] = AddressClass::eCode; + lldb_symtab.SetAddressClass(entry_point_file_addr, AddressClass::eCode); } lldb_symtab.AddSymbol(symbol); } diff --git a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h index bc8e34981a9d8..1adc547d960f5 100644 --- a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h +++ b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.h @@ -187,9 +187,6 @@ class ObjectFileELF : public lldb_private::ObjectFile { typedef DynamicSymbolColl::iterator DynamicSymbolCollIter; typedef DynamicSymbolColl::const_iterator DynamicSymbolCollConstIter; - typedef std::map<lldb::addr_t, lldb_private::AddressClass> - FileAddressToAddressClassMap; - /// Version of this reader common to all plugins based on this class. static const uint32_t m_plugin_version = 1; static const uint32_t g_core_uuid_magic; @@ -227,9 +224,6 @@ class ObjectFileELF : public lldb_private::ObjectFile { /// The architecture detected from parsing elf file contents. lldb_private::ArchSpec m_arch_spec; - /// The address class for each symbol in the elf file - FileAddressToAddressClassMap m_address_class_map; - /// Returns the index of the given section header. size_t SectionIndex(const SectionHeaderCollIter &I); diff --git a/lldb/source/Symbol/Symtab.cpp b/lldb/source/Symbol/Symtab.cpp index 5b5bf5c3f6f8c..d7ba4f133acaa 100644 --- a/lldb/source/Symbol/Symtab.cpp +++ b/lldb/source/Symbol/Symtab.cpp @@ -1372,3 +1372,22 @@ bool Symtab::LoadFromCache() { SetWasLoadedFromCache(); return result; } + +void Symtab::SetAddressClass(lldb::addr_t addr, + lldb_private::AddressClass addr_class) { + m_address_class_map.insert_or_assign(addr, addr_class); +} + +lldb_private::AddressClass Symtab::GetAddressClass(lldb::addr_t addr) { + auto ub = m_address_class_map.upper_bound(addr); + if (ub == m_address_class_map.begin()) { + // No entry in the address class map before the address. Return default + // address class for an address in a code section. + return AddressClass::eCode; + } + + // Move iterator to the address class entry preceding address + --ub; + + return ub->second; +} _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
