Re: [Lldb-commits] [PATCH] D23604: gdb-remote: Centralize thread specific packet handling
This revision was automatically updated to reflect the committed changes. Closed by commit rL279040: gdb-remote: Centralize thread specific packet handling (authored by labath). Changed prior to commit: https://reviews.llvm.org/D23604?vs=68337&id=68495#toc Repository: rL LLVM https://reviews.llvm.org/D23604 Files: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h lldb/trunk/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp Index: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h === --- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h +++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h @@ -633,6 +633,10 @@ void OnRunPacketSent(bool first) override; +PacketResult +SendThreadSpecificPacketAndWaitForResponse(lldb::tid_t tid, StreamString &&payload, + StringExtractorGDBRemote &response, bool send_async); + private: DISALLOW_COPY_AND_ASSIGN (GDBRemoteCommunicationClient); }; Index: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp === --- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp +++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp @@ -574,6 +574,31 @@ 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. @@ -584,18 +609,15 @@ { 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; +m_supports_p = eLazyBoolYes; } } return m_supports_p; @@ -3473,108 +3495,42 @@ 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(payl
Re: [Lldb-commits] [PATCH] D23604: gdb-remote: Centralize thread specific packet handling
clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Looks good. https://reviews.llvm.org/D23604 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D23604: gdb-remote: Centralize thread specific packet handling
labath created this revision. labath added a reviewer: clayborg. labath added a subscriber: lldb-commits. 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. https://reviews.llvm.org/D23604 Files: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp Index: unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp === --- unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp +++ unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp @@ -54,6 +54,8 @@ ASSERT_EQ(PacketResult::Success, server.SendPacket(response)); } +const char all_registers[] = "404142434445464748494a4b4c4d4e4f"; + } // end anonymous namespace class GDBRemoteCommunicationClientTest : public GDBRemoteTest @@ -78,10 +80,9 @@ 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()); } @@ -103,9 +104,58 @@ 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 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 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()); +} Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h === --- source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h +++ source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h @@ -633,6 +633,10 @@ void OnRunPacketSent(bool first) override; +PacketResult +SendThreadSpecificPacketAndWaitForResponse(lldb::tid_t tid, StreamString &&payload, +