[Lldb-commits] [lldb] b1b70f6 - [lldb-server] Add setting to force 'g' packet use
Pavel Labath via lldb-commits
lldb-commits at lists.llvm.org
Thu Nov 7 01:49:52 PST 2019
Author: Guilherme Andrade
Date: 2019-11-07T10:48:54+01:00
New Revision: b1b70f6761266c3eecaf8bd71529eaf51994207b
URL: https://github.com/llvm/llvm-project/commit/b1b70f6761266c3eecaf8bd71529eaf51994207b
DIFF: https://github.com/llvm/llvm-project/commit/b1b70f6761266c3eecaf8bd71529eaf51994207b.diff
LOG: [lldb-server] Add setting to force 'g' packet use
Following up on https://reviews.llvm.org/D62221, this change introduces
the settings plugin.process.gdb-remote.use-g-packet-for-reading. When
they are on, 'g' packets are used for reading registers.
Using 'g' packets can improve performance by reducing the number of
packets exchanged between client and server when a large number of
registers needs to be fetched.
Differential revision: https://reviews.llvm.org/D62931
Added:
Modified:
lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py
lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemoteProperties.td
lldb/source/Plugins/Process/gdb-remote/ThreadGDBRemote.cpp
Removed:
################################################################################
diff --git a/lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py b/lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py
index 3bf0c52edaed..fd13b1f2cd64 100644
--- a/lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py
+++ b/lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py
@@ -7,6 +7,18 @@
class TestGDBRemoteClient(GDBRemoteTestBase):
+ class gPacketResponder(MockGDBServerResponder):
+ def readRegisters(self):
+ return '0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000'
+
+ def setUp(self):
+ super(TestGDBRemoteClient, self).setUp()
+ self._initial_platform = lldb.DBG.GetSelectedPlatform()
+
+ def tearDown(self):
+ lldb.DBG.SetSelectedPlatform(self._initial_platform)
+ super(TestGDBRemoteClient, self).tearDown()
+
def test_connect(self):
"""Test connecting to a remote gdb server"""
target = self.createTarget("a.yaml")
@@ -37,3 +49,75 @@ def vAttach(self, pid):
error = lldb.SBError()
target.AttachToProcessWithID(lldb.SBListener(), 47, error)
self.assertEquals(error_msg, error.GetCString())
+
+ def test_read_registers_using_g_packets(self):
+ """Test reading registers using 'g' packets (default behavior)"""
+ self.server.responder = self.gPacketResponder()
+ target = self.createTarget("a.yaml")
+ process = self.connect(target)
+
+ self.assertEquals(1, self.server.responder.packetLog.count("g"))
+ self.server.responder.packetLog = []
+ self.read_registers(process)
+ # Reading registers should not cause any 'p' packets to be exchanged.
+ self.assertEquals(
+ 0, len([p for p in self.server.responder.packetLog if p.startswith("p")]))
+
+ def test_read_registers_using_p_packets(self):
+ """Test reading registers using 'p' packets"""
+ self.dbg.HandleCommand(
+ "settings set plugin.process.gdb-remote.use-g-packet-for-reading false")
+ target = self.createTarget("a.yaml")
+ process = self.connect(target)
+
+ self.read_registers(process)
+ self.assertFalse("g" in self.server.responder.packetLog)
+ self.assertGreater(
+ len([p for p in self.server.responder.packetLog if p.startswith("p")]), 0)
+
+ def test_write_registers_using_P_packets(self):
+ """Test writing registers using 'P' packets (default behavior)"""
+ self.server.responder = self.gPacketResponder()
+ target = self.createTarget("a.yaml")
+ process = self.connect(target)
+
+ self.write_registers(process)
+ self.assertEquals(0, len(
+ [p for p in self.server.responder.packetLog if p.startswith("G")]))
+ self.assertGreater(
+ len([p for p in self.server.responder.packetLog if p.startswith("P")]), 0)
+
+ def test_write_registers_using_G_packets(self):
+ """Test writing registers using 'G' packets"""
+
+ class MyResponder(self.gPacketResponder):
+ def readRegister(self, register):
+ # empty string means unsupported
+ return ""
+
+ self.server.responder = MyResponder()
+ target = self.createTarget("a.yaml")
+ process = self.connect(target)
+
+ self.write_registers(process)
+ self.assertEquals(0, len(
+ [p for p in self.server.responder.packetLog if p.startswith("P")]))
+ self.assertGreater(len(
+ [p for p in self.server.responder.packetLog if p.startswith("G")]), 0)
+
+ def read_registers(self, process):
+ self.for_each_gpr(
+ process, lambda r: self.assertEquals("0x00000000", r.GetValue()))
+
+ def write_registers(self, process):
+ self.for_each_gpr(
+ process, lambda r: r.SetValueFromCString("0x00000000"))
+
+ def for_each_gpr(self, process, operation):
+ registers = process.GetThreadAtIndex(0).GetFrameAtIndex(0).GetRegisters()
+ self.assertGreater(registers.GetSize(), 0)
+ regSet = registers[0]
+ numChildren = regSet.GetNumChildren()
+ self.assertGreater(numChildren, 0)
+ for i in range(numChildren):
+ operation(regSet.GetChildAtIndex(i))
diff --git a/lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py b/lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
index 8237d1e07121..7caee175ae79 100644
--- a/lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
+++ b/lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
@@ -115,7 +115,7 @@ def respond(self, packet):
return self.readRegister(int(regnum, 16))
if packet[0] == "P":
register, value = packet[1:].split("=")
- return self.readRegister(int(register, 16), value)
+ return self.writeRegister(int(register, 16), value)
if packet[0] == "m":
addr, length = [int(x, 16) for x in packet[1:].split(',')]
return self.readMemory(addr, length)
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
index feb9f0589cee..905ebe24a684 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -543,21 +543,24 @@ GDBRemoteCommunicationClient::SendThreadSpecificPacketAndWaitForResponse(
//
// Takes a valid thread ID because p needs to apply to a thread.
bool GDBRemoteCommunicationClient::GetpPacketSupported(lldb::tid_t tid) {
- if (m_supports_p == eLazyBoolCalculate) {
- m_supports_p = eLazyBoolNo;
- StreamString payload;
- payload.PutCString("p0");
- StringExtractorGDBRemote response;
- if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload),
- response, false) ==
- PacketResult::Success &&
- response.IsNormalResponse()) {
- m_supports_p = eLazyBoolYes;
- }
- }
+ if (m_supports_p == eLazyBoolCalculate)
+ m_supports_p = GetThreadPacketSupported(tid, "p0");
return m_supports_p;
}
+LazyBool GDBRemoteCommunicationClient::GetThreadPacketSupported(
+ lldb::tid_t tid, llvm::StringRef packetStr) {
+ StreamString payload;
+ payload.PutCString(packetStr);
+ StringExtractorGDBRemote response;
+ if (SendThreadSpecificPacketAndWaitForResponse(
+ tid, std::move(payload), response, false) == PacketResult::Success &&
+ response.IsNormalResponse()) {
+ return eLazyBoolYes;
+ }
+ return eLazyBoolNo;
+}
+
StructuredData::ObjectSP GDBRemoteCommunicationClient::GetThreadsInfo() {
// Get information on all threads at one using the "jThreadsInfo" packet
StructuredData::ObjectSP object_sp;
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
index 574cd0fd70c5..6539286b1a45 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
@@ -596,6 +596,8 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase {
Status GetQXferMemoryMapRegionInfo(lldb::addr_t addr,
MemoryRegionInfo ®ion);
+ LazyBool GetThreadPacketSupported(lldb::tid_t tid, llvm::StringRef packetStr);
+
private:
DISALLOW_COPY_AND_ASSIGN(GDBRemoteCommunicationClient);
};
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
index c06c9527708e..6fc61c0f30a3 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
@@ -31,9 +31,11 @@ using namespace lldb_private::process_gdb_remote;
// GDBRemoteRegisterContext constructor
GDBRemoteRegisterContext::GDBRemoteRegisterContext(
ThreadGDBRemote &thread, uint32_t concrete_frame_idx,
- GDBRemoteDynamicRegisterInfo ®_info, bool read_all_at_once)
+ GDBRemoteDynamicRegisterInfo ®_info, bool read_all_at_once,
+ bool write_all_at_once)
: RegisterContext(thread, concrete_frame_idx), m_reg_info(reg_info),
- m_reg_valid(), m_reg_data(), m_read_all_at_once(read_all_at_once) {
+ m_reg_valid(), m_reg_data(), m_read_all_at_once(read_all_at_once),
+ m_write_all_at_once(write_all_at_once) {
// Resize our vector of bools to contain one bool for every register. We will
// use these boolean values to know when a register value is valid in
// m_reg_data.
@@ -333,7 +335,7 @@ bool GDBRemoteRegisterContext::WriteRegisterBytes(const RegisterInfo *reg_info,
{
GDBRemoteClientBase::Lock lock(gdb_comm, false);
if (lock) {
- if (m_read_all_at_once) {
+ if (m_write_all_at_once) {
// Invalidate all register values
InvalidateIfNeeded(true);
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h
index 25e9b716f8cb..b42c87b5991b 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h
@@ -41,7 +41,7 @@ class GDBRemoteRegisterContext : public RegisterContext {
public:
GDBRemoteRegisterContext(ThreadGDBRemote &thread, uint32_t concrete_frame_idx,
GDBRemoteDynamicRegisterInfo ®_info,
- bool read_all_at_once);
+ bool read_all_at_once, bool write_all_at_once);
~GDBRemoteRegisterContext() override;
@@ -114,6 +114,7 @@ class GDBRemoteRegisterContext : public RegisterContext {
std::vector<bool> m_reg_valid;
DataExtractor m_reg_data;
bool m_read_all_at_once;
+ bool m_write_all_at_once;
private:
// Helper function for ReadRegisterBytes().
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index f1762abc55f8..9e459cf3382b 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -154,6 +154,11 @@ class PluginProperties : public Properties {
nullptr, idx,
g_processgdbremote_properties[idx].default_uint_value != 0);
}
+
+ bool GetUseGPacketForReading() const {
+ const uint32_t idx = ePropertyUseGPacketForReading;
+ return m_collection_sp->GetPropertyAtIndexAsBoolean(nullptr, idx, true);
+ }
};
typedef std::shared_ptr<PluginProperties> ProcessKDPPropertiesSP;
@@ -309,6 +314,9 @@ ProcessGDBRemote::ProcessGDBRemote(lldb::TargetSP target_sp,
GetGlobalPluginProperties()->GetPacketTimeout();
if (timeout_seconds > 0)
m_gdb_comm.SetPacketTimeout(std::chrono::seconds(timeout_seconds));
+
+ m_use_g_packet_for_reading =
+ GetGlobalPluginProperties()->GetUseGPacketForReading();
}
// Destructor
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
index 0e3e3b39d9c8..9ea3940103b6 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -284,6 +284,7 @@ class ProcessGDBRemote : public Process,
lldb::CommandObjectSP m_command_sp;
int64_t m_breakpoint_pc_offset;
lldb::tid_t m_initial_tid; // The initial thread ID, given by stub on attach
+ bool m_use_g_packet_for_reading;
bool m_replay_mode;
bool m_allow_flash_writes;
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemoteProperties.td b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemoteProperties.td
index 16e7723e3061..4c8202945501 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemoteProperties.td
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemoteProperties.td
@@ -13,4 +13,8 @@ let Definition = "processgdbremote" in {
Global,
DefaultFalse,
Desc<"If true, the libraries-svr4 feature will be used to get a hold of the process's loaded modules.">;
+ def UseGPacketForReading: Property<"use-g-packet-for-reading", "Boolean">,
+ Global,
+ DefaultTrue,
+ Desc<"Specify if the server should use 'g' packets to read registers.">;
}
diff --git a/lldb/source/Plugins/Process/gdb-remote/ThreadGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ThreadGDBRemote.cpp
index 8a6a58c55450..9da481979f73 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ThreadGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ThreadGDBRemote.cpp
@@ -301,13 +301,14 @@ ThreadGDBRemote::CreateRegisterContextForFrame(StackFrame *frame) {
if (process_sp) {
ProcessGDBRemote *gdb_process =
static_cast<ProcessGDBRemote *>(process_sp.get());
- // read_all_registers_at_once will be true if 'p' packet is not
- // supported.
+ bool pSupported =
+ gdb_process->GetGDBRemote().GetpPacketSupported(GetID());
bool read_all_registers_at_once =
- !gdb_process->GetGDBRemote().GetpPacketSupported(GetID());
+ !pSupported || gdb_process->m_use_g_packet_for_reading;
+ bool write_all_registers_at_once = !pSupported;
reg_ctx_sp = std::make_shared<GDBRemoteRegisterContext>(
*this, concrete_frame_idx, gdb_process->m_register_info,
- read_all_registers_at_once);
+ read_all_registers_at_once, write_all_registers_at_once);
}
} else {
Unwind *unwinder = GetUnwinder();
More information about the lldb-commits
mailing list