diff --git a/lldb/include/lldb/Target/RegisterFlags.h b/lldb/include/lldb/Target/RegisterFlags.h index 1250fd03309580..1112972cf72e19 100644 --- a/lldb/include/lldb/Target/RegisterFlags.h +++ b/lldb/include/lldb/Target/RegisterFlags.h @@ -32,15 +32,10 @@ class FieldEnum { : m_value(value), m_name(std::move(name)) {} void ToXML(Stream &strm) const; - - void DumpToLog(Log *log) const; }; typedef std::vector 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; } @@ -49,8 +44,6 @@ class FieldEnum { void ToXML(Stream &strm, unsigned size) const; - void DumpToLog(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 eccc6784cd4974..83347954169024 100644 --- a/lldb/source/Core/DumpRegisterInfo.cpp +++ b/lldb/source/Core/DumpRegisterInfo.cpp @@ -111,11 +111,6 @@ 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 604c92369e9a27..060a30abee83eb 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -4179,134 +4179,21 @@ struct GdbServerTargetInfo { RegisterSetMap reg_set_map; }; -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 enumerators; - - enum_node.ForEachChildElementWithName( - "evalue", [&enumerators, &log](const XMLNode &enumerator_node) { - std::optional name; - std::optional 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> ®isters_enum_types) { - Log *log(GetLog(GDBRLog::Process)); - - // The top level element is "(id, enumerators)); - } - } - - // Find all elements. - return true; - }); -} - -static std::vector ParseFlagsFields( - XMLNode flags_node, unsigned size, - const llvm::StringMap> ®isters_enum_types) { +static std::vector ParseFlagsFields(XMLNode flags_node, + unsigned size) { Log *log(GetLog(GDBRLog::Process)); const unsigned max_start_bit = size * 8 - 1; // Process the fields of this set of flags. std::vector fields; - flags_node.ForEachChildElementWithName("field", [&fields, max_start_bit, &log, - ®isters_enum_types]( - const XMLNode - &field_node) { + flags_node.ForEachChildElementWithName("field", [&fields, max_start_bit, + &log](const XMLNode + &field_node) { std::optional name; std::optional start; std::optional end; - std::optional type; - field_node.ForEachAttribute([&name, &start, &end, &type, max_start_bit, + field_node.ForEachAttribute([&name, &start, &end, 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 @@ -4353,7 +4240,8 @@ static std::vector ParseFlagsFields( attr_value.data()); } } else if (attr_name == "type") { - type = attr_value; + // Type is a known attribute but we do not currently use it and it is + // not required. } else { LLDB_LOG( log, @@ -4366,55 +4254,14 @@ static std::vector ParseFlagsFields( }); 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 { - 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)); - } + } else { + fields.push_back(RegisterFlags::Field(name->str(), *start, *end)); } } @@ -4425,14 +4272,12 @@ static std::vector ParseFlagsFields( void ParseFlags( XMLNode feature_node, - llvm::StringMap> ®isters_flags_types, - const llvm::StringMap> ®isters_enum_types) { + llvm::StringMap> ®isters_flags_types) { Log *log(GetLog(GDBRLog::Process)); feature_node.ForEachChildElementWithName( "flags", - [&log, ®isters_flags_types, - ®isters_enum_types](const XMLNode &flags_node) -> bool { + [&log, ®isters_flags_types](const XMLNode &flags_node) -> bool { LLDB_LOG(log, "ProcessGDBRemote::ParseFlags Found flags node \"{0}\"", flags_node.GetAttributeValue("id").c_str()); @@ -4465,7 +4310,7 @@ void ParseFlags( if (id && size) { // Process the fields of this set of flags. std::vector fields = - ParseFlagsFields(flags_node, *size, registers_enum_types); + ParseFlagsFields(flags_node, *size); if (fields.size()) { // Sort so that the fields with the MSBs are first. std::sort(fields.rbegin(), fields.rend()); @@ -4530,19 +4375,13 @@ void ParseFlags( bool ParseRegisters( XMLNode feature_node, GdbServerTargetInfo &target_info, std::vector ®isters, - llvm::StringMap> ®isters_flags_types, - llvm::StringMap> ®isters_enum_types) { + llvm::StringMap> ®isters_flags_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); + ParseFlags(feature_node, registers_flags_types); for (const auto &flags : registers_flags_types) flags.second->DumpToLog(log); @@ -4804,7 +4643,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); + m_registers_flags_types); } for (const auto &include : target_info.includes) { @@ -4869,14 +4708,13 @@ bool ProcessGDBRemote::GetGDBServerRegisterInfo(ArchSpec &arch_to_use) { if (!m_gdb_comm.GetQXferFeaturesReadSupported()) return false; - // These hold register type information for the whole of target.xml. + // This holds register flags 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 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 b44ffefcd0d366..610a1ee0b34d2f 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h @@ -484,11 +484,6 @@ class ProcessGDBRemote : public Process, // entries are added. Which would invalidate any pointers set in the register // info up to that point. llvm::StringMap> 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> 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 a088a8742718f3..067768537c0684 100644 --- a/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp +++ b/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp @@ -43,7 +43,8 @@ CompilerType RegisterTypeBuilderClang::GetRegisterType( ScratchTypeSystemClang::GetForTarget(m_target); assert(type_system); - std::string register_type_name = "__lldb_register_fields_" + name; + std::string register_type_name = "__lldb_register_fields_"; + register_type_name += name; // See if we have made this type before and can reuse it. CompilerType fields_type = type_system->GetTypeForIdentifier( @@ -66,43 +67,8 @@ 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()) { - 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( - 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_type, lldb::eAccessPublic, + field_uint_type, lldb::eAccessPublic, field.GetSizeInBits()); } diff --git a/lldb/source/Target/RegisterFlags.cpp b/lldb/source/Target/RegisterFlags.cpp index 976e03870ad9e4..476150251221a1 100644 --- a/lldb/source/Target/RegisterFlags.cpp +++ b/lldb/source/Target/RegisterFlags.cpp @@ -366,16 +366,6 @@ 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: // diff --git a/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py b/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py index 2dbb2b5f5e3a9c..e2c75970c2d2ed 100644 --- a/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py +++ b/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py @@ -654,405 +654,3 @@ 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('' "") - - 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( - """\ - - - - - - """ - ) - - 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( - """\ - - - - - - - - - - - """ - ) - - 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( - """\ - - - - - - - - - - - - """ - ) - - 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( - """\ - - - - - - - - - - - - - - - - """ - ) - - 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( - """\ - - - - - - - - - - """ - ) - - 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( - """\ - - - - - - - - - - - - - - - - - - """ - ) - - 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( - """\ - - - - - - - - - - """ - ) - - # 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( - """\ - - - - - - - - - """ - ) - - 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( - """\ - - - - - - - - - - - - """ - ) - - 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( - """\ - - - - - - - - - - - - - """ - ) - - 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( - """\ - - - - - - - - - - - - - - """ - ) - - # 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( - """\ - - - - - - - - - - - """ - ) - - 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( - """\ - - - - - - - """ - ) - - 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( - """\ - - - - - - - - - - - - - - """ - ) - - 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( - """\ - - - - - - - - - - - - - """ - ) - - 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\)$"]) diff --git a/lldb/unittests/Core/DumpRegisterInfoTest.cpp b/lldb/unittests/Core/DumpRegisterInfoTest.cpp index 593170c2822abf..0d3ff8f542595f 100644 --- a/lldb/unittests/Core/DumpRegisterInfoTest.cpp +++ b/lldb/unittests/Core/DumpRegisterInfoTest.cpp @@ -102,29 +102,3 @@ 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"); -}