[Lldb-commits] [lldb] 54981bb - [lldb] Read register fields from target XML
David Spickett via lldb-commits
lldb-commits at lists.llvm.org
Thu Apr 13 05:34:21 PDT 2023
Author: David Spickett
Date: 2023-04-13T12:34:14Z
New Revision: 54981bb75d374863c6a15530018c6d7ac5be6478
URL: https://github.com/llvm/llvm-project/commit/54981bb75d374863c6a15530018c6d7ac5be6478
DIFF: https://github.com/llvm/llvm-project/commit/54981bb75d374863c6a15530018c6d7ac5be6478.diff
LOG: [lldb] Read register fields from target XML
This teaches ProcessGDBRemote to look for "flags" nodes
in the target XML that tell you what fields a register has.
https://sourceware.org/gdb/onlinedocs/gdb/Target-Description-Format.html
It will check for various invalid inputs like:
* Flags nodes with 0 fields in them.
* Start or end being > the register size.
* Fields that overlap.
* Required properties not being present (e.g. no name).
* Flag sets being redefined.
If anything untoward is found, we'll just drop the field or the
flag set altogether. Register fields are a "nice to have" so LLDB
shouldn't be crashing because of them, instead just log anything
we throw away. So the user can fix their XML/file a bug with their
vendor.
Once that is done it will sort the fields and pass them to
the RegisterFields class I added previously.
There is no way to see these fields yet, so tests for this code
will come later when the formatting code is added.
The fields are stored in a map of unique pointers on the
ProcessGDBRemote class. It will give out raw pointers on the
assumption that the GDB process lives longer than the users
of those pointers do. Which means RegisterInfo is still a trivial struct
but we are properly destroying the fields when the GDB process ends.
We can't store the fields directly in the map because adding new
items may cause its storage to be reallocated, which would invalidate
pointers we've already given out.
Reviewed By: jasonmolenda, JDevlieghere
Differential Revision: https://reviews.llvm.org/D145574
Added:
Modified:
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
Removed:
################################################################################
diff --git a/lldb/include/lldb/Target/DynamicRegisterInfo.h b/lldb/include/lldb/Target/DynamicRegisterInfo.h
index 20f442529da8e..1b1df848315ce 100644
--- a/lldb/include/lldb/Target/DynamicRegisterInfo.h
+++ b/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 @@ class DynamicRegisterInfo {
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;
diff --git a/lldb/include/lldb/lldb-private-types.h b/lldb/include/lldb/lldb-private-types.h
index 94f08496426f4..b89b8eae0d124 100644
--- a/lldb/include/lldb/lldb-private-types.h
+++ b/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 @@ struct RegisterInfo {
/// 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);
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index 41d453a4acb3a..95bc79ebdfc9d 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/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"
@@ -4059,15 +4061,213 @@ struct GdbServerTargetInfo {
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
diff icult. 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;
@@ -4143,29 +4343,40 @@ bool ParseRegisters(XMLNode feature_node, GdbServerTargetInfo &target_info,
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());
+ }
}
}
@@ -4297,8 +4508,8 @@ bool ProcessGDBRemote::GetGDBServerRegisterInfoXMLAndProcess(
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) {
@@ -4363,6 +4574,13 @@ 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.
+ // 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))
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
index 39065e5f87d35..b04ebf20a0a39 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ b/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 @@ class ProcessGDBRemote : public Process,
// 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
diff --git a/lldb/source/Target/DynamicRegisterInfo.cpp b/lldb/source/Target/DynamicRegisterInfo.cpp
index 31bd647fe80f9..b4d02a0c9bb1c 100644
--- a/lldb/source/Target/DynamicRegisterInfo.cpp
+++ b/lldb/source/Target/DynamicRegisterInfo.cpp
@@ -407,7 +407,7 @@ size_t DynamicRegisterInfo::SetRegisterInfo(
{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);
More information about the lldb-commits
mailing list