[llvm-branch-commits] [lldb] [lldb] Store all XML register types in a single string map (PR #213887)
David Spickett via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Tue Aug 4 02:33:05 PDT 2026
https://github.com/DavidSpickett created https://github.com/llvm/llvm-project/pull/213887
We are assuming that their ID's are unique, so there's no need to keep
separate maps. We can do basic type checking by checking the kind of
the type pointed to.
A few more methods were added to the base RegisterType. GetSize()
returns 0 for enums because enums don't have a size until they are
used by a register. This is not ideal but it works for now.
---
<sub>Stack created with <a href="https://github.com/github/gh-stack">GitHub Stacks CLI</a> • <a href="https://gh.io/stacks-feedback">Give Feedback 💬</a></sub>
>From 23c4d2a462c1568e8faaaabeca0f0382d847f770 Mon Sep 17 00:00:00 2001
From: David Spickett <david.spickett at linaro.org>
Date: Tue, 3 Sep 2024 10:46:24 +0000
Subject: [PATCH] [lldb] Store all XML register types in a single string map
We are assuming that their ID's are unique, so there's no need to keep
separate maps. We can do basic type checking by checking the kind of
the type pointed to.
A few more methods were added to the base RegisterType. GetSize()
returns 0 for enums because enums don't have a size until they are
used by a register. This is not ideal but it works for now.
---
lldb/include/lldb/Utility/RegisterType.h | 6 +
lldb/include/lldb/Utility/RegisterTypeFlags.h | 14 +-
.../Process/gdb-remote/ProcessGDBRemote.cpp | 134 +++++++++---------
.../Process/gdb-remote/ProcessGDBRemote.h | 15 +-
lldb/source/Utility/RegisterTypeFlags.cpp | 4 +-
5 files changed, 90 insertions(+), 83 deletions(-)
diff --git a/lldb/include/lldb/Utility/RegisterType.h b/lldb/include/lldb/Utility/RegisterType.h
index 9ecd6c1dfb639..96551262268dd 100644
--- a/lldb/include/lldb/Utility/RegisterType.h
+++ b/lldb/include/lldb/Utility/RegisterType.h
@@ -51,6 +51,12 @@ class RegisterType {
m_dependencies = dependencies;
}
+ virtual void DumpToLog(Log *log) const = 0;
+
+ /// The size of the type in bytes. Return 0 if the size is unknown or context
+ /// specific.
+ virtual unsigned GetSize() const = 0;
+
private:
const RegisterTypeKind m_kind;
const std::string m_id;
diff --git a/lldb/include/lldb/Utility/RegisterTypeFlags.h b/lldb/include/lldb/Utility/RegisterTypeFlags.h
index b15e7e6999335..461c3e8e0e0e6 100644
--- a/lldb/include/lldb/Utility/RegisterTypeFlags.h
+++ b/lldb/include/lldb/Utility/RegisterTypeFlags.h
@@ -46,7 +46,15 @@ class RegisterTypeEnum : public RegisterType {
const Enumerators &GetEnumerators() const { return m_enumerators; }
- void DumpToLog(Log *log) const;
+ virtual void DumpToLog(Log *log) const override;
+
+ virtual unsigned GetSize() const override {
+ // Enums don't have a size until they are used by a specific register,
+ // so we return 0 just to be sure they don't end up attached directly to a
+ // register. We expect them to only be used by flags, then the flags are
+ // attached to the register.
+ return 0;
+ }
virtual void ToXMLElement(Stream &strm,
const RegisterType *user = nullptr) const override;
@@ -163,9 +171,9 @@ class RegisterTypeFlags : public RegisterType {
}
const std::vector<Field> &GetFields() const { return m_fields; }
- unsigned GetSize() const { return m_size; }
+ virtual unsigned GetSize() const override { return m_size; }
- void DumpToLog(Log *log) const;
+ virtual void DumpToLog(Log *log) const override;
/// Produce a text table showing the layout of all the fields. Unnamed/padding
/// fields will be included, with only their positions shown.
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index f704106822d75..1da65bc7005e2 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -5005,14 +5005,14 @@ 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,
+ llvm::StringMap<std::unique_ptr<RegisterType>> ®ister_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](const XMLNode &enum_node) {
std::string id;
enum_node.ForEachAttribute([&id](const llvm::StringRef &attr_name,
@@ -5040,7 +5040,7 @@ static void ParseEnums(
LLDB_LOG(log,
"ProcessGDBRemote::ParseEnums Found enum type \"{0}\"",
id);
- registers_enum_types.insert_or_assign(
+ register_types.insert_or_assign(
id, std::make_unique<RegisterTypeEnum>(id, enumerators));
}
}
@@ -5050,17 +5050,16 @@ static void ParseEnums(
});
}
-static std::vector<RegisterTypeFlags::Field>
-ParseFlagsFields(XMLNode flags_node, unsigned size,
- const llvm::StringMap<std::unique_ptr<RegisterTypeEnum>>
- ®isters_enum_types) {
+static std::vector<RegisterTypeFlags::Field> ParseFlagsFields(
+ XMLNode flags_node, unsigned size,
+ const llvm::StringMap<std::unique_ptr<RegisterType>> ®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;
@@ -5141,35 +5140,39 @@ ParseFlagsFields(XMLNode flags_node, unsigned size,
"that has size > 64 bits, this is not supported",
name->data());
else {
- // A field's type may be set to the name of an enum type.
+ // A field's type may be set to another previously defined type.
+ // Right now we only support enum.
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();
-
- // 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;
+ auto found = register_types.find(*type);
+ if (found != register_types.end()) {
+ enum_type = llvm::dyn_cast<RegisterTypeEnum>(found->second.get());
+ if (enum_type) {
+ // 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;
+ }
}
}
} else {
- LLDB_LOG(log,
- "ProcessGDBRemote::ParseFlagsFields Could not find type "
- "\"{0}\" "
- "for field \"{1}\", ignoring",
- type->data(), name->data());
+ LLDB_LOG(
+ log,
+ "ProcessGDBRemote::ParseFlagsFields Could not find enum type "
+ "\"{0}\" "
+ "for field \"{1}\", ignoring",
+ type->data(), name->data());
}
}
@@ -5186,15 +5189,11 @@ 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) {
+ llvm::StringMap<std::unique_ptr<RegisterType>> ®ister_types) {
Log *log(GetLog(GDBRLog::Process));
feature_node.ForEachChildElementWithName(
- "flags",
- [&log, ®isters_flags_types,
- ®isters_enum_types](const XMLNode &flags_node) -> bool {
+ "flags", [&log, ®ister_types](const XMLNode &flags_node) -> bool {
LLDB_LOG(log, "ProcessGDBRemote::ParseFlags Found flags node \"{0}\"",
flags_node.GetAttributeValue("id").c_str());
@@ -5227,7 +5226,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());
@@ -5240,26 +5239,27 @@ void ParseFlags(
// If no fields overlap, use them.
if (overlap == fields.end()) {
- if (registers_flags_types.contains(*id)) {
+ if (register_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
+ // register then reuse the ID. 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
+ // expensive and difficult. This means that pointers to types
+ // in the register info remain valid if later the ID is reused.
+ // If we allowed redefinitions, LLDB would crash
// when you tried to print a register that used the original
// definition.
LLDB_LOG(
log,
- "ProcessGDBRemote::ParseFlags Definition of flags "
+ "ProcessGDBRemote::ParseFlags Definition of flags with ID "
"\"{0}\" shadows "
- "previous definition, using original definition instead.",
+ "previous use of that ID, using original definition "
+ "instead.",
id->data());
} else {
- registers_flags_types.insert_or_assign(
+ register_types.insert_or_assign(
*id, std::make_unique<RegisterTypeFlags>(
id->str(), *size, std::move(fields)));
}
@@ -5292,25 +5292,21 @@ 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) {
+ llvm::StringMap<std::unique_ptr<RegisterType>> ®ister_types) {
if (!feature_node)
return false;
Log *log(GetLog(GDBRLog::Process));
// 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);
+ ParseFlags(feature_node, register_types);
+ for (const auto ®ister_type : register_types)
+ register_type.second->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;
@@ -5389,19 +5385,19 @@ 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;
+ llvm::StringMap<std::unique_ptr<RegisterType>>::iterator it =
+ register_types.find(gdb_type);
+ if (it != register_types.end()) {
+ auto register_type = it->second.get();
+ if (reg_info.byte_size == register_type->GetSize())
+ reg_info.register_type = register_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(),
+ register_type->GetID().c_str(), register_type->GetSize(),
reg_info.name, reg_info.byte_size);
}
@@ -5571,8 +5567,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) {
@@ -5648,8 +5643,7 @@ llvm::Error ProcessGDBRemote::GetGDBServerRegisterInfo(ArchSpec &arch_to_use) {
// 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();
+ m_register_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..89bd9605710bb 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -560,19 +560,18 @@ 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
+ // Lists of register types generated from the remote's target XML.
+ // Pointers to these RegisterTypes 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;
+ // The key is the XML ID of the type. The kind of element does not play a part
+ // here, the XML author should use unique global IDs.
+ // RegisterTypes may contain pointers to other RegisterTypes, but they will
+ // not attempt to destroy those types when they themselves destruct.
+ llvm::StringMap<std::unique_ptr<RegisterType>> m_register_types;
};
} // namespace process_gdb_remote
diff --git a/lldb/source/Utility/RegisterTypeFlags.cpp b/lldb/source/Utility/RegisterTypeFlags.cpp
index 7c6ba6ef6d3ef..214c97855ba2a 100644
--- a/lldb/source/Utility/RegisterTypeFlags.cpp
+++ b/lldb/source/Utility/RegisterTypeFlags.cpp
@@ -162,7 +162,7 @@ RegisterTypeFlags::RegisterTypeFlags(std::string id, unsigned size,
}
void RegisterTypeFlags::DumpToLog(Log *log) const {
- LLDB_LOG(log, "ID: \"{0}\" Size: {1}", GetID().c_str(), m_size);
+ LLDB_LOG(log, "flags ID: \"{0}\" Size: {1}", GetID().c_str(), m_size);
for (const Field &field : m_fields)
field.DumpToLog(log);
}
@@ -376,7 +376,7 @@ void RegisterTypeEnum::Enumerator::DumpToLog(Log *log) const {
}
void RegisterTypeEnum::DumpToLog(Log *log) const {
- LLDB_LOG(log, "ID: \"{0}\"", GetID().c_str());
+ LLDB_LOG(log, "enum ID: \"{0}\"", GetID().c_str());
for (const auto &enumerator : GetEnumerators())
enumerator.DumpToLog(log);
}
More information about the llvm-branch-commits
mailing list