[Lldb-commits] [lldb] bc8a305 - [lldb] Scope XML register type IDs to features (#216384)
via lldb-commits
lldb-commits at lists.llvm.org
Tue Aug 18 09:56:25 PDT 2026
Author: Bar Soloveychik
Date: 2026-08-18T09:56:20-07:00
New Revision: bc8a305ac6872b3149526fd253e0ac9f06b6aa2c
URL: https://github.com/llvm/llvm-project/commit/bc8a305ac6872b3149526fd253e0ac9f06b6aa2c
DIFF: https://github.com/llvm/llvm-project/commit/bc8a305ac6872b3149526fd253e0ac9f06b6aa2c.diff
LOG: [lldb] Scope XML register type IDs to features (#216384)
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.
Relates to #214444.
---------
Co-authored-by: Bar Soloveychik <barsolo at fb.com>
Added:
Modified:
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py
Removed:
################################################################################
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index d8005ebfcea72..ff09740e637a3 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -4942,6 +4942,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));
@@ -5009,14 +5011,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 &feature_register_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, &feature_register_types,
+ &owned_register_types](const XMLNode &enum_node) {
std::string id;
enum_node.ForEachAttribute([&id](const llvm::StringRef &attr_name,
@@ -5044,8 +5047,27 @@ 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] =
+ feature_register_types.try_emplace(id, enum_type_ptr);
+ if (inserted) {
+ owned_register_types.push_back(std::move(enum_type));
+ } else if (llvm::isa<RegisterTypeEnum>(it->second)) {
+ // Preserve the existing behavior where the last valid enum with
+ // a repeated ID wins. All enums are parsed before flags, so no
+ // fields can reference the enum being replaced yet. The earlier
+ // object remains owned; only the feature lookup is updated.
+ 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);
+ }
}
}
@@ -5056,15 +5078,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 &feature_register_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](
+ &feature_register_types](
const XMLNode
&field_node) {
std::optional<llvm::StringRef> name;
@@ -5148,24 +5169,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 = feature_register_types.find(*type);
+ if (found != feature_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 {
@@ -5189,16 +5219,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 &feature_register_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, &feature_register_types,
+ &owned_register_types](const XMLNode &flags_node) -> bool {
LLDB_LOG(log, "ProcessGDBRemote::ParseFlags Found flags node \"{0}\"",
flags_node.GetAttributeValue("id").c_str());
@@ -5231,7 +5259,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, feature_register_types);
if (fields.size()) {
// Sort so that the fields with the MSBs are first.
std::sort(fields.rbegin(), fields.rend());
@@ -5244,28 +5272,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
diff icult. 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 (feature_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));
+ feature_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.
@@ -5296,25 +5316,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 feature_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, feature_register_types, owned_register_types);
+ for (const auto ®ister_type : feature_register_types)
+ if (const auto *enum_type =
+ llvm::dyn_cast<RegisterTypeEnum>(register_type.second))
+ enum_type->DumpToLog(log);
+
+ ParseFlags(feature_node, feature_register_types, owned_register_types);
+ for (const auto ®ister_type : feature_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, &feature_register_types,
log](const XMLNode ®_node) -> bool {
std::string gdb_group;
std::string gdb_type;
@@ -5393,20 +5417,21 @@ 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 = feature_register_types.find(gdb_type);
+ if (it != feature_register_types.end()) {
+ if (const auto *flags_type =
+ llvm::dyn_cast<RegisterTypeFlags>(it->second)) {
+ 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
@@ -5575,8 +5600,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) {
@@ -5646,14 +5670,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 f132858e14ca7..7a7857a12afbe 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..9f10fd5172d4a 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,51 @@ 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", substrs=["(field_a = enum_a)"])
+ self.expect("register read cpsr", substrs=["(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 +618,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_type"/>
</flags>
<reg name="pc" bitsize="64"/>
- <reg name="x0" regnum="0" bitsize="64" type="my_flags"/>
+ <reg name="x0" regnum="0" bitsize="64" type="x0_flags"/>
</feature>"""
),
- # The my_flags here is ignored, so x1 will use the my_flags from above.
"core-2.xml": dedent(
"""\
<?xml version="1.0"?>
- <feature name="org.gnu.gdb.aarch64.core">
- <flags id="my_flags" size="8">
+ <feature name="org.gnu.gdb.aarch64.system">
+ <flags id="shared_type" size="4">
<field name="incorrect" start="0" end="0"/>
</flags>
- <reg name="x1" regnum="33" bitsize="64" type="my_flags"/>
+ <reg name="cpsr" regnum="33" bitsize="32"
+ type="shared_type"/>
</feature>
"""
),
}
)
- self.expect("register read x0", substrs=["(correct = 1)"])
- self.expect("register read x1", substrs=["(correct = 1)"])
+ self.expect("register read x0", substrs=["(field = correct)"])
+ self.expect("register read cpsr", substrs=["(incorrect = 1)"])
@skipIfXmlSupportMissing
@skipIfRemote
More information about the lldb-commits
mailing list