From 02ef0f5ab483875b7b6b38e24b245e4fd4053959 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20G=C3=B3rny?= Date: Wed, 14 Apr 2021 11:56:09 +0200 Subject: [PATCH] [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 --- .../GDBRemoteCommunicationClient.cpp | 80 +++++++------------ .../gdb-remote/GDBRemoteCommunicationClient.h | 2 + .../GDBRemoteCommunicationClientTest.cpp | 4 +- 3 files changed, 33 insertions(+), 53 deletions(-) diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp index 0e529d221495..ec320543a782 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 +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::SetCurrentThread(uint64_t tid) { + if (m_curr_tid == tid) + return true; + + llvm::Optional ret = SendSetCurrentThreadPacket(tid, 'g'); + if (ret.hasValue()) + m_curr_tid = ret.getValue(); + return ret.hasValue(); } bool GDBRemoteCommunicationClient::SetCurrentThreadForRun(uint64_t tid) { if (m_curr_tid_run == 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; - } - - /* - * 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 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 fa67a6c69a53..03704dfdd8cf 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h @@ -336,6 +336,8 @@ public: // and response times. bool SendSpeedTestPacket(uint32_t send_size, uint32_t recv_size); + llvm::Optional 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 b9fc107527a2..45e0356c4948 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);