[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

Author: gbuenoandrade

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))

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)

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;

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h

@@ -596,6 +596,8 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase {
   Status GetQXferMemoryMapRegionInfo(lldb::addr_t addr,
                                      MemoryRegionInfo &region);
 
+  LazyBool GetThreadPacketSupported(lldb::tid_t tid, llvm::StringRef packetStr);
+
 private:
   DISALLOW_COPY_AND_ASSIGN(GDBRemoteCommunicationClient);
 };

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 &reg_info, bool read_all_at_once)
+    GDBRemoteDynamicRegisterInfo &reg_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);
 

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 &reg_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().

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

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;

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.">;
 }

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();