[lldb] Store all XML register types in a single string map

[DavidSpickett]

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.

This highlighted that whereas for flags we would use the first flags
with a given ID and ignore all others with the same ID, we would
use the last for enums.

I have changed enums to handle duplicate IDs as flags do, and
expanded testing to cover this.

Note that because we parse all enums and then all flags, enums
always win over flags. This is probably not ideal because it's
not intuitive to the author of the XML document, but I do not
intended to change it at the moment.

We should also not be sharing IDs between <feature> elements,
but I'm not going to fix that here.

Tracking this in #214444.

---

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

[llvmorg-github-actions]

@llvm/pr-subscribers-lldb

Author: David Spickett (DavidSpickett)

Changes

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.

---

_{Stack created with <a href="https://github.com/github/gh-stack"&gt;GitHub Stacks CLI</a> • <a href="https://gh.io/stacks-feedback"&gt;Give Feedback 💬</a>}

---

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 --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>> &registers_enum_types) {
+static void
+ParseEnums(XMLNode feature_node,
+           llvm::StringMap<std::unique_ptr<RegisterType>> &register_types) {
   Log *log(GetLog(GDBRLog::Process));
 
   // The top level element is "<enum...".
   feature_node.ForEachChildElementWithName(
-      "enum", [log, &registers_enum_types](const XMLNode &enum_node) {
+      "enum", [log, &register_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>>
-                     &registers_enum_types) {
+static std::vector<RegisterTypeFlags::Field> ParseFlagsFields(
+    XMLNode flags_node, unsigned size,
+    const llvm::StringMap<std::unique_ptr<RegisterType>> &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,
-                                                   &registers_enum_types](
+                                                   &register_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>> &registers_flags_types,
-    const llvm::StringMap<std::unique_ptr<RegisterTypeEnum>>
-        &registers_enum_types) {
+    llvm::StringMap<std::unique_ptr<RegisterType>> &register_types) {
   Log *log(GetLog(GDBRLog::Process));
 
   feature_node.ForEachChildElementWithName(
-      "flags",
-      [&log, &registers_flags_types,
-       &registers_enum_types](const XMLNode &flags_node) -> bool {
+      "flags", [&log, &register_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> &registers,
-    llvm::StringMap<std::unique_ptr<RegisterTypeFlags>> &registers_flags_types,
-    llvm::StringMap<std::unique_ptr<RegisterTypeEnum>> &registers_enum_types) {
+    llvm::StringMap<std::unique_ptr<RegisterType>> &register_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 &register_type : register_types)
+    register_type.second->DumpToLog(log);
 
   feature_node.ForEachChildElementWithName(
       "reg",
-      [&target_info, &registers, &registers_flags_types,
+      [&target_info, &registers, &register_types,
        log](const XMLNode &reg_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);
 }

[barsolo2000]

Is insert_or_assign safe here now that all types share one map?
shouldn't it follow something similiar to ParseFlags where we explicitly preserves the first definition for exactly this lifetime reason? wouldn't leave some dangling pointers?

[DavidSpickett]

I think it is safe, for now, but you're right that it's suspicious and it wouldn't take much to break it.

We parse all enums first, then flags, so:

1. Nothing that could reference an enum is parsed by the point the enums are parsed.
2. I there are N enums with ID X, we are using the last one.

1 is hard to verify without knowing where to look, and 2 is the reverse of how we treat flags.

I will treat duplicate enum IDs the same as we do for flags.

[DavidSpickett]

And this parsing code needs a refactor at some point anyway, so it's likely to break sooner than later.

[DavidSpickett]

I have fixed this and added tests for it.

[barsolo2000]

The shared map is reused across features/includes, so valid XML can break:

1. Feature A defines enum T.
2. Feature B defines flags T and a register using it.
3. The enum occupies T; Feature B’s flags are discarded.

Could lookup be feature-local while ownership remains process wide?

[clayborg]

I am fine with keeping this simple. A GDB remote server should be returning valid data. Maybe we just want to emit an error to the debugger's output stream (and to a log) so the user can see when there are issues? So see a type, check if there is already a type and emit an error message with the XML output and just ignore any new types that have already been defined using that name?

[DavidSpickett]

https://sourceware.org/gdb/current/onlinedocs/gdb.html/Target-Description-Format.html

"Each type element must have an ‘id’ attribute, which gives a unique (within the containing ‘<feature>’) name to the type. Types must be defined before they are used."

So you are right we are not following this rule correctly.

Let's address this in a follow up PR along with the fact that id lookup (and collisions) should not care what element type has the ID.

[DavidSpickett]

A GDB remote server should be returning valid data.

I think our definition of valid is incorrect, but we can fix it. And you're right that most servers aren't going to get adventurous.

Maybe we just want to emit an error to the debugger's output stream (and to a log) so the user can see when there are issues? So see a type, check if there is already a type and emit an error message with the XML output and just ignore any new types that have already been defined using that name?

Currently we log them.

We could print them in the interface, or we could have a general "there were problems, check the log". I'll deal with that in a follow up if that's ok. The errors and logging are scattered throughout the GDB client code at the moment, so it's hard to gather it into one coherent report. So I should address that along the way.

[DavidSpickett]

There is precedent for telling the user - we print a warning if the server offers XML but we cannot parse it. So let's see if I can make the reporting more structured and issue a compact warning.

[github-actions]

🐧 Linux x64 Test Results

• 34158 tests passed
• 511 tests skipped

✅ The build succeeded and all tests passed.

[clayborg]

I am fine with keeping this simple. A GDB remote server should be returning valid data. Maybe we just want to emit an error to the debugger's output stream (and to a log) so the user can see when there are issues? So see a type, check if there is already a type and emit an error message with the XML output and just ignore any new types that have already been defined using that name?

[DavidSpickett]

If you are ok with addressing the incorrect parsing and the logging/warning in follow ups, this is ready to land I think.

[DavidSpickett]

I have added test cases to show the known issues with parsing.