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

[mgorny]

Summary

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.

[rovka]

This looks good to me, but I'll let others comment on whether or not we want to send extra leading zeroes for the thread ID. In my opinion it would be nice to 1) be consistent between all packages that deal with thread ids, and 2) send as little clutter as possible. But these are just theoretical concerns, I'm not sure if it matters in practice.

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

339

Nitpick: You could call this 'op' instead of 'type', to match the documentation for the H packet.

[mgorny]

In D100459#2721989, @rovka wrote:

This looks good to me, but I'll let others comment on whether or not we want to send extra leading zeroes for the thread ID. In my opinion it would be nice to 1) be consistent between all packages that deal with thread ids, and 2) send as little clutter as possible. But these are just theoretical concerns, I'm not sure if it matters in practice.

To be honest, I didn't want to touch it because I don't know if one of the other uses of PutHex64() doesn't actually require fixed-width output.

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

339

Sounds like a good idea. Thanks!

[mgorny]

type → op.

[JDevlieghere]

LGTM

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

2648

Should this be LLDB_INVALID_THREAD_ID?

[mgorny]

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

2648

No, LLDB_INVALID_THREAD_ID is 0, while here we expect -1. In fact, the original functions are called with literal -1 that gets converted to UINT64_MAX. I find it kinda ugly but I suppose it's a matter of taste.

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

public:
 
   // This packet is for testing the speed of the interface only. Both
   // the client and server need to support it, but this allows us to
   // measure the packet speed without any other work being done on the
   // other end and avoids any of that work affecting the packet send
   // 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);
 
   bool GetQXferAuxvReadSupported();
 
   void EnableErrorStringInPacket();
 

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

bool GDBRemoteCommunicationClient::KillSpawnedProcess(lldb::pid_t pid) {
   if (SendPacketAndWaitForResponse(stream.GetString(), response, false) ==
       PacketResult::Success) {
     if (response.IsOKResponse())
       return true;
   }
   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
      * Hg 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 = 1;
-      return true;
-    }
-  }
-  return false;
+    if (response.IsUnsupportedResponse() && IsConnected())
+      return 1;
+  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);
-
-  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;
-    }
+  llvm::Optional<uint64_t> ret = SendSetCurrentThreadPacket(tid, 'g');
+  if (ret.hasValue())
+    m_curr_tid = ret.getValue();
+  return ret.hasValue();
 
-    /*
-     * 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;
+bool GDBRemoteCommunicationClient::SetCurrentThreadForRun(uint64_t tid) {
+  if (m_curr_tid_run == tid)
+
+  llvm::Optional<uint64_t> ret = SendSetCurrentThreadPacket(tid, 'c');
+  if (ret.hasValue())
+    m_curr_tid_run = ret.getValue();
+  return ret.hasValue();
 }
 
 bool GDBRemoteCommunicationClient::GetStopReply(
     StringExtractorGDBRemote &response) {
   if (SendPacketAndWaitForResponse("?", response, false) ==
       PacketResult::Success)
     return response.IsNormalResponse();
   return false;

lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp

 TEST_F(GDBRemoteCommunicationClientTest, WriteRegisterNoSuffix) {
   const lldb::tid_t tid = 0x47;
   const uint32_t reg_num = 4;
   std::future<bool> write_result = std::async(std::launch::async, [&] {
     return client.WriteRegister(tid, reg_num, one_register);
   });
 
   Handle_QThreadSuffixSupported(server, false);
-  HandlePacket(server, "Hg47", "OK");
+  HandlePacket(server, "Hg0000000000000047", "OK");
   HandlePacket(server, "P4=" + one_register_hex, "OK");
   ASSERT_TRUE(write_result.get());
 
   write_result = std::async(std::launch::async, [&] {
     return client.WriteAllRegisters(tid, all_registers);
   });
 
   HandlePacket(server, "G" + all_registers_hex, "OK");

 
 TEST_F(GDBRemoteCommunicationClientTest, SaveRestoreRegistersNoSuffix) {
   const lldb::tid_t tid = 0x47;
   uint32_t save_id;
   std::future<bool> async_result = std::async(std::launch::async, [&] {
     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);
 
   async_result = std::async(std::launch::async, [&] {
     return client.RestoreRegisterState(tid, save_id);
   });
   HandlePacket(server, "QRestoreRegisterState:1", "OK");