DavidSpickett updated this revision to Diff 506903. DavidSpickett added a comment.
Use LOG instead of LOGF. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145574/new/ https://reviews.llvm.org/D145574 Files: lldb/include/lldb/Target/DynamicRegisterInfo.h lldb/include/lldb/lldb-private-types.h lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h lldb/source/Target/DynamicRegisterInfo.cpp
Index: lldb/source/Target/DynamicRegisterInfo.cpp =================================================================== --- lldb/source/Target/DynamicRegisterInfo.cpp +++ lldb/source/Target/DynamicRegisterInfo.cpp @@ -407,7 +407,7 @@ {reg.regnum_ehframe, reg.regnum_dwarf, reg.regnum_generic, reg.regnum_remote, local_regnum}, // value_regs and invalidate_regs are filled by Finalize() - nullptr, nullptr, nullptr + nullptr, nullptr, reg.flags_type }; m_regs.push_back(reg_info); Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h =================================================================== --- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h +++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h @@ -38,6 +38,7 @@ #include "GDBRemoteRegisterContext.h" #include "llvm/ADT/DenseMap.h" +#include "llvm/ADT/StringMap.h" namespace lldb_private { namespace repro { @@ -466,6 +467,15 @@ // fork helpers void DidForkSwitchSoftwareBreakpoints(bool enable); void DidForkSwitchHardwareTraps(bool enable); + + // Lists of register fields generated from the remote's target XML. + // Pointers to these RegisterFlags will be set in the register info passed + // back to the upper levels of lldb. Doing so is safe because this class will + // live at least as long as the debug session. We therefore do not store the + // data directly in the map because the map may reallocate it's storage as new + // entries are added. Which would invalidate any pointers set in the register + // info up to that point. + llvm::StringMap<std::unique_ptr<RegisterFlags>> m_registers_flags_types; }; } // namespace process_gdb_remote Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp =================================================================== --- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -53,6 +53,7 @@ #include "lldb/Target/ABI.h" #include "lldb/Target/DynamicLoader.h" #include "lldb/Target/MemoryRegionInfo.h" +#include "lldb/Target/RegisterFlags.h" #include "lldb/Target/SystemRuntime.h" #include "lldb/Target/Target.h" #include "lldb/Target/TargetList.h" @@ -84,6 +85,7 @@ #include "lldb/Utility/StringExtractorGDBRemote.h" #include "llvm/ADT/ScopeExit.h" +#include "llvm/ADT/StringMap.h" #include "llvm/ADT/StringSwitch.h" #include "llvm/Support/FormatAdapters.h" #include "llvm/Support/Threading.h" @@ -4027,15 +4029,213 @@ RegisterSetMap reg_set_map; }; -bool ParseRegisters(XMLNode feature_node, GdbServerTargetInfo &target_info, - std::vector<DynamicRegisterInfo::Register> ®isters) { +static std::vector<RegisterFlags::Field> 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<RegisterFlags::Field> fields; + flags_node.ForEachChildElementWithName("field", [&fields, max_start_bit, + &log](const XMLNode + &field_node) { + std::optional<llvm::StringRef> name; + std::optional<unsigned> start; + std::optional<unsigned> end; + + 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 + // appears once, so we don't have to handle that here. + if (attr_name == "name") { + LLDB_LOG(log, + "ProcessGDBRemote::ParseFlags Found field node name \"{0}\"", + attr_value.data()); + name = attr_value; + } else if (attr_name == "start") { + unsigned parsed_start = 0; + if (llvm::to_integer(attr_value, parsed_start)) { + if (parsed_start > max_start_bit) { + LLDB_LOG( + log, + "ProcessGDBRemote::ParseFlags Invalid start {0} in field node, " + "cannot be > {1}", + parsed_start, max_start_bit); + } else + start = parsed_start; + } else { + LLDB_LOG(log, + "ProcessGDBRemote::ParseFlags Invalid start \"{0}\" in " + "field node", + attr_value.data()); + } + } else if (attr_name == "end") { + unsigned parsed_end = 0; + if (llvm::to_integer(attr_value, parsed_end)) + if (parsed_end > max_start_bit) { + LLDB_LOG( + log, + "ProcessGDBRemote::ParseFlags Invalid end {0} in field node, " + "cannot be > {1}", + parsed_end, max_start_bit); + } else + end = parsed_end; + else { + LLDB_LOG( + log, + "ProcessGDBRemote::ParseFlags Invalid end \"{0}\" in field 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. + } else { + LLDB_LOG(log, + "ProcessGDBRemote::ParseFlags Ignoring unknown attribute " + "\"{0}\" in field node", + attr_name.data()); + } + + return true; // Walk all attributes of the field. + }); + + if (name && start && end) { + if (*start > *end) { + LLDB_LOG(log, + "ProcessGDBRemote::ParseFlags Start {0} > end {1} in field " + "\"{2}\", ignoring", + *start, *end, name->data()); + } else { + fields.push_back(RegisterFlags::Field(name->str(), *start, *end)); + } + } + + return true; // Iterate all "field" nodes. + }); + return fields; +} + +void ParseFlags( + XMLNode feature_node, + llvm::StringMap<std::unique_ptr<RegisterFlags>> ®isters_flags_types) { + Log *log(GetLog(GDBRLog::Process)); + + feature_node.ForEachChildElementWithName( + "flags", + [&log, ®isters_flags_types](const XMLNode &flags_node) -> bool { + LLDB_LOG(log, "ProcessGDBRemote::ParseFlags Found flags node \"{0}\"", + flags_node.GetAttributeValue("id").c_str()); + + std::optional<llvm::StringRef> id; + std::optional<unsigned> size; + flags_node.ForEachAttribute( + [&id, &size, &log](const llvm::StringRef &name, + const llvm::StringRef &value) { + if (name == "id") { + id = value; + } else if (name == "size") { + unsigned parsed_size = 0; + if (llvm::to_integer(value, parsed_size)) + size = parsed_size; + else { + LLDB_LOG(log, + "ProcessGDBRemote::ParseFlags Invalid size \"{0}\" " + "in flags node", + value.data()); + } + } else { + LLDB_LOG(log, + "ProcessGDBRemote::ParseFlags Ignoring unknown " + "attribute \"{0}\" in flags node", + name.data()); + } + return true; // Walk all attributes. + }); + + if (id && size) { + // Process the fields of this set of flags. + std::vector<RegisterFlags::Field> fields = + ParseFlagsFields(flags_node, *size); + if (fields.size()) { + // Sort so that the fields with the MSBs are first. + std::sort(fields.rbegin(), fields.rend()); + std::vector<RegisterFlags::Field>::const_iterator overlap = + std::adjacent_find(fields.begin(), fields.end(), + [](const RegisterFlags::Field &lhs, + const RegisterFlags::Field &rhs) { + return lhs.Overlaps(rhs); + }); + + // If no fields overlap, use them. + if (overlap == fields.end()) { + if (registers_flags_types.find(*id) != + registers_flags_types.end()) { + // In theory you could define some flag set, use it with a + // register then redefine it. We do not know if anyone does + // that, or what they would expect to happen in that case. + // + // LLDB chooses to take the first definition and ignore the rest + // as waiting until everything has been processed is more + // expensive and difficult. This means that pointers to flag + // sets in the register info remain valid if later the flag set + // is redefined. If we allowed redefinitions, LLDB would crash + // when you tried to print a register that used the original + // definition. + LLDB_LOG( + log, + "ProcessGDBRemote::ParseFlags Definition of flags " + "\"{0}\" shadows " + "previous definition, using original definition instead.", + id->data()); + } else { + registers_flags_types.insert_or_assign( + *id, std::make_unique<RegisterFlags>(id->str(), *size, + std::move(fields))); + } + } else { + // If any fields overlap, ignore the whole set of flags. + std::vector<RegisterFlags::Field>::const_iterator next = + std::next(overlap); + LLDB_LOG( + log, + "ProcessGDBRemote::ParseFlags Ignoring flags because fields " + "{0} (start: {1} end: {2}) and {3} (start: {4} end: {5}) " + "overlap.", + overlap->GetName().c_str(), overlap->GetStart(), + overlap->GetEnd(), next->GetName().c_str(), next->GetStart(), + next->GetEnd()); + } + } else { + LLDB_LOG( + log, + "ProcessGDBRemote::ParseFlags Ignoring definition of flags " + "\"{0}\" because it contains no fields.", + id->data()); + } + } + + return true; // Keep iterating through all "flags" elements. + }); +} + +bool ParseRegisters( + XMLNode feature_node, GdbServerTargetInfo &target_info, + std::vector<DynamicRegisterInfo::Register> ®isters, + llvm::StringMap<std::unique_ptr<RegisterFlags>> ®isters_flags_types) { if (!feature_node) return false; Log *log(GetLog(GDBRLog::Process)); + ParseFlags(feature_node, registers_flags_types); + for (const auto &flags : registers_flags_types) + flags.second->log(log); + feature_node.ForEachChildElementWithName( - "reg", [&target_info, ®isters, log](const XMLNode ®_node) -> bool { + "reg", + [&target_info, ®isters, ®isters_flags_types, + log](const XMLNode ®_node) -> bool { std::string gdb_group; std::string gdb_type; DynamicRegisterInfo::Register reg_info; @@ -4111,29 +4311,40 @@ return true; // Keep iterating through all attributes }); - if (!gdb_type.empty() && !(encoding_set || format_set)) { - if (llvm::StringRef(gdb_type).startswith("int")) { - reg_info.format = eFormatHex; - reg_info.encoding = eEncodingUint; - } else if (gdb_type == "data_ptr" || gdb_type == "code_ptr") { - reg_info.format = eFormatAddressInfo; - reg_info.encoding = eEncodingUint; - } else if (gdb_type == "float") { - reg_info.format = eFormatFloat; - reg_info.encoding = eEncodingIEEE754; - } else if (gdb_type == "aarch64v" || - llvm::StringRef(gdb_type).startswith("vec") || - gdb_type == "i387_ext" || gdb_type == "uint128") { - // lldb doesn't handle 128-bit uints correctly (for ymm*h), so treat - // them as vector (similarly to xmm/ymm) - reg_info.format = eFormatVectorOfUInt8; - reg_info.encoding = eEncodingVector; - } else { - LLDB_LOGF( - log, - "ProcessGDBRemote::ParseRegisters Could not determine lldb" - "format and encoding for gdb type %s", - gdb_type.c_str()); + if (!gdb_type.empty()) { + // gdb_type could reference some flags type defined in XML. + llvm::StringMap<std::unique_ptr<RegisterFlags>>::iterator it = + registers_flags_types.find(gdb_type); + if (it != registers_flags_types.end()) + reg_info.flags_type = it->second.get(); + + // There's a slim chance that the gdb_type name is both a flags type + // and a simple type. Just in case, look for that too (setting both + // does no harm). + if (!gdb_type.empty() && !(encoding_set || format_set)) { + if (llvm::StringRef(gdb_type).startswith("int")) { + reg_info.format = eFormatHex; + reg_info.encoding = eEncodingUint; + } else if (gdb_type == "data_ptr" || gdb_type == "code_ptr") { + reg_info.format = eFormatAddressInfo; + reg_info.encoding = eEncodingUint; + } else if (gdb_type == "float") { + reg_info.format = eFormatFloat; + reg_info.encoding = eEncodingIEEE754; + } else if (gdb_type == "aarch64v" || + llvm::StringRef(gdb_type).startswith("vec") || + gdb_type == "i387_ext" || gdb_type == "uint128") { + // lldb doesn't handle 128-bit uints correctly (for ymm*h), so + // treat them as vector (similarly to xmm/ymm) + reg_info.format = eFormatVectorOfUInt8; + reg_info.encoding = eEncodingVector; + } else { + LLDB_LOGF( + log, + "ProcessGDBRemote::ParseRegisters Could not determine lldb" + "format and encoding for gdb type %s", + gdb_type.c_str()); + } } } @@ -4265,8 +4476,8 @@ if (arch_to_use.IsValid()) { for (auto &feature_node : feature_nodes) { - ParseRegisters(feature_node, target_info, - registers); + ParseRegisters(feature_node, target_info, registers, + m_registers_flags_types); } for (const auto &include : target_info.includes) { @@ -4331,6 +4542,13 @@ if (!m_gdb_comm.GetQXferFeaturesReadSupported()) return false; + // 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(); std::vector<DynamicRegisterInfo::Register> registers; if (GetGDBServerRegisterInfoXMLAndProcess(arch_to_use, "target.xml", registers)) Index: lldb/include/lldb/lldb-private-types.h =================================================================== --- lldb/include/lldb/lldb-private-types.h +++ lldb/include/lldb/lldb-private-types.h @@ -11,6 +11,7 @@ #if defined(__cplusplus) +#include "lldb/Target/RegisterFlags.h" #include "lldb/lldb-private.h" #include "llvm/ADT/ArrayRef.h" @@ -62,8 +63,8 @@ /// this register changes. For example, the invalidate list for eax would be /// rax ax, ah, and al. uint32_t *invalidate_regs; - // Will be replaced with register flags info in the next patch. - void *unused; + /// If not nullptr, a type defined by XML descriptions. + const RegisterFlags *flags_type; llvm::ArrayRef<uint8_t> data(const uint8_t *context_base) const { return llvm::ArrayRef<uint8_t>(context_base + byte_offset, byte_size); Index: lldb/include/lldb/Target/DynamicRegisterInfo.h =================================================================== --- lldb/include/lldb/Target/DynamicRegisterInfo.h +++ lldb/include/lldb/Target/DynamicRegisterInfo.h @@ -12,6 +12,7 @@ #include <map> #include <vector> +#include "lldb/Target/RegisterFlags.h" #include "lldb/Utility/ConstString.h" #include "lldb/Utility/StructuredData.h" #include "lldb/lldb-private.h" @@ -39,6 +40,8 @@ std::vector<uint32_t> value_regs; std::vector<uint32_t> invalidate_regs; uint32_t value_reg_offset = 0; + // Non-null if there is an XML provided type. + const RegisterFlags *flags_type = nullptr; }; DynamicRegisterInfo() = default;
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits