[llvm-branch-commits] [lldb] [lldb] Store all XML register types in a single string map (PR #213887)
via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Tue Aug 4 02:33:47 PDT 2026
llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-lldb
Author: David Spickett (DavidSpickett)
<details>
<summary>Changes</summary>
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>
---
Full diff: https://github.com/llvm/llvm-project/pull/213887.diff
5 Files Affected:
- (modified) lldb/include/lldb/Utility/RegisterType.h (+6)
- (modified) lldb/include/lldb/Utility/RegisterTypeFlags.h (+11-3)
- (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (+64-70)
- (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h (+7-8)
- (modified) lldb/source/Utility/RegisterTypeFlags.cpp (+2-2)
``````````diff
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);
}
``````````
</details>
https://github.com/llvm/llvm-project/pull/213887
More information about the llvm-branch-commits
mailing list