[Lldb-commits] [lldb] 02ef0f5 - [lldb] [gdb-remote client] Refactor SetCurrentThread*()

[Michał Górny via lldb-commits]

Author: Michał Górny
Date: 2021-07-02T14:36:17+02:00
New Revision: 02ef0f5ab483875b7b6b38e24b245e4fd4053959

URL: https://github.com/llvm/llvm-project/commit/02ef0f5ab483875b7b6b38e24b245e4fd4053959
DIFF: https://github.com/llvm/llvm-project/commit/02ef0f5ab483875b7b6b38e24b245e4fd4053959.diff

LOG: [lldb] [gdb-remote client] Refactor SetCurrentThread*()

Refactor SetCurrentThread() and SetCurrentThreadForRun() to reduce code
duplication and simplify it.  Both methods now call common
SendSetCurrentThreadPacket() that implements the common protocol
exchange part (the only variable is sending `Hg` vs `Hc`) and returns
the selected TID.  The logic is rewritten to use a StreamString
instead of snprintf().

A side effect of the change is that thread-id sent is now zero-padded.
However, this should not have practical impact on the server as both
forms are equivalent.

Differential Revision: https://reviews.llvm.org/D100459

Added: 
    

Modified: 
    lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
    lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
    lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp

Removed: 
    


################################################################################
diff  --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
index 0e529d221495e..ec320543a7828 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -2639,25 +2639,21 @@ bool GDBRemoteCommunicationClient::KillSpawnedProcess(lldb::pid_t pid) {
   return false;
 }
 
-bool GDBRemoteCommunicationClient::SetCurrentThread(uint64_t tid) {
-  if (m_curr_tid == tid)
-    return true;
-
-  char packet[32];
-  int packet_len;
+llvm::Optional<uint64_t>
+GDBRemoteCommunicationClient::SendSetCurrentThreadPacket(uint64_t tid,
+                                                         char op) {
+  lldb_private::StreamString packet;
+  packet.PutChar('H');
+  packet.PutChar(op);
   if (tid == UINT64_MAX)
-    packet_len = ::snprintf(packet, sizeof(packet), "Hg-1");
+    packet.PutCString("-1");
   else
-    packet_len = ::snprintf(packet, sizeof(packet), "Hg%" PRIx64, tid);
-  assert(packet_len + 1 < (int)sizeof(packet));
-  UNUSED_IF_ASSERT_DISABLED(packet_len);
+    packet.PutHex64(tid);
   StringExtractorGDBRemote response;
-  if (SendPacketAndWaitForResponse(packet, response, false) ==
+  if (SendPacketAndWaitForResponse(packet.GetString(), response, false) ==
       PacketResult::Success) {
-    if (response.IsOKResponse()) {
-      m_curr_tid = tid;
-      return true;
-    }
+    if (response.IsOKResponse())
+      return tid;
 
     /*
      * Connected bare-iron target (like YAMON gdb-stub) may not have support for
@@ -2665,49 +2661,31 @@ bool GDBRemoteCommunicationClient::SetCurrentThread(uint64_t tid) {
      * The reply from '?' packet could be as simple as 'S05'. There is no packet
      * which can
      * give us pid and/or tid. Assume pid=tid=1 in such cases.
-    */
-    if (response.IsUnsupportedResponse() && IsConnected()) {
-      m_curr_tid = 1;
-      return true;
-    }
+     */
+    if (response.IsUnsupportedResponse() && IsConnected())
+      return 1;
   }
-  return false;
+  return llvm::None;
 }
 
-bool GDBRemoteCommunicationClient::SetCurrentThreadForRun(uint64_t tid) {
-  if (m_curr_tid_run == tid)
+bool GDBRemoteCommunicationClient::SetCurrentThread(uint64_t tid) {
+  if (m_curr_tid == tid)
     return true;
 
-  char packet[32];
-  int packet_len;
-  if (tid == UINT64_MAX)
-    packet_len = ::snprintf(packet, sizeof(packet), "Hc-1");
-  else
-    packet_len = ::snprintf(packet, sizeof(packet), "Hc%" PRIx64, tid);
+  llvm::Optional<uint64_t> ret = SendSetCurrentThreadPacket(tid, 'g');
+  if (ret.hasValue())
+    m_curr_tid = ret.getValue();
+  return ret.hasValue();
+}
 
-  assert(packet_len + 1 < (int)sizeof(packet));
-  UNUSED_IF_ASSERT_DISABLED(packet_len);
-  StringExtractorGDBRemote response;
-  if (SendPacketAndWaitForResponse(packet, response, false) ==
-      PacketResult::Success) {
-    if (response.IsOKResponse()) {
-      m_curr_tid_run = tid;
-      return true;
-    }
+bool GDBRemoteCommunicationClient::SetCurrentThreadForRun(uint64_t tid) {
+  if (m_curr_tid_run == tid)
+    return true;
 
-    /*
-     * Connected bare-iron target (like YAMON gdb-stub) may not have support for
-     * Hc packet.
-     * The reply from '?' packet could be as simple as 'S05'. There is no packet
-     * which can
-     * give us pid and/or tid. Assume pid=tid=1 in such cases.
-    */
-    if (response.IsUnsupportedResponse() && IsConnected()) {
-      m_curr_tid_run = 1;
-      return true;
-    }
-  }
-  return false;
+  llvm::Optional<uint64_t> ret = SendSetCurrentThreadPacket(tid, 'c');
+  if (ret.hasValue())
+    m_curr_tid_run = ret.getValue();
+  return ret.hasValue();
 }
 
 bool GDBRemoteCommunicationClient::GetStopReply(

diff  --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
index fa67a6c69a535..03704dfdd8cf0 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
@@ -336,6 +336,8 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase {
   // and response times.
   bool SendSpeedTestPacket(uint32_t send_size, uint32_t recv_size);
 
+  llvm::Optional<uint64_t> SendSetCurrentThreadPacket(uint64_t tid, char op);
+
   bool SetCurrentThread(uint64_t tid);
 
   bool SetCurrentThreadForRun(uint64_t tid);

diff  --git a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
index b9fc107527a21..45e0356c49486 100644
--- a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
+++ b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
@@ -98,7 +98,7 @@ TEST_F(GDBRemoteCommunicationClientTest, WriteRegisterNoSuffix) {
   });
 
   Handle_QThreadSuffixSupported(server, false);
-  HandlePacket(server, "Hg47", "OK");
+  HandlePacket(server, "Hg0000000000000047", "OK");
   HandlePacket(server, "P4=" + one_register_hex, "OK");
   ASSERT_TRUE(write_result.get());
 
@@ -143,7 +143,7 @@ TEST_F(GDBRemoteCommunicationClientTest, SaveRestoreRegistersNoSuffix) {
     return client.SaveRegisterState(tid, save_id);
   });
   Handle_QThreadSuffixSupported(server, false);
-  HandlePacket(server, "Hg47", "OK");
+  HandlePacket(server, "Hg0000000000000047", "OK");
   HandlePacket(server, "QSaveRegisterState", "1");
   ASSERT_TRUE(async_result.get());
   EXPECT_EQ(1u, save_id);