[Lldb-commits] [lldb] [lldb] Scope XML register type IDs to features (PR #216384)
Bar Soloveychik via lldb-commits
lldb-commits at lists.llvm.org
Fri Aug 14 11:59:46 PDT 2026
https://github.com/barsolo2000 created https://github.com/llvm/llvm-project/pull/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.
>From b6cb03b8ab28e378179b44a5839330d6e3236e15 Mon Sep 17 00:00:00 2001
From: Bar Soloveychik <barsolo at fb.com>
Date: Fri, 14 Aug 2026 10:33:18 -0700
Subject: [PATCH] [lldb] Scope XML register type IDs to features
---
.../Process/gdb-remote/ProcessGDBRemote.cpp | 178 +++++++++---------
.../Process/gdb-remote/ProcessGDBRemote.h | 20 +-
.../gdb_remote_client/TestXMLRegisterFlags.py | 76 ++++++--
3 files changed, 161 insertions(+), 113 deletions(-)
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_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