https://github.com/DavidSpickett updated https://github.com/llvm/llvm-project/pull/95768
>From a9b542a1686be0afd73ad89a504d2c66ba6c4a4f Mon Sep 17 00:00:00 2001 From: David Spickett <david.spick...@linaro.org> Date: Mon, 11 Mar 2024 10:56:29 +0000 Subject: [PATCH 1/2] [lldb] Parse and display register field enums This teaches lldb to parse the enum XML elements sent by lldb-server, and make use of the information in `register read` and `register info`. The format is described in https://sourceware.org/gdb/current/onlinedocs/gdb.html/Enum-Target-Types.html. The target XML parser will drop any invalid enum or evalue. If we find multiple evalue for the same value, we will use the last one we find. The order of evalues from the XML is preserved as there may be good reason they are not in numerical order. --- lldb/include/lldb/Target/RegisterFlags.h | 7 + lldb/source/Core/DumpRegisterInfo.cpp | 7 +- .../Process/gdb-remote/ProcessGDBRemote.cpp | 188 +++++++++- .../Process/gdb-remote/ProcessGDBRemote.h | 5 + .../RegisterTypeBuilderClang.cpp | 30 +- lldb/source/Target/RegisterFlags.cpp | 10 + .../gdb_remote_client/TestXMLRegisterFlags.py | 322 ++++++++++++++++++ lldb/unittests/Core/DumpRegisterInfoTest.cpp | 26 ++ 8 files changed, 573 insertions(+), 22 deletions(-) diff --git a/lldb/include/lldb/Target/RegisterFlags.h b/lldb/include/lldb/Target/RegisterFlags.h index 1c6bf5dcf4a7f..628c841c10d95 100644 --- a/lldb/include/lldb/Target/RegisterFlags.h +++ b/lldb/include/lldb/Target/RegisterFlags.h @@ -32,10 +32,15 @@ class FieldEnum { : m_value(value), m_name(std::move(name)) {} void ToXML(Stream &strm) const; + + void log(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 @@ class FieldEnum { void ToXML(Stream &strm, unsigned size) const; + void log(Log *log) const; + private: std::string m_id; Enumerators m_enumerators; diff --git a/lldb/source/Core/DumpRegisterInfo.cpp b/lldb/source/Core/DumpRegisterInfo.cpp index 8334795416902..eccc6784cd497 100644 --- a/lldb/source/Core/DumpRegisterInfo.cpp +++ b/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; + } } diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp index a5a731981299f..4754698e6e88f 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -4179,21 +4179,124 @@ 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. + 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>> ®isters_enum_types) { + Log *log(GetLog(GDBRLog::Process)); + + // The top level element is "<enum...". + feature_node.ForEachChildElementWithName( + "enum", [log, ®isters_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>> ®isters_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, + ®isters_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 +4343,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 +4356,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 +4415,14 @@ static std::vector<RegisterFlags::Field> ParseFlagsFields(XMLNode flags_node, void ParseFlags( XMLNode feature_node, - llvm::StringMap<std::unique_ptr<RegisterFlags>> ®isters_flags_types) { + llvm::StringMap<std::unique_ptr<RegisterFlags>> ®isters_flags_types, + const llvm::StringMap<std::unique_ptr<FieldEnum>> ®isters_enum_types) { Log *log(GetLog(GDBRLog::Process)); feature_node.ForEachChildElementWithName( "flags", - [&log, ®isters_flags_types](const XMLNode &flags_node) -> bool { + [&log, ®isters_flags_types, + ®isters_enum_types](const XMLNode &flags_node) -> bool { LLDB_LOG(log, "ProcessGDBRemote::ParseFlags Found flags node \"{0}\"", flags_node.GetAttributeValue("id").c_str()); @@ -4310,7 +4455,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 +4520,19 @@ void ParseFlags( bool ParseRegisters( XMLNode feature_node, GdbServerTargetInfo &target_info, std::vector<DynamicRegisterInfo::Register> ®isters, - llvm::StringMap<std::unique_ptr<RegisterFlags>> ®isters_flags_types) { + llvm::StringMap<std::unique_ptr<RegisterFlags>> ®isters_flags_types, + llvm::StringMap<std::unique_ptr<FieldEnum>> ®isters_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->log(log); + + ParseFlags(feature_node, registers_flags_types, registers_enum_types); for (const auto &flags : registers_flags_types) flags.second->log(log); @@ -4643,7 +4794,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 +4859,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)) diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h index 610a1ee0b34d2..b44ffefcd0d36 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h @@ -484,6 +484,11 @@ class ProcessGDBRemote : public Process, // 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 diff --git a/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp b/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp index 067768537c068..556febc1f6cf1 100644 --- a/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp +++ b/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,33 @@ 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()) + continue; + + std::string enum_type_name = + register_type_name + "_" + field.GetName() + "_enum"; + 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()); } diff --git a/lldb/source/Target/RegisterFlags.cpp b/lldb/source/Target/RegisterFlags.cpp index d8a87090a7a41..d6669a3edd003 100644 --- a/lldb/source/Target/RegisterFlags.cpp +++ b/lldb/source/Target/RegisterFlags.cpp @@ -370,6 +370,16 @@ void FieldEnum::Enumerator::ToXML(Stream &strm) const { escaped_name.c_str(), m_value); } +void FieldEnum::Enumerator::log(Log *log) const { + LLDB_LOG(log, " Name: \"{0}\" Value: {1}", m_name.c_str(), m_value); +} + +void FieldEnum::log(Log *log) const { + LLDB_LOG(log, "ID: \"{0}\"", m_id.c_str()); + for (const auto &enumerator : GetEnumerators()) + enumerator.log(log); +} + void RegisterFlags::ToXML(Stream &strm) const { // Example XML: // <flags id="cpsr_flags" size="4"> diff --git a/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py b/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py index e2c75970c2d2e..59beb1857ffd9 100644 --- a/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py +++ b/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py @@ -654,3 +654,325 @@ def test_flags_name_xml_reserved_characters(self): "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 & + when found in evalue names.""" + 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="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.""" + + self.setup_register_test( + """\ + <enum id="some_enum" size="8"> + <evalue name="min" value="0"/> + <evalue name="max" value="0xffffffffffffffff"/> + </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 |"] + ) diff --git a/lldb/unittests/Core/DumpRegisterInfoTest.cpp b/lldb/unittests/Core/DumpRegisterInfoTest.cpp index 0d3ff8f542595..593170c2822ab 100644 --- a/lldb/unittests/Core/DumpRegisterInfoTest.cpp +++ b/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"); +} >From 5b6c66f26b764afefa41df56fd333fd777a29b21 Mon Sep 17 00:00:00 2001 From: David Spickett <david.spick...@linaro.org> Date: Thu, 20 Jun 2024 11:23:51 +0000 Subject: [PATCH 2/2] * Cache enum types * Key enum types by enum ID not register name, since they can be used across many registers. * Add tests for sharing enums across registers and fields, even if the field names are identical. --- .../RegisterTypeBuilderClang.cpp | 48 ++++++----- .../gdb_remote_client/TestXMLRegisterFlags.py | 79 +++++++++++++++++++ 2 files changed, 108 insertions(+), 19 deletions(-) diff --git a/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp b/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp index 556febc1f6cf1..a088a8742718f 100644 --- a/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp +++ b/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp @@ -70,25 +70,35 @@ CompilerType RegisterTypeBuilderClang::GetRegisterType( if (const FieldEnum *enum_type = field.GetEnum()) { const FieldEnum::Enumerators &enumerators = enum_type->GetEnumerators(); - - if (enumerators.empty()) - continue; - - std::string enum_type_name = - register_type_name + "_" + field.GetName() + "_enum"; - 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); + if (!enumerators.empty()) { + std::string enum_type_name = + "__lldb_register_fields_enum_" + enum_type->GetID(); + + // 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(), diff --git a/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py b/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py index 59beb1857ffd9..b3eb822c125a0 100644 --- a/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py +++ b/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py @@ -976,3 +976,82 @@ def test_field_size_limit(self): 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-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
