gdb-remote: Centralize thread specific packet handling

[labath]

Summary

Before this, each function had a copy of the code which handled appending of the thread suffix to
the packet (or using $Hg instead). I have moved that code into a single function and made
everyone else use that. The function takes the partial packet as a StreamString rvalue reference,
to avoid a copy and to remind the users that the packet will have undeterminate contents after
the call.

This also fixes the incorrect formatting of the QRestoreRegisterState packet in case thread
suffix is not supported.

[clayborg]

Looks good.

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

protected:
 
     bool
     DecodeProcessInfoResponse (StringExtractorGDBRemote &response, 
                                ProcessInstanceInfo &process_info);
 
     void
     OnRunPacketSent(bool first) override;
 
+    PacketResult
+    SendThreadSpecificPacketAndWaitForResponse(lldb::tid_t tid, StreamString &&payload,
+                                               StringExtractorGDBRemote &response, bool send_async);
+
 private:
     DISALLOW_COPY_AND_ASSIGN (GDBRemoteCommunicationClient);
 };
 
 } // namespace process_gdb_remote
 } // namespace lldb_private
 
 #endif // liblldb_GDBRemoteCommunicationClient_h_

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

GDBRemoteCommunicationClient::GetVContSupported (char flavor)
     case 'C': return m_supports_vCont_C;
     case 's': return m_supports_vCont_s;
     case 'S': return m_supports_vCont_S;
     default: break;
     }
     return false;
 }
 
+GDBRemoteCommunication::PacketResult
+GDBRemoteCommunicationClient::SendThreadSpecificPacketAndWaitForResponse(lldb::tid_t tid, StreamString &&payload,
+                                                                         StringExtractorGDBRemote &response,
+                                                                         bool send_async)
+{
+    Lock lock(*this, send_async);
+    if (!lock)
+    {
+        if (Log *log = ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_PROCESS | GDBR_LOG_PACKETS))
+            log->Printf("GDBRemoteCommunicationClient::%s: Didn't get sequence mutex for %s packet.", __FUNCTION__,
+                        payload.GetString().c_str());
+        return PacketResult::ErrorNoSequenceLock;
+    }
+
+    if (GetThreadSuffixSupported())
+        payload.Printf(";thread:%4.4" PRIx64 ";", tid);
+    else
+    {
+        if (!SetCurrentThread(tid))
+            return PacketResult::ErrorSendFailed;
+    }
+
+    return SendPacketAndWaitForResponseNoLock(payload.GetString(), response);
+}
+
 // Check if the target supports 'p' packet. It sends out a 'p'
 // packet and checks the response. A normal packet will tell us
 // that support is available.
 //
 // 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)
     {
-        StringExtractorGDBRemote response;
         m_supports_p = eLazyBoolNo;
-        char packet[256];
-        if (GetThreadSuffixSupported())
-            snprintf(packet, sizeof(packet), "p0;thread:%" PRIx64 ";", tid);
-        else
-            snprintf(packet, sizeof(packet), "p0");
-        
-        if (SendPacketAndWaitForResponse(packet, response, false) == PacketResult::Success)
+        StreamString payload;
+        payload.PutCString("p0");
+        StringExtractorGDBRemote response;
+        if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, false) ==
+                PacketResult::Success &&
+            response.IsNormalResponse())
         {
-            if (response.IsNormalResponse())
-                m_supports_p = eLazyBoolYes;
         }
     }
     return m_supports_p;
 }
 
 StructuredData::ObjectSP
 GDBRemoteCommunicationClient::GetThreadsInfo()
 {
if (m_avoid_g_packets == eLazyBoolCalculate)
         }
     }
     return m_avoid_g_packets == eLazyBoolYes;
 }
 
 bool
 GDBRemoteCommunicationClient::ReadRegister(lldb::tid_t tid, uint32_t reg, StringExtractorGDBRemote &response)
 {
-    Lock lock(*this, false);
-    if (lock)
-    {
-        const bool thread_suffix_supported = GetThreadSuffixSupported();
-        
-        if (thread_suffix_supported || SetCurrentThread(tid))
-        {
-            char packet[64];
-            int packet_len = 0;
-            if (thread_suffix_supported)
-                packet_len = ::snprintf (packet, sizeof(packet), "p%x;thread:%4.4" PRIx64 ";", reg, tid);
-            else
-                packet_len = ::snprintf (packet, sizeof(packet), "p%x", reg);
-            assert (packet_len < ((int)sizeof(packet) - 1));
-            return SendPacketAndWaitForResponse(packet, response, false) == PacketResult::Success;
-        }
-    }
-    else if (Log *log = ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_PROCESS | GDBR_LOG_PACKETS))
-    {
-        log->Printf("GDBRemoteCommunicationClient::%s: Didn't get sequence mutex for p packet.", __FUNCTION__);
-    }
-    return false;
-
+    StreamString payload;
+    payload.Printf("p%x", reg);
+    return SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, false) ==
+           PacketResult::Success;
 }
 
 
 bool
 GDBRemoteCommunicationClient::ReadAllRegisters (lldb::tid_t tid, StringExtractorGDBRemote &response)
 {
-    Lock lock(*this, false);
-    if (lock)
-    {
-        const bool thread_suffix_supported = GetThreadSuffixSupported();
-
-        if (thread_suffix_supported || SetCurrentThread(tid))
-        {
-            char packet[64];
-            int packet_len = 0;
-            // Get all registers in one packet
-            if (thread_suffix_supported)
-                packet_len = ::snprintf (packet, sizeof(packet), "g;thread:%4.4" PRIx64 ";", tid);
-            else
-                packet_len = ::snprintf (packet, sizeof(packet), "g");
-            assert (packet_len < ((int)sizeof(packet) - 1));
-            return SendPacketAndWaitForResponse(packet, response, false) == PacketResult::Success;
-        }
-    }
-    else if (Log *log = ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_PROCESS | GDBR_LOG_PACKETS))
-    {
-        log->Printf("GDBRemoteCommunicationClient::%s: Didn't get sequence mutex for g packet.", __FUNCTION__);
-    }
-    return false;
+    StreamString payload;
+    payload.PutChar('g');
+    return SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, false) ==
+           PacketResult::Success;
 }
 
 bool
 GDBRemoteCommunicationClient::WriteRegister(lldb::tid_t tid, uint32_t reg_num, llvm::StringRef data)
 {
-    Lock lock(*this, false);
-    if (!lock)
-    {
-        if (Log *log = ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_PROCESS | GDBR_LOG_PACKETS))
-            log->Printf("GDBRemoteCommunicationClient::%s: Didn't get sequence mutex for P packet.", __FUNCTION__);
-        return false;
-    }
-
-    const bool thread_suffix_supported = GetThreadSuffixSupported();
-    if (!thread_suffix_supported && !SetCurrentThread(tid))
-        return false;
-
-    StreamString packet;
-    packet.Printf("P%x=", reg_num);
-    packet.PutBytesAsRawHex8(data.data(), data.size(), endian::InlHostByteOrder(), endian::InlHostByteOrder());
-    if (thread_suffix_supported)
-        packet.Printf(";thread:%4.4" PRIx64 ";", tid);
+    StreamString payload;
+    payload.Printf("P%x=", reg_num);
+    payload.PutBytesAsRawHex8(data.data(), data.size(), endian::InlHostByteOrder(), endian::InlHostByteOrder());
     StringExtractorGDBRemote response;
-    return SendPacketAndWaitForResponse(packet.GetData(), packet.GetSize(), response, false) == PacketResult::Success &&
-           response.IsOKResponse();
+    return SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, false) ==
+           PacketResult::Success;
 }
 
 bool
 GDBRemoteCommunicationClient::WriteAllRegisters(lldb::tid_t tid, llvm::StringRef data)
 {
-    Lock lock(*this, false);
-    if (!lock)
-    {
-        if (Log *log = ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_PROCESS | GDBR_LOG_PACKETS))
-            log->Printf("GDBRemoteCommunicationClient::%s: Didn't get sequence mutex for G packet.", __FUNCTION__);
-        return false;
-    }
-
-    const bool thread_suffix_supported = GetThreadSuffixSupported();
-    if (!thread_suffix_supported && !SetCurrentThread(tid))
-        return false;
-
-    StreamString packet;
-    packet.PutChar('G');
-    packet.Write(data.data(), data.size());
-    if (thread_suffix_supported)
-        packet.Printf(";thread:%4.4" PRIx64 ";", tid);
+    StreamString payload;
+    payload.PutChar('G');
+    payload.Write(data.data(), data.size());
     StringExtractorGDBRemote response;
-    return SendPacketAndWaitForResponse(packet.GetData(), packet.GetSize(), response, false) == PacketResult::Success &&
-           response.IsOKResponse();
+    return SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, false) ==
+           PacketResult::Success;
 }
 
 bool
 GDBRemoteCommunicationClient::SaveRegisterState (lldb::tid_t tid, uint32_t &save_id)
 {
     save_id = 0; // Set to invalid save ID
     if (m_supports_QSaveRegisterState == eLazyBoolNo)
         return false;
     
     m_supports_QSaveRegisterState = eLazyBoolYes;
-    Lock lock(*this, false);
-    if (lock)
-    {
-        const bool thread_suffix_supported = GetThreadSuffixSupported();
-        if (thread_suffix_supported || SetCurrentThread(tid))
-        {
-            char packet[256];
-            if (thread_suffix_supported)
-                ::snprintf (packet, sizeof(packet), "QSaveRegisterState;thread:%4.4" PRIx64 ";", tid);
-            else
-                ::snprintf(packet, sizeof(packet), "QSaveRegisterState");
-            
-            StringExtractorGDBRemote response;
+    StreamString payload;
+    payload.PutCString("QSaveRegisterState");
+    if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, false) != PacketResult::Success)
+        return false;
 
-            if (SendPacketAndWaitForResponse(packet, response, false) == PacketResult::Success)
-            {
-                if (response.IsUnsupportedResponse())
-                {
-                    // This packet isn't supported, don't try calling it again
-                    m_supports_QSaveRegisterState = eLazyBoolNo;
-                }
-                    
-                const uint32_t response_save_id = response.GetU32(0);
-                if (response_save_id != 0)
-                {
-                    save_id = response_save_id;
-                    return true;
-                }
-            }
-        }
-    }
-    else if (Log *log = ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_PROCESS | GDBR_LOG_PACKETS))
-    {
-        log->Printf("GDBRemoteCommunicationClient::%s: Didn't get sequence mutex for QSaveRegisterState packet.",
-                    __FUNCTION__);
-    }
-    return false;
-}
+    if (response_save_id == 0)
+        return false;
+
 
 bool
 GDBRemoteCommunicationClient::RestoreRegisterState (lldb::tid_t tid, uint32_t save_id)
 {
     // We use the "m_supports_QSaveRegisterState" variable here because the
     // QSaveRegisterState and QRestoreRegisterState packets must both be supported in
     // order to be useful
     if (m_supports_QSaveRegisterState == eLazyBoolNo)
         return false;
 
-    Lock lock(*this, false);
-    if (lock)
-    {
-        const bool thread_suffix_supported = GetThreadSuffixSupported();
-        if (thread_suffix_supported || SetCurrentThread(tid))
-        {
-            char packet[256];
-            if (thread_suffix_supported)
-                ::snprintf (packet, sizeof(packet), "QRestoreRegisterState:%u;thread:%4.4" PRIx64 ";", save_id, tid);
-            else
-                ::snprintf (packet, sizeof(packet), "QRestoreRegisterState:%u" PRIx64 ";", save_id);
-            
-            StringExtractorGDBRemote response;
-            
-            if (SendPacketAndWaitForResponse(packet, response, false) == PacketResult::Success)
-            {
-                if (response.IsOKResponse())
-                {
-                    return true;
-                }
-                else if (response.IsUnsupportedResponse())
-                {
-                    // This packet isn't supported, don't try calling this packet or
-                    // QSaveRegisterState again...
-                    m_supports_QSaveRegisterState = eLazyBoolNo;
-                }
-            }
-        }
-    }
-    else if (Log *log = ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_PROCESS | GDBR_LOG_PACKETS))
-    {
-        log->Printf("GDBRemoteCommunicationClient::%s: Didn't get sequence mutex for QRestoreRegisterState packet.",
-                    __FUNCTION__);
-    }
+    StreamString payload;
+    payload.Printf("QRestoreRegisterState:%u", save_id);
+    if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, false) != PacketResult::Success)
+        return false;
+
+    if (response.IsUnsupportedResponse())
     return false;
 }
 
 bool
 GDBRemoteCommunicationClient::GetModuleInfo (const FileSpec& module_file_spec,
                                              const lldb_private::ArchSpec& arch_spec,
                                              ModuleSpec &module_spec)
 {

unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp

 HandlePacket(MockServer &server, llvm::StringRef expected, llvm::StringRef response)
 {
     StringExtractorGDBRemote request;
     ASSERT_EQ(PacketResult::Success, server.GetPacket(request));
     ASSERT_EQ(expected, request.GetStringRef());
     ASSERT_EQ(PacketResult::Success, server.SendPacket(response));
 }
 
+const char all_registers[] = "404142434445464748494a4b4c4d4e4f";
+
 } // end anonymous namespace
 
 class GDBRemoteCommunicationClientTest : public GDBRemoteTest
 {
 };
 
 TEST_F(GDBRemoteCommunicationClientTest, WriteRegister)
 {
     TestClient client;
     MockServer server;
     Connect(client, server);
     if (HasFailure())
         return;
 
     const lldb::tid_t tid = 0x47;
     const uint32_t reg_num = 4;
     std::future<bool> write_result =
         std::async(std::launch::async, [&client] { return client.WriteRegister(tid, reg_num, "ABCD"); });
 
     Handle_QThreadSuffixSupported(server, true);
 
     HandlePacket(server, "P4=41424344;thread:0047;", "OK");
     ASSERT_TRUE(write_result.get());
 
-    write_result = std::async(std::launch::async,
-                              [&client] { return client.WriteAllRegisters(tid, "404142434445464748494a4b4c4d4e4f"); });
+    write_result = std::async(std::launch::async, [&client] { return client.WriteAllRegisters(tid, all_registers); });
 
-    HandlePacket(server, "G404142434445464748494a4b4c4d4e4f;thread:0047;", "OK");
+    HandlePacket(server, std::string("G") + all_registers + ";thread:0047;", "OK");
     ASSERT_TRUE(write_result.get());
 }
 
 TEST_F(GDBRemoteCommunicationClientTest, WriteRegisterNoSuffix)
 {
     TestClient client;
     MockServer server;
     Connect(client, server);
     if (HasFailure())
         return;
 
     const lldb::tid_t tid = 0x47;
     const uint32_t reg_num = 4;
     std::future<bool> write_result =
         std::async(std::launch::async, [&client] { return client.WriteRegister(tid, reg_num, "ABCD"); });
 
     Handle_QThreadSuffixSupported(server, false);
     HandlePacket(server, "Hg47", "OK");
     HandlePacket(server, "P4=41424344", "OK");
     ASSERT_TRUE(write_result.get());
 
-    write_result = std::async(std::launch::async,
-                              [&client] { return client.WriteAllRegisters(tid, "404142434445464748494a4b4c4d4e4f"); });
+    write_result = std::async(std::launch::async, [&client] { return client.WriteAllRegisters(tid, all_registers); });
 
-    HandlePacket(server, "G404142434445464748494a4b4c4d4e4f", "OK");
+    HandlePacket(server, std::string("G") + all_registers, "OK");
     ASSERT_TRUE(write_result.get());
 }
+
+TEST_F(GDBRemoteCommunicationClientTest, ReadRegister)
+{
+    TestClient client;
+    MockServer server;
+    Connect(client, server);
+    if (HasFailure())
+        return;
+
+    const lldb::tid_t tid = 0x47;
+    const uint32_t reg_num = 4;
+    std::future<bool> async_result = std::async(std::launch::async, [&] { return client.GetpPacketSupported(tid); });
+    Handle_QThreadSuffixSupported(server, true);
+    HandlePacket(server, "p0;thread:0047;", "41424344");
+    ASSERT_TRUE(async_result.get());
+
+    StringExtractorGDBRemote response;
+    async_result = std::async(std::launch::async, [&] { return client.ReadRegister(tid, reg_num, response); });
+    HandlePacket(server, "p4;thread:0047;", "41424344");
+    ASSERT_TRUE(async_result.get());
+    ASSERT_EQ("41424344", response.GetStringRef());
+
+    async_result = std::async(std::launch::async, [&] { return client.ReadAllRegisters(tid, response); });
+    HandlePacket(server, "g;thread:0047;", all_registers);
+    ASSERT_TRUE(async_result.get());
+    ASSERT_EQ(all_registers, response.GetStringRef());
+}
+
+TEST_F(GDBRemoteCommunicationClientTest, SaveRestoreRegistersNoSuffix)
+{
+    TestClient client;
+    MockServer server;
+    Connect(client, server);
+    if (HasFailure())
+        return;
+
+    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, "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");
+    ASSERT_TRUE(async_result.get());
+}