llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: David Spickett (DavidSpickett) <details> <summary>Changes</summary> 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. --- Patch is 35.12 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/97270.diff 8 Files Affected: - (modified) lldb/include/lldb/Target/RegisterFlags.h (+7) - (modified) lldb/source/Core/DumpRegisterInfo.cpp (+6-1) - (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (+180-18) - (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h (+5) - (modified) lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp (+42-3) - (modified) lldb/source/Target/RegisterFlags.cpp (+10) - (modified) lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py (+402) - (modified) lldb/unittests/Core/DumpRegisterInfoTest.cpp (+26) ``````````diff diff --git a/lldb/include/lldb/Target/RegisterFlags.h b/lldb/include/lldb/Target/RegisterFlags.h index 1112972cf72e1..1250fd0330958 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 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 @@ 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 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 060a30abee83e..604c92369e9a2 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ b/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>> ®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 +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>> ®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 +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> ®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->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) && 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..1ecde7bee5820 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,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()); } diff --git a/lldb/source/Target/RegisterFlags.cpp b/lldb/source/Target/RegisterFlags.cpp index 476150251221a..976e03870ad9e 100644 --- a/lldb/source/Target/RegisterFlags.cpp +++ b/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"> diff --git a/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py b/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py index e2c75970c2d2e..2dbb2b5f5e3a9 100644 --- a/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py +++ b/lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py @@ -654,3 +654,405 @@ 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 insta... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/97270 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
