Reland "[lldb] Parse and display register field enums" (#97258)" (#97270)

This reverts commit d9e659c538516036e40330b6a98160cbda4ff100.

I could not reproduce the Mac OS ASAN failure locally but I narrowed it
down to the test `test_many_fields_same_enum`. This test shares an enum
between x0, which is 64 bit, and cpsr, which is 32 bit.

My theory is that when it does `register read x0`, an enum type is
created where the undlerying enumerators are 64 bit, matching the
register size.

Then it does `register read cpsr` which used the cached enum type, but
this register is 32 bit. This caused lldb to try to read an 8 byte value
out of a 4 byte allocation:
READ of size 8 at 0x60200014b874 thread T0
<...>
=>0x60200014b800: fa fa fd fa fa fa fd fa fa fa fd fa fa fa[04]fa

To fix this I've added the register's size in bytes to the constructed
enum type's name. This means that x0 uses:
__lldb_register_fields_enum_some_enum_8
And cpsr uses:
__lldb_register_fields_enum_some_enum_4

If any other registers use this enum and are read, they will use the
cached type as long as their size matches, otherwise we make a new type.

lldb/include/lldb/Target/RegisterFlags.h

@@ -32,10 +32,15 @@ public:
         : m_value(value), m_name(std::move(name)) {}
 
     void ToXML(Stream &strm) const;
+
+    void DumpToLog(Log *log) const;
   };
 
   typedef std::vector<Enumerator> Enumerators;
 
+  // GDB also includes a "size" that is the size of the underlying register.
+  // We will not store that here but instead use the size of the register
+  // this gets attached to when emitting XML.
   FieldEnum(std::string id, const Enumerators &enumerators);
 
   const Enumerators &GetEnumerators() const { return m_enumerators; }
@@ -44,6 +49,8 @@ public:
 
   void ToXML(Stream &strm, unsigned size) const;
 
+  void DumpToLog(Log *log) const;
+
 private:
   std::string m_id;
   Enumerators m_enumerators;

lldb/source/Core/DumpRegisterInfo.cpp

@@ -111,6 +111,11 @@ void lldb_private::DoDumpRegisterInfo(
   };
   DumpList(strm, "    In sets: ", in_sets, emit_set);
 
-  if (flags_type)
+  if (flags_type) {
     strm.Printf("\n\n%s", flags_type->AsTable(terminal_width).c_str());
+
+    std::string enumerators = flags_type->DumpEnums(terminal_width);
+    if (enumerators.size())
+      strm << "\n\n" << enumerators;
+  }
 }

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

@@ -4179,21 +4179,134 @@ struct GdbServerTargetInfo {
   RegisterSetMap reg_set_map;
 };
 
-static std::vector<RegisterFlags::Field> ParseFlagsFields(XMLNode flags_node,
-                                                          unsigned size) {
+static FieldEnum::Enumerators ParseEnumEvalues(const XMLNode &enum_node) {
+  Log *log(GetLog(GDBRLog::Process));
+  // We will use the last instance of each value. Also we preserve the order
+  // of declaration in the XML, as it may not be numerical.
+  // For example, hardware may intially release with two states that softwware
+  // can read from a register field:
+  // 0 = startup, 1 = running
+  // If in a future hardware release, the designers added a pre-startup state:
+  // 0 = startup, 1 = running, 2 = pre-startup
+  // Now it makes more sense to list them in this logical order as opposed to
+  // numerical order:
+  // 2 = pre-startup, 1 = startup, 0 = startup
+  // This only matters for "register info" but let's trust what the server
+  // chose regardless.
+  std::map<uint64_t, FieldEnum::Enumerator> enumerators;
+
+  enum_node.ForEachChildElementWithName(
+      "evalue", [&enumerators, &log](const XMLNode &enumerator_node) {
+        std::optional<llvm::StringRef> name;
+        std::optional<uint64_t> value;
+
+        enumerator_node.ForEachAttribute(
+            [&name, &value, &log](const llvm::StringRef &attr_name,
+                                  const llvm::StringRef &attr_value) {
+              if (attr_name == "name") {
+                if (attr_value.size())
+                  name = attr_value;
+                else
+                  LLDB_LOG(log, "ProcessGDBRemote::ParseEnumEvalues "
+                                "Ignoring empty name in evalue");
+              } else if (attr_name == "value") {
+                uint64_t parsed_value = 0;
+                if (llvm::to_integer(attr_value, parsed_value))
+                  value = parsed_value;
+                else
+                  LLDB_LOG(log,
+                           "ProcessGDBRemote::ParseEnumEvalues "
+                           "Invalid value \"{0}\" in "
+                           "evalue",
+                           attr_value.data());
+              } else
+                LLDB_LOG(log,
+                         "ProcessGDBRemote::ParseEnumEvalues Ignoring "
+                         "unknown attribute "
+                         "\"{0}\" in evalue",
+                         attr_name.data());
+
+              // Keep walking attributes.
+              return true;
+            });
+
+        if (value && name)
+          enumerators.insert_or_assign(
+              *value, FieldEnum::Enumerator(*value, name->str()));
+
+        // Find all evalue elements.
+        return true;
+      });
+
+  FieldEnum::Enumerators final_enumerators;
+  for (auto [_, enumerator] : enumerators)
+    final_enumerators.push_back(enumerator);
+
+  return final_enumerators;
+}
+
+static void
+ParseEnums(XMLNode feature_node,
+           llvm::StringMap<std::unique_ptr<FieldEnum>> &registers_enum_types) {
+  Log *log(GetLog(GDBRLog::Process));
+
+  // The top level element is "<enum...".
+  feature_node.ForEachChildElementWithName(
+      "enum", [log, &registers_enum_types](const XMLNode &enum_node) {
+        std::string id;
+
+        enum_node.ForEachAttribute([&id](const llvm::StringRef &attr_name,
+                                         const llvm::StringRef &attr_value) {
+          if (attr_name == "id")
+            id = attr_value;
+
+          // There is also a "size" attribute that is supposed to be the size in
+          // bytes of the register this applies to. However:
+          // * LLDB doesn't need this information.
+          // * It  is difficult to verify because you have to wait until the
+          //   enum is applied to a field.
+          //
+          // So we will emit this attribute in XML for GDB's sake, but will not
+          // bother ingesting it.
+
+          // Walk all attributes.
+          return true;
+        });
+
+        if (!id.empty()) {
+          FieldEnum::Enumerators enumerators = ParseEnumEvalues(enum_node);
+          if (!enumerators.empty()) {
+            LLDB_LOG(log,
+                     "ProcessGDBRemote::ParseEnums Found enum type \"{0}\"",
+                     id);
+            registers_enum_types.insert_or_assign(
+                id, std::make_unique<FieldEnum>(id, enumerators));
+          }
+        }
+
+        // Find all <enum> elements.
+        return true;
+      });
+}
+
+static std::vector<RegisterFlags::Field> ParseFlagsFields(
+    XMLNode flags_node, unsigned size,
+    const llvm::StringMap<std::unique_ptr<FieldEnum>> &registers_enum_types) {
   Log *log(GetLog(GDBRLog::Process));
   const unsigned max_start_bit = size * 8 - 1;
 
   // Process the fields of this set of flags.
   std::vector<RegisterFlags::Field> fields;
-  flags_node.ForEachChildElementWithName("field", [&fields, max_start_bit,
-                                                   &log](const XMLNode
-                                                             &field_node) {
+  flags_node.ForEachChildElementWithName("field", [&fields, max_start_bit, &log,
+                                                   &registers_enum_types](
+                                                      const XMLNode
+                                                          &field_node) {
     std::optional<llvm::StringRef> name;
     std::optional<unsigned> start;
     std::optional<unsigned> end;
+    std::optional<llvm::StringRef> type;
 
-    field_node.ForEachAttribute([&name, &start, &end, max_start_bit,
+    field_node.ForEachAttribute([&name, &start, &end, &type, max_start_bit,
                                  &log](const llvm::StringRef &attr_name,
                                        const llvm::StringRef &attr_value) {
       // Note that XML in general requires that each of these attributes only
@@ -4240,8 +4353,7 @@ static std::vector<RegisterFlags::Field> ParseFlagsFields(XMLNode flags_node,
                    attr_value.data());
         }
       } else if (attr_name == "type") {
-        // Type is a known attribute but we do not currently use it and it is
-        // not required.
+        type = attr_value;
       } else {
         LLDB_LOG(
             log,
@@ -4254,14 +4366,55 @@ static std::vector<RegisterFlags::Field> ParseFlagsFields(XMLNode flags_node,
     });
 
     if (name && start && end) {
-      if (*start > *end) {
+      if (*start > *end)
         LLDB_LOG(
             log,
             "ProcessGDBRemote::ParseFlagsFields Start {0} > end {1} in field "
             "\"{2}\", ignoring",
             *start, *end, name->data());
-      } else {
-        fields.push_back(RegisterFlags::Field(name->str(), *start, *end));
+      else {
+        if (RegisterFlags::Field::GetSizeInBits(*start, *end) > 64)
+          LLDB_LOG(log,
+                   "ProcessGDBRemote::ParseFlagsFields Ignoring field \"{2}\" "
+                   "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.
+          const FieldEnum *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 =
+                  RegisterFlags::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());
+            }
+          }
+
+          fields.push_back(
+              RegisterFlags::Field(name->str(), *start, *end, enum_type));
+        }
       }
     }
 
@@ -4272,12 +4425,14 @@ static std::vector<RegisterFlags::Field> ParseFlagsFields(XMLNode flags_node,
 
 void ParseFlags(
     XMLNode feature_node,
-    llvm::StringMap<std::unique_ptr<RegisterFlags>> &registers_flags_types) {
+    llvm::StringMap<std::unique_ptr<RegisterFlags>> &registers_flags_types,
+    const llvm::StringMap<std::unique_ptr<FieldEnum>> &registers_enum_types) {
   Log *log(GetLog(GDBRLog::Process));
 
   feature_node.ForEachChildElementWithName(
       "flags",
-      [&log, &registers_flags_types](const XMLNode &flags_node) -> bool {
+      [&log, &registers_flags_types,
+       &registers_enum_types](const XMLNode &flags_node) -> bool {
         LLDB_LOG(log, "ProcessGDBRemote::ParseFlags Found flags node \"{0}\"",
                  flags_node.GetAttributeValue("id").c_str());
 
@@ -4310,7 +4465,7 @@ void ParseFlags(
         if (id && size) {
           // Process the fields of this set of flags.
           std::vector<RegisterFlags::Field> fields =
-              ParseFlagsFields(flags_node, *size);
+              ParseFlagsFields(flags_node, *size, registers_enum_types);
           if (fields.size()) {
             // Sort so that the fields with the MSBs are first.
             std::sort(fields.rbegin(), fields.rend());
@@ -4375,13 +4530,19 @@ void ParseFlags(
 bool ParseRegisters(
     XMLNode feature_node, GdbServerTargetInfo &target_info,
     std::vector<DynamicRegisterInfo::Register> &registers,
-    llvm::StringMap<std::unique_ptr<RegisterFlags>> &registers_flags_types) {
+    llvm::StringMap<std::unique_ptr<RegisterFlags>> &registers_flags_types,
+    llvm::StringMap<std::unique_ptr<FieldEnum>> &registers_enum_types) {
   if (!feature_node)
     return false;
 
   Log *log(GetLog(GDBRLog::Process));
 
-  ParseFlags(feature_node, registers_flags_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);
 
@@ -4643,7 +4804,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_flags_types, m_registers_enum_types);
       }
 
       for (const auto &include : target_info.includes) {
@@ -4708,13 +4869,14 @@ bool ProcessGDBRemote::GetGDBServerRegisterInfo(ArchSpec &arch_to_use) {
   if (!m_gdb_comm.GetQXferFeaturesReadSupported())
     return false;
 
-  // This holds register flags information for the whole of target.xml.
+  // 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) &&

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h

@@ -484,6 +484,11 @@ private:
   // entries are added. Which would invalidate any pointers set in the register
   // info up to that point.
   llvm::StringMap<std::unique_ptr<RegisterFlags>> 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 RegisterFlags.
+  llvm::StringMap<std::unique_ptr<FieldEnum>> m_registers_enum_types;
 };
 
 } // namespace process_gdb_remote

lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp

@@ -43,8 +43,7 @@ CompilerType RegisterTypeBuilderClang::GetRegisterType(
       ScratchTypeSystemClang::GetForTarget(m_target);
   assert(type_system);
 
-  std::string register_type_name = "__lldb_register_fields_";
-  register_type_name += name;
+  std::string register_type_name = "__lldb_register_fields_" + name;
   // See if we have made this type before and can reuse it.
   CompilerType fields_type =
       type_system->GetTypeForIdentifier<clang::CXXRecordDecl>(
@@ -67,8 +66,48 @@ CompilerType RegisterTypeBuilderClang::GetRegisterType(
     // We assume that RegisterFlags has padded and sorted the fields
     // already.
     for (const RegisterFlags::Field &field : flags.GetFields()) {
+      CompilerType field_type = field_uint_type;
+
+      if (const FieldEnum *enum_type = field.GetEnum()) {
+        const FieldEnum::Enumerators &enumerators = enum_type->GetEnumerators();
+        if (!enumerators.empty()) {
+          // Enums can be used by many registers and the size of each register
+          // may be different. The register size is used as the underlying size
+          // of the enumerators, so we must make one enum type per register size
+          // it is used with.
+          std::string enum_type_name = "__lldb_register_fields_enum_" +
+                                       enum_type->GetID() + "_" +
+                                       std::to_string(byte_size);
+
+          // Enums can be used by mutiple fields and multiple registers, so we
+          // may have built this one already.
+          CompilerType field_enum_type =
+              type_system->GetTypeForIdentifier<clang::EnumDecl>(
+                  enum_type_name);
+
+          if (field_enum_type)
+            field_type = field_enum_type;
+          else {
+            field_type = type_system->CreateEnumerationType(
+                enum_type_name, type_system->GetTranslationUnitDecl(),
+                OptionalClangModuleID(), Declaration(), field_uint_type, false);
+
+            type_system->StartTagDeclarationDefinition(field_type);
+
+            Declaration decl;
+            for (auto enumerator : enumerators) {
+              type_system->AddEnumerationValueToEnumerationType(
+                  field_type, decl, enumerator.m_name.c_str(),
+                  enumerator.m_value, byte_size * 8);
+            }
+
+            type_system->CompleteTagDeclarationDefinition(field_type);
+          }
+        }
+      }
+
       type_system->AddFieldToRecordType(fields_type, field.GetName(),
-                                        field_uint_type, lldb::eAccessPublic,
+                                        field_type, lldb::eAccessPublic,
                                         field.GetSizeInBits());
     }
 

lldb/source/Target/RegisterFlags.cpp

@@ -366,6 +366,16 @@ void FieldEnum::Enumerator::ToXML(Stream &strm) const {
               escaped_name.c_str(), m_value);
 }
 
+void FieldEnum::Enumerator::DumpToLog(Log *log) const {
+  LLDB_LOG(log, "  Name: \"{0}\" Value: {1}", m_name.c_str(), m_value);
+}
+
+void FieldEnum::DumpToLog(Log *log) const {
+  LLDB_LOG(log, "ID: \"{0}\"", m_id.c_str());
+  for (const auto &enumerator : GetEnumerators())
+    enumerator.DumpToLog(log);
+}
+
 void RegisterFlags::ToXML(Stream &strm) const {
   // Example XML:
   // <flags id="cpsr_flags" size="4">

lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py

@@ -654,3 +654,405 @@ class TestXMLRegisterFlags(GDBRemoteTestBase):
             "register info cpsr",
             substrs=["| A< | B> | C' | D\" | E& |"],
         )
+
+    @skipIfXmlSupportMissing
+    @skipIfRemote
+    def test_no_enum(self):
+        """Check that lldb does not try to print an enum when there isn't one."""
+
+        self.setup_flags_test('<field name="E" start="0" end="0">' "</field>")
+
+        self.expect("register info cpsr", patterns=["E:.*$"], matching=False)
+
+    @skipIfXmlSupportMissing
+    @skipIfRemote
+    def test_enum_type_not_found(self):
+        """Check that lldb uses the default format if we don't find the enum type."""
+        self.setup_register_test(
+            """\
+          <flags id="cpsr_flags" size="4">
+            <field name="E" start="0" end="0" type="some_enum"/>
+          </flags>
+          <reg name="pc" bitsize="64"/>
+          <reg name="x0" regnum="0" bitsize="64"/>
+          <reg name="cpsr" regnum="33" bitsize="32" type="cpsr_flags"/>"""
+        )
+
+        self.expect("register read cpsr", patterns=["\(E = 1\)$"])
+
+    @skipIfXmlSupportMissing
+    @skipIfRemote
+    def test_enum_duplicated_evalue(self):
+        """Check that lldb only uses the last instance of a evalue for each
+        value."""
+        self.setup_register_test(
+            """\
+          <enum id="some_enum" size="4">
+            <evalue name="abc" value="1"/>
+            <evalue name="def" value="1"/>
+            <evalue name="geh" value="2"/>
+          </enum>
+          <flags id="cpsr_flags" size="4">
+            <field name="E" start="0" end="1" type="some_enum"/>
+          </flags>
+          <reg name="pc" bitsize="64"/>
+          <reg name="x0" regnum="0" bitsize="64"/>
+          <reg name="cpsr" regnum="33" bitsize="32" type="cpsr_flags"/>"""
+        )
+
+        self.expect("register info cpsr", patterns=["E: 1 = def, 2 = geh$"])
+        self.expect("register read cpsr", patterns=["\(E = def \| geh\)$"])
+
+    @skipIfXmlSupportMissing
+    @skipIfRemote
+    def test_enum_duplicated(self):
+        """Check that lldb only uses the last instance of enums with the same
+        id."""
+        self.setup_register_test(
+            """\
+          <enum id="some_enum" size="4">
+            <evalue name="abc" value="1"/>
+          </enum>
+          <enum id="some_enum" size="4">
+            <evalue name="def" value="1"/>
+          </enum>
+          <flags id="cpsr_flags" size="4">
+            <field name="E" start="0" end="0" type="some_enum"/>
+          </flags>
+          <reg name="pc" bitsize="64"/>
+          <reg name="x0" regnum="0" bitsize="64"/>
+          <reg name="cpsr" regnum="33" bitsize="32" type="cpsr_flags"/>"""
+        )
+
+        self.expect("register info cpsr", patterns=["E: 1 = def$"])
+        self.expect("register read cpsr", patterns=["\(E = def\)$"])
+
+    @skipIfXmlSupportMissing
+    @skipIfRemote
+    def test_enum_use_first_valid(self):
+        """Check that lldb uses the first enum that parses correctly and ignores
+        the rest."""
+        self.setup_register_test(
+            """\
+          <enum id="some_enum" size="4"/>
+          <enum size="4">
+            <evalue name="invalid" value="1"/>
+          </enum>
+          <enum id="some_enum" size="4">
+            <evalue name="valid" value="1"/>
+          </enum>
+          <enum id="another_enum" size="4">
+            <evalue name="invalid" value="1"/>
+          </enum>
+          <flags id="cpsr_flags" size="4">
+            <field name="E" start="0" end="0" type="some_enum"/>
+          </flags>
+          <reg name="pc" bitsize="64"/>
+          <reg name="x0" regnum="0" bitsize="64"/>
+          <reg name="cpsr" regnum="33" bitsize="32" type="cpsr_flags"/>"""
+        )
+
+        self.expect("register info cpsr", patterns=["E: 1 = valid$"])
+
+    @skipIfXmlSupportMissing
+    @skipIfRemote
+    def test_evalue_empty_name(self):
+        """Check that lldb ignores evalues with an empty name."""
+
+        # The only potential use case for empty names is to shadow an evalue
+        # declared later so that it's name is hidden should the debugger only
+        # pick one of them. This behaviour would be debugger specific so the protocol
+        # would probably not care or leave it up to us, and I think it's not a
+        # useful thing to allow.
+
+        self.setup_register_test(
+            """\
+          <enum id="some_enum" size="4">
+            <evalue name="" value="1"/>
+            <evalue name="valid" value="2"/>
+          </enum>
+          <flags id="cpsr_flags" size="4">
+            <field name="E" start="0" end="1" type="some_enum"/>
+          </flags>
+          <reg name="pc" bitsize="64"/>
+          <reg name="x0" regnum="0" bitsize="64"/>
+          <reg name="cpsr" regnum="33" bitsize="32" type="cpsr_flags"/>"""
+        )
+
+        self.expect("register info cpsr", patterns=["E: 2 = valid$"])
+
+    @skipIfXmlSupportMissing
+    @skipIfRemote
+    def test_evalue_invalid_value(self):
+        """Check that lldb ignores evalues with an invalid value."""
+        self.setup_register_test(
+            """\
+          <enum id="some_enum" size="4">
+            <evalue name="negative_dec" value="-1"/>
+            <evalue name="negative_hex" value="-0x1"/>
+            <evalue name="negative_bin" value="-0b1"/>
+            <evalue name="negative_float" value="-0.5"/>
+            <evalue name="nan" value="aardvark"/>
+            <evalue name="dec" value="1"/>
+            <evalue name="hex" value="0x2"/>
+            <evalue name="octal" value="03"/>
+            <evalue name="float" value="0.5"/>
+            <evalue name="bin" value="0b100"/>
+          </enum>
+          <flags id="cpsr_flags" size="4">
+            <field name="E" start="0" end="2" type="some_enum"/>
+          </flags>
+          <reg name="pc" bitsize="64"/>
+          <reg name="x0" regnum="0" bitsize="64"/>
+          <reg name="cpsr" regnum="33" bitsize="32" type="cpsr_flags"/>"""
+        )
+
+        self.expect(
+            "register info cpsr", patterns=["E: 1 = dec, 2 = hex, 3 = octal, 4 = bin$"]
+        )
+
+    @skipIfXmlSupportMissing
+    @skipIfRemote
+    def test_evalue_out_of_range(self):
+        """Check that lldb will not use an enum type if one of its evalues
+        exceeds the size of the field it is applied to."""
+        self.setup_register_test(
+            """\
+          <enum id="some_enum" size="4">
+            <evalue name="A" value="0"/>
+            <evalue name="B" value="2"/>
+          </enum>
+          <flags id="cpsr_flags" size="4">
+            <field name="E" start="0" end="0" type="some_enum"/>
+          </flags>
+          <reg name="pc" bitsize="64"/>
+          <reg name="x0" regnum="0" bitsize="64"/>
+          <reg name="cpsr" regnum="33" bitsize="32" type="cpsr_flags"/>"""
+        )
+
+        # The whole eunm is rejected even if just 1 value is out of range.
+        self.expect("register info cpsr", patterns=["E: 0 = "], matching=False)
+
+    @skipIfXmlSupportMissing
+    @skipIfRemote
+    def test_enum_ignore_unknown_attributes(self):
+        """Check that lldb ignores unknown attributes on an enum or evalue."""
+        self.setup_register_test(
+            """\
+          <enum id="some_enum" size="4" foo=\"bar\">
+            <evalue name="valid" value="1" colour=\"red"/>
+          </enum>
+          <flags id="cpsr_flags" size="4">
+            <field name="E" start="0" end="0" type="some_enum"/>
+          </flags>
+          <reg name="pc" bitsize="64"/>
+          <reg name="x0" regnum="0" bitsize="64"/>
+          <reg name="cpsr" regnum="33" bitsize="32" type="cpsr_flags"/>"""
+        )
+
+        self.expect("register info cpsr", patterns=["E: 1 = valid$"])
+
+    @skipIfXmlSupportMissing
+    @skipIfRemote
+    def test_evalue_required_attributes(self):
+        """Check that lldb rejects any evalue missing a name and/or value."""
+        self.setup_register_test(
+            """\
+          <enum id="some_enum" size="4">
+            <evalue name="foo"/>
+            <evalue value="1"/>
+            <evalue />
+            <evalue name="valid" value="1"/>
+          </enum>
+          <flags id="cpsr_flags" size="4">
+            <field name="E" start="0" end="0" type="some_enum"/>
+          </flags>
+          <reg name="pc" bitsize="64"/>
+          <reg name="x0" regnum="0" bitsize="64"/>
+          <reg name="cpsr" regnum="33" bitsize="32" type="cpsr_flags"/>"""
+        )
+
+        self.expect("register info cpsr", patterns=["E: 1 = valid$"])
+
+    @skipIfXmlSupportMissing
+    @skipIfRemote
+    def test_evalue_name_xml_reserved_characters(self):
+        """Check that lldb converts reserved character replacements like &amp;
+        when found in evalue names."""
+        self.setup_register_test(
+            """\
+          <enum id="some_enum" size="4">
+            <evalue name="A&amp;"  value="0"/>
+            <evalue name="B&quot;" value="1"/>
+            <evalue name="C&apos;" value="2"/>
+            <evalue name="D&gt;"   value="3"/>
+            <evalue name="E&lt;"   value="4"/>
+          </enum>
+          <flags id="cpsr_flags" size="4">
+            <field name="E" start="0" end="2" type="some_enum"/>
+          </flags>
+          <reg name="pc" bitsize="64"/>
+          <reg name="x0" regnum="0" bitsize="64"/>
+          <reg name="cpsr" regnum="33" bitsize="32" type="cpsr_flags"/>"""
+        )
+
+        self.expect(
+            "register info cpsr",
+            patterns=["E: 0 = A&, 1 = B\", 2 = C', 3 = D>, 4 = E<$"],
+        )
+
+    @skipIfXmlSupportMissing
+    @skipIfRemote
+    def test_enum_value_range(self):
+        """Check that lldb ignores enums whose values would not fit into
+        their field."""
+
+        self.setup_register_test(
+            """\
+          <enum id="some_enum" size="4">
+            <evalue name="A" value="0"/>
+            <evalue name="B" value="1"/>
+            <evalue name="C" value="2"/>
+            <evalue name="D" value="3"/>
+            <evalue name="E" value="4"/>
+          </enum>
+          <flags id="cpsr_flags" size="4">
+            <field name="foo" start="0" end="1" type="some_enum"/>
+            <field name="bar" start="2" end="10" type="some_enum"/>
+          </flags>
+          <reg name="pc" bitsize="64"/>
+          <reg name="x0" regnum="0" bitsize="64"/>
+          <reg name="cpsr" regnum="33" bitsize="32" type="cpsr_flags"/>"""
+        )
+
+        # some_enum can apply to foo
+        self.expect(
+            "register info cpsr", patterns=["bar: 0 = A, 1 = B, 2 = C, 3 = D, 4 = E$"]
+        )
+        # but not to bar
+        self.expect("register info cpsr", patterns=["foo: "], matching=False)
+
+    @skipIfXmlSupportMissing
+    @skipIfRemote
+    def test_evalue_value_limits(self):
+        """Check that lldb can handle an evalue for a field up to 64 bits
+        in size and anything greater is ignored."""
+
+        self.setup_register_test(
+            """\
+          <enum id="some_enum" size="8">
+            <evalue name="min" value="0"/>
+            <evalue name="max" value="0xffffffffffffffff"/>
+            <evalue name="invalid" value="0xfffffffffffffffff"/>
+          </enum>
+          <flags id="x0_flags" size="8">
+            <field name="foo" start="0" end="63" type="some_enum"/>
+          </flags>
+          <reg name="pc" bitsize="64"/>
+          <reg name="x0" regnum="0" bitsize="64" type="x0_flags"/>
+          <reg name="cpsr" regnum="33" bitsize="32"/>"""
+        )
+
+        self.expect(
+            "register info x0", patterns=["foo: 0 = min, 18446744073709551615 = max$"]
+        )
+
+    @skipIfXmlSupportMissing
+    @skipIfRemote
+    def test_field_size_limit(self):
+        """Check that lldb ignores any field > 64 bits. We can't handle those
+        correctly."""
+
+        self.setup_register_test(
+            """\
+          <flags id="x0_flags" size="8">
+            <field name="invalid" start="0" end="64"/>
+            <field name="valid" start="0" end="63"/>
+          </flags>
+          <reg name="pc" bitsize="64"/>
+          <reg name="x0" regnum="0" bitsize="64" type="x0_flags"/>
+          <reg name="cpsr" regnum="33" bitsize="32"/>"""
+        )
+
+        self.expect(
+            "register info x0", substrs=["| 63-0  |\n" "|-------|\n" "| valid |"]
+        )
+
+    @skipIfXmlSupportMissing
+    @skipIfRemote
+    def test_many_fields_same_enum(self):
+        """Check that an enum can be reused by many fields, and fields of many
+        registers."""
+
+        self.setup_register_test(
+            """\
+          <enum id="some_enum" size="8">
+            <evalue name="valid" value="1"/>
+          </enum>
+          <flags id="x0_flags" size="8">
+            <field name="f1" start="0" end="0" type="some_enum"/>
+            <field name="f2" start="1" end="1" type="some_enum"/>
+          </flags>
+          <flags id="cpsr_flags" size="4">
+            <field name="f1" start="0" end="0" type="some_enum"/>
+            <field name="f2" start="1" end="1" type="some_enum"/>
+          </flags>
+          <reg name="pc" bitsize="64"/>
+          <reg name="x0" regnum="0" bitsize="64" type="x0_flags"/>
+          <reg name="cpsr" regnum="33" bitsize="32" type="cpsr_flags"/>"""
+        )
+
+        expected_info = [
+            dedent(
+                """\
+             f2: 1 = valid
+
+             f1: 1 = valid$"""
+            )
+        ]
+        self.expect("register info x0", patterns=expected_info)
+
+        self.expect("register info cpsr", patterns=expected_info)
+
+        expected_read = ["\(f2 = valid, f1 = valid\)$"]
+        self.expect("register read x0", patterns=expected_read)
+        self.expect("register read cpsr", patterns=expected_read)
+
+    @skipIfXmlSupportMissing
+    @skipIfRemote
+    def test_fields_same_name_different_enum(self):
+        """Check that lldb does something sensible when there are two fields with
+        the same name, but their enum types differ."""
+
+        # It's unlikely anyone would do this intentionally but it is allowed by
+        # the protocol spec so we have to cope with it.
+        self.setup_register_test(
+            """\
+          <enum id="foo_enum" size="8">
+            <evalue name="foo_0" value="1"/>
+          </enum>
+          <enum id="foo_alt_enum" size="8">
+            <evalue name="foo_1" value="1"/>
+          </enum>
+          <flags id="x0_flags" size="8">
+            <field name="foo" start="0" end="0" type="foo_enum"/>
+            <field name="foo" start="1" end="1" type="foo_alt_enum"/>
+          </flags>
+          <reg name="pc" bitsize="64"/>
+          <reg name="x0" regnum="0" bitsize="64" type="x0_flags"/>
+          <reg name="cpsr" regnum="33" bitsize="32"/>"""
+        )
+
+        self.expect(
+            "register info x0",
+            patterns=[
+                dedent(
+                    """\
+             foo: 1 = foo_1
+
+             foo: 1 = foo_0$"""
+                )
+            ],
+        )
+
+        self.expect("register read x0", patterns=["\(foo = foo_1, foo = foo_0\)$"])

lldb/unittests/Core/DumpRegisterInfoTest.cpp

@@ -102,3 +102,29 @@ TEST(DoDumpRegisterInfoTest, FieldsTable) {
                               "|-------|-------|------|-----|\n"
                               "|   A   |   B   |  C   |  D  |");
 }
+
+TEST(DoDumpRegisterInfoTest, Enumerators) {
+  StreamString strm;
+
+  FieldEnum enum_one("enum_one", {{0, "an_enumerator"}});
+  FieldEnum enum_two("enum_two",
+                     {{1, "another_enumerator"}, {2, "another_enumerator_2"}});
+
+  RegisterFlags flags("", 4,
+                      {RegisterFlags::Field("A", 24, 31, &enum_one),
+                       RegisterFlags::Field("B", 16, 23),
+                       RegisterFlags::Field("C", 8, 15, &enum_two)});
+
+  DoDumpRegisterInfo(strm, "abc", nullptr, 4, {}, {}, {}, &flags, 100);
+  ASSERT_EQ(strm.GetString(),
+            "       Name: abc\n"
+            "       Size: 4 bytes (32 bits)\n"
+            "\n"
+            "| 31-24 | 23-16 | 15-8 | 7-0 |\n"
+            "|-------|-------|------|-----|\n"
+            "|   A   |   B   |  C   |     |\n"
+            "\n"
+            "A: 0 = an_enumerator\n"
+            "\n"
+            "C: 1 = another_enumerator, 2 = another_enumerator_2");
+}