llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Bar Soloveychik (barsolo2000) <details> <summary>Changes</summary> This change separates XML register-type lookup from ownership: - Uses a temporary type registry for each `<feature>`, allowing IDs to be reused safely across features. - Keeps all parsed types alive in process-owned storage. - Uses one common registry for enums, flags, and future vector/union types. - Prevents stale enum pointers and cross-feature type reuse. --- Patch is 21.37 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/216384.diff 3 Files Affected: - (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (+94-84) - (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h (+5-15) - (modified) lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py (+62-14) ``````````diff diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp index bed8f936ec888..cd787671f6571 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -4941,6 +4941,8 @@ struct GdbServerTargetInfo { RegisterSetMap reg_set_map; }; +using RegisterTypeMap = llvm::StringMap<const RegisterType *>; + static RegisterTypeEnum::Enumerators ParseEnumEvalues(const XMLNode &enum_node) { Log *log(GetLog(GDBRLog::Process)); @@ -5008,14 +5010,15 @@ ParseEnumEvalues(const XMLNode &enum_node) { return final_enumerators; } -static void ParseEnums( - XMLNode feature_node, - llvm::StringMap<std::unique_ptr<RegisterTypeEnum>> ®isters_enum_types) { +static void +ParseEnums(XMLNode feature_node, RegisterTypeMap ®ister_types, + std::vector<std::unique_ptr<RegisterType>> &owned_register_types) { Log *log(GetLog(GDBRLog::Process)); // The top level element is "<enum...". feature_node.ForEachChildElementWithName( - "enum", [log, ®isters_enum_types](const XMLNode &enum_node) { + "enum", + [log, ®ister_types, &owned_register_types](const XMLNode &enum_node) { std::string id; enum_node.ForEachAttribute([&id](const llvm::StringRef &attr_name, @@ -5043,8 +5046,20 @@ static void ParseEnums( LLDB_LOG(log, "ProcessGDBRemote::ParseEnums Found enum type \"{0}\"", id); - registers_enum_types.insert_or_assign( - id, std::make_unique<RegisterTypeEnum>(id, enumerators)); + auto enum_type = + std::make_unique<RegisterTypeEnum>(id, enumerators); + const RegisterTypeEnum *enum_type_ptr = enum_type.get(); + auto [it, inserted] = register_types.try_emplace(id, enum_type_ptr); + if (inserted || llvm::isa<RegisterTypeEnum>(it->second)) { + owned_register_types.push_back(std::move(enum_type)); + it->second = enum_type_ptr; + } else { + LLDB_LOG( + log, + "ProcessGDBRemote::ParseEnums Ignoring enum type \"{0}\" " + "because another type with that id already exists", + id); + } } } @@ -5055,15 +5070,14 @@ static void ParseEnums( static std::vector<RegisterTypeFlags::Field> ParseFlagsFields(XMLNode flags_node, unsigned size, - const llvm::StringMap<std::unique_ptr<RegisterTypeEnum>> - ®isters_enum_types) { + const RegisterTypeMap ®ister_types) { Log *log(GetLog(GDBRLog::Process)); const unsigned max_start_bit = size * 8 - 1; // Process the fields of this set of flags. std::vector<RegisterTypeFlags::Field> fields; flags_node.ForEachChildElementWithName("field", [&fields, max_start_bit, &log, - ®isters_enum_types]( + ®ister_types]( const XMLNode &field_node) { std::optional<llvm::StringRef> name; @@ -5147,24 +5161,33 @@ ParseFlagsFields(XMLNode flags_node, unsigned size, // A field's type may be set to the name of an enum type. const RegisterTypeEnum *enum_type = nullptr; if (type && !type->empty()) { - auto found = registers_enum_types.find(*type); - if (found != registers_enum_types.end()) { - enum_type = found->second.get(); + auto found = register_types.find(*type); + if (found != register_types.end()) { + enum_type = llvm::dyn_cast<RegisterTypeEnum>(found->second); + + if (!enum_type) { + LLDB_LOG(log, + "ProcessGDBRemote::ParseFlagsFields Type \"{0}\" for " + "field \"{1}\" is not an enum, ignoring", + type->data(), name->data()); + } // No enumerator can exceed the range of the field itself. - uint64_t max_value = - RegisterTypeFlags::Field::GetMaxValue(*start, *end); - for (const auto &enumerator : enum_type->GetEnumerators()) { - if (enumerator.m_value > max_value) { - enum_type = nullptr; - LLDB_LOG( - log, - "ProcessGDBRemote::ParseFlagsFields In enum \"{0}\" " - "evalue \"{1}\" with value {2} exceeds the maximum value " - "of field \"{3}\" ({4}), ignoring enum", - type->data(), enumerator.m_name, enumerator.m_value, - name->data(), max_value); - break; + if (enum_type) { + uint64_t max_value = + RegisterTypeFlags::Field::GetMaxValue(*start, *end); + for (const auto &enumerator : enum_type->GetEnumerators()) { + if (enumerator.m_value > max_value) { + enum_type = nullptr; + LLDB_LOG( + log, + "ProcessGDBRemote::ParseFlagsFields In enum \"{0}\" " + "evalue \"{1}\" with value {2} exceeds the maximum " + "value of field \"{3}\" ({4}), ignoring enum", + type->data(), enumerator.m_name, enumerator.m_value, + name->data(), max_value); + break; + } } } } else { @@ -5188,16 +5211,14 @@ ParseFlagsFields(XMLNode flags_node, unsigned size, } void ParseFlags( - XMLNode feature_node, - llvm::StringMap<std::unique_ptr<RegisterTypeFlags>> ®isters_flags_types, - const llvm::StringMap<std::unique_ptr<RegisterTypeEnum>> - ®isters_enum_types) { + XMLNode feature_node, RegisterTypeMap ®ister_types, + std::vector<std::unique_ptr<RegisterType>> &owned_register_types) { Log *log(GetLog(GDBRLog::Process)); feature_node.ForEachChildElementWithName( "flags", - [&log, ®isters_flags_types, - ®isters_enum_types](const XMLNode &flags_node) -> bool { + [&log, ®ister_types, + &owned_register_types](const XMLNode &flags_node) -> bool { LLDB_LOG(log, "ProcessGDBRemote::ParseFlags Found flags node \"{0}\"", flags_node.GetAttributeValue("id").c_str()); @@ -5230,7 +5251,7 @@ void ParseFlags( if (id && size) { // Process the fields of this set of flags. std::vector<RegisterTypeFlags::Field> fields = - ParseFlagsFields(flags_node, *size, registers_enum_types); + ParseFlagsFields(flags_node, *size, register_types); if (fields.size()) { // Sort so that the fields with the MSBs are first. std::sort(fields.rbegin(), fields.rend()); @@ -5243,28 +5264,20 @@ void ParseFlags( // If no fields overlap, use them. if (overlap == fields.end()) { - if (registers_flags_types.contains(*id)) { - // In theory you could define some flag set, use it with a - // register then redefine it. We do not know if anyone does - // that, or what they would expect to happen in that case. - // - // LLDB chooses to take the first definition and ignore the rest - // as waiting until everything has been processed is more - // expensive and difficult. This means that pointers to flag - // sets in the register info remain valid if later the flag set - // is redefined. If we allowed redefinitions, LLDB would crash - // when you tried to print a register that used the original - // definition. + if (register_types.contains(*id)) { + // Type IDs must be unique within a feature. Keep the type that + // was already registered by the enum and flags parsing passes. LLDB_LOG( log, - "ProcessGDBRemote::ParseFlags Definition of flags " - "\"{0}\" shadows " - "previous definition, using original definition instead.", + "ProcessGDBRemote::ParseFlags Definition of flags \"{0}\" " + "conflicts with an existing type, ignoring this " + "definition.", id->data()); } else { - registers_flags_types.insert_or_assign( - *id, std::make_unique<RegisterTypeFlags>( - id->str(), *size, std::move(fields))); + auto flags_type = std::make_unique<RegisterTypeFlags>( + id->str(), *size, std::move(fields)); + register_types.try_emplace(*id, flags_type.get()); + owned_register_types.push_back(std::move(flags_type)); } } else { // If any fields overlap, ignore the whole set of flags. @@ -5295,25 +5308,29 @@ void ParseFlags( bool ParseRegisters( XMLNode feature_node, GdbServerTargetInfo &target_info, std::vector<DynamicRegisterInfo::Register> ®isters, - llvm::StringMap<std::unique_ptr<RegisterTypeFlags>> ®isters_flags_types, - llvm::StringMap<std::unique_ptr<RegisterTypeEnum>> ®isters_enum_types) { + std::vector<std::unique_ptr<RegisterType>> &owned_register_types) { if (!feature_node) return false; Log *log(GetLog(GDBRLog::Process)); + RegisterTypeMap register_types; // Enums first because they are referenced by fields in the flags. - ParseEnums(feature_node, registers_enum_types); - for (const auto &enum_type : registers_enum_types) - enum_type.second->DumpToLog(log); - - ParseFlags(feature_node, registers_flags_types, registers_enum_types); - for (const auto &flags : registers_flags_types) - flags.second->DumpToLog(log); + ParseEnums(feature_node, register_types, owned_register_types); + for (const auto ®ister_type : register_types) + if (const auto *enum_type = + llvm::dyn_cast<RegisterTypeEnum>(register_type.second)) + enum_type->DumpToLog(log); + + ParseFlags(feature_node, register_types, owned_register_types); + for (const auto ®ister_type : register_types) + if (const auto *flags_type = + llvm::dyn_cast<RegisterTypeFlags>(register_type.second)) + flags_type->DumpToLog(log); feature_node.ForEachChildElementWithName( "reg", - [&target_info, ®isters, ®isters_flags_types, + [&target_info, ®isters, ®ister_types, log](const XMLNode ®_node) -> bool { std::string gdb_group; std::string gdb_type; @@ -5392,20 +5409,22 @@ bool ParseRegisters( if (!gdb_type.empty()) { // gdb_type could reference some flags type defined in XML. - llvm::StringMap<std::unique_ptr<RegisterTypeFlags>>::iterator it = - registers_flags_types.find(gdb_type); - if (it != registers_flags_types.end()) { - auto flags_type = it->second.get(); - if (reg_info.byte_size == flags_type->GetSize()) - reg_info.register_type = flags_type; - else - LLDB_LOG( - log, - "ProcessGDBRemote::ParseRegisters Size of register flags {0} " - "({1} bytes) for register {2} does not match the register " - "size ({3} bytes). Ignoring this set of flags.", - flags_type->GetID().c_str(), flags_type->GetSize(), - reg_info.name, reg_info.byte_size); + auto it = register_types.find(gdb_type); + if (it != register_types.end()) { + const auto *flags_type = + llvm::dyn_cast<RegisterTypeFlags>(it->second); + if (flags_type) { + if (reg_info.byte_size == flags_type->GetSize()) + reg_info.register_type = flags_type; + else + LLDB_LOG( + log, + "ProcessGDBRemote::ParseRegisters Size of register flags " + "{0} ({1} bytes) for register {2} does not match the " + "register size ({3} bytes). Ignoring this set of flags.", + flags_type->GetID().c_str(), flags_type->GetSize(), + reg_info.name, reg_info.byte_size); + } } // There's a slim chance that the gdb_type name is both a flags type @@ -5574,8 +5593,7 @@ bool ProcessGDBRemote::GetGDBServerRegisterInfoXMLAndProcess( if (arch_to_use.IsValid()) { for (auto &feature_node : feature_nodes) { - ParseRegisters(feature_node, target_info, registers, - m_registers_flags_types, m_registers_enum_types); + ParseRegisters(feature_node, target_info, registers, m_register_types); } for (const auto &include : target_info.includes) { @@ -5645,14 +5663,6 @@ llvm::Error ProcessGDBRemote::GetGDBServerRegisterInfo(ArchSpec &arch_to_use) { "the debug server supports \"qXfer:features:read\", but LLDB does not " "have XML parsing enabled (check LLLDB_ENABLE_LIBXML2)"); - // These hold register type information for the whole of target.xml. - // target.xml may include further documents that - // GetGDBServerRegisterInfoXMLAndProcess will recurse to fetch and process. - // That's why we clear the cache here, and not in - // GetGDBServerRegisterInfoXMLAndProcess. To prevent it being cleared on every - // include read. - m_registers_flags_types.clear(); - m_registers_enum_types.clear(); std::vector<DynamicRegisterInfo::Register> registers; if (GetGDBServerRegisterInfoXMLAndProcess(arch_to_use, "target.xml", registers) && diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h index 4b60f9c662910..3042e212d4b77 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h @@ -11,6 +11,7 @@ #include <atomic> #include <map> +#include <memory> #include <mutex> #include <optional> #include <string> @@ -28,7 +29,7 @@ #include "lldb/Utility/Broadcaster.h" #include "lldb/Utility/ConstString.h" #include "lldb/Utility/GDBRemote.h" -#include "lldb/Utility/RegisterTypeFlags.h" +#include "lldb/Utility/RegisterType.h" #include "lldb/Utility/Status.h" #include "lldb/Utility/StreamString.h" #include "lldb/Utility/StringExtractor.h" @@ -40,7 +41,6 @@ #include "GDBRemoteRegisterContext.h" #include "llvm/ADT/DenseMap.h" -#include "llvm/ADT/StringMap.h" namespace lldb_private { namespace process_gdb_remote { @@ -560,19 +560,9 @@ class ProcessGDBRemote : public Process, void ParseExpeditedRegisters(ExpeditedRegisterMap &expedited_register_map, lldb::ThreadSP thread_sp); - // Lists of register fields generated from the remote's target XML. - // Pointers to these RegisterTypeFlags will be set in the register info passed - // back to the upper levels of lldb. Doing so is safe because this class will - // live at least as long as the debug session. We therefore do not store the - // data directly in the map because the map may reallocate it's storage as new - // entries are added. Which would invalidate any pointers set in the register - // info up to that point. - llvm::StringMap<std::unique_ptr<RegisterTypeFlags>> m_registers_flags_types; - - // Enum types are referenced by register fields. This does not store the data - // directly because the map may reallocate. Pointers to these are contained - // within instances of RegisterTypeFlags. - llvm::StringMap<std::unique_ptr<RegisterTypeEnum>> m_registers_enum_types; + // RegisterInfo and nested register types contain non-owning pointers to these + // objects. Keep every parsed type alive for the lifetime of this process. + std::vector<std::unique_ptr<RegisterType>> m_register_types; }; } // namespace process_gdb_remote diff --git a/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py b/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py index f600f807a787c..1bebbcbe576e8 100644 --- a/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py +++ b/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py @@ -547,7 +547,7 @@ def test_xml_includes_multiple(self): "core-2.xml": dedent( """\ <?xml version="1.0"?> - <feature name="org.gnu.gdb.aarch64.core"> + <feature name="org.gnu.gdb.aarch64.system"> <flags id="cpsr_flags" size="4"> <field name="C" start="0" end="0"/> </flags> @@ -562,7 +562,53 @@ def test_xml_includes_multiple(self): @skipIfXmlSupportMissing @skipIfRemote - def test_xml_includes_flags_redefined(self): + def test_xml_type_ids_scoped_to_feature(self): + self.setup_multidoc_test( + { + "target.xml": dedent( + """\ + <?xml version="1.0"?> + <target version="1.0"> + <architecture>aarch64</architecture> + <feature name="feature.a"> + <enum id="shared_enum" size="8"> + <evalue name="enum_a" value="1"/> + </enum> + <flags id="shared_flags" size="8"> + <field name="field_a" start="0" end="0" + type="shared_enum"/> + </flags> + <reg name="x0" regnum="0" bitsize="64" + type="shared_flags"/> + </feature> + <feature name="feature.b"> + <enum id="shared_enum" size="4"> + <evalue name="enum_b" value="1"/> + </enum> + <flags id="shared_flags" size="4"> + <field name="field_b" start="0" end="0" + type="shared_enum"/> + </flags> + <reg name="cpsr" regnum="33" bitsize="32" + type="shared_flags"/> + </feature> + <feature name="feature.c"> + <reg name="pc" bitsize="64" type="shared_flags"/> + </feature> + </target>""" + ), + } + ) + + self.expect( + "register read x0 cpsr", + substrs=["(field_a = enum_a)", "(field_b = enum_b)"], + ) + self.expect("register read pc", substrs=["("], matching=False) + + @skipIfXmlSupportMissing + @skipIfRemote + def test_xml_type_kinds_scoped_to_included_feature(self): self.setup_multidoc_test( { "target.xml": dedent( @@ -574,37 +620,39 @@ def test_xml_includes_flags_redefined(self): <xi:include href="core-2.xml"/> </target>""" ), - # Treating xi:include as a textual include, my_flags is first defined - # in core.xml. The second definition in core-2.xml - # is ignored. + # Type IDs are local to the feature that defines them. "core.xml": dedent( """\ <?xml version="1.0"?> <feature name="org.gnu.gdb.aarch64.core"> - <flags id="my_flags" size="8"> - <field name="correct" start="0" end="0"/> + <enum id="shared_type" size="8"> + <evalue name="correct" value="1"/> + </enum> + <flags id="x0_flags" size="8"> + <field name="field" start="0" end="0" + type="shared_... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/216384 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
