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(); _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
