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
