Index: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h =================================================================== --- source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h +++ source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h @@ -14,6 +14,8 @@ #include +#include "llvm/Support/RWMutex.h" + namespace lldb_private { namespace process_gdb_remote @@ -33,34 +35,16 @@ HandleStopReply() = 0; }; - GDBRemoteClientBase(const char *comm_name, const char *listener_name); - - bool - SendAsyncSignal(int signo); - - bool - Interrupt(); - - lldb::StateType - SendContinuePacketAndWaitForResponse(ContinueDelegate &delegate, const UnixSignals &signals, - llvm::StringRef payload, StringExtractorGDBRemote &response); - - PacketResult - SendPacketAndWaitForResponse(const char *payload, size_t len, StringExtractorGDBRemote &response, bool send_async) - { - return SendPacketAndWaitForResponse(llvm::StringRef(payload, len), response, send_async); - } - - PacketResult - SendPacketAndWaitForResponse(llvm::StringRef payload, StringExtractorGDBRemote &response, bool send_async); - - bool - SendvContPacket(llvm::StringRef payload, StringExtractorGDBRemote &response); - class Lock { public: - Lock(GDBRemoteClientBase &comm, bool interrupt); + enum Kind + { + Shared, + Exclusive + }; + + Lock(GDBRemoteClientBase &comm, Kind kind = Exclusive, bool interrupt = false); ~Lock(); explicit operator bool() { return m_acquired; } @@ -73,8 +57,8 @@ } private: - std::unique_lock m_async_lock; GDBRemoteClientBase &m_comm; + Kind m_kind; bool m_acquired; bool m_did_interrupt; @@ -82,6 +66,31 @@ SyncWithContinueThread(bool interrupt); }; + GDBRemoteClientBase(const char *comm_name, const char *listener_name); + + bool + SendAsyncSignal(int signo); + + bool + Interrupt(); + + lldb::StateType + SendContinuePacketAndWaitForResponse(ContinueDelegate &delegate, const UnixSignals &signals, + llvm::StringRef payload, StringExtractorGDBRemote &response); + + PacketResult + SendPacketAndWaitForResponse(const char *payload, size_t len, StringExtractorGDBRemote &response, bool send_async) + { + return SendPacketAndWaitForResponse(llvm::StringRef(payload, len), response, Lock::Exclusive, send_async); + } + + PacketResult + SendPacketAndWaitForResponse(llvm::StringRef payload, StringExtractorGDBRemote &response, + Lock::Kind kind = Lock::Exclusive, bool send_async = false); + + bool + SendvContPacket(llvm::StringRef payload, StringExtractorGDBRemote &response); + protected: PacketResult SendPacketAndWaitForResponseNoLock(llvm::StringRef payload, StringExtractorGDBRemote &response); @@ -114,13 +123,21 @@ bool m_should_stop; // Whether we should resume after a stop. // end of continue thread synchronization block - // This handles the synchronization between individual async threads. For now they just use a - // simple mutex. - std::recursive_mutex m_async_mutex; + // This handles the synchronization between individual async threads. Either many threads can + // share the connection, or one thread has exclusive access. + llvm::sys::RWMutex m_async_mutex; + + // The data structures for matching up concurrent requests and responses. + std::mutex m_queue_mutex; + std::condition_variable m_queue_cv; + std::queue m_queue; bool ShouldStop(const UnixSignals &signals, StringExtractorGDBRemote &response); + PacketResult + ReadAndValidatePacket(StringExtractorGDBRemote &response); + class ContinueLock { public: Index: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp =================================================================== --- source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp +++ source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp @@ -140,7 +140,7 @@ bool GDBRemoteClientBase::SendAsyncSignal(int signo) { - Lock lock(*this, true); + Lock lock(*this, Lock::Exclusive, true); if (!lock || !lock.DidInterrupt()) return false; @@ -153,7 +153,7 @@ bool GDBRemoteClientBase::Interrupt() { - Lock lock(*this, true); + Lock lock(*this, Lock::Exclusive, true); if (!lock.DidInterrupt()) return false; m_should_stop = true; @@ -161,9 +161,9 @@ } GDBRemoteCommunication::PacketResult GDBRemoteClientBase::SendPacketAndWaitForResponse(llvm::StringRef payload, StringExtractorGDBRemote &response, - bool send_async) + Lock::Kind kind, bool send_async) { - Lock lock(*this, send_async); + Lock lock(*this, kind, send_async); if (!lock) { if (Log *log = ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS)) @@ -178,25 +178,49 @@ GDBRemoteCommunication::PacketResult GDBRemoteClientBase::SendPacketAndWaitForResponseNoLock(llvm::StringRef payload, StringExtractorGDBRemote &response) { - PacketResult packet_result = SendPacketNoLock(payload.data(), payload.size()); - if (packet_result != PacketResult::Success) - return packet_result; + PacketResult packet_result; + { + std::unique_lock lock(m_queue_mutex); + + packet_result = SendPacketNoLock(payload.data(), payload.size()); + if (packet_result != PacketResult::Success) + return packet_result; + + // Any unique value would work here. Address of a local variable is guaranteed to satisfy that. + m_queue.push(&packet_result); + m_cv.wait(lock, [this, &packet_result] { return m_queue.front() == &packet_result; }); + } + + // Do the blocking part (reading) with the queue lock released. + packet_result = ReadAndValidatePacket(response); + + { + std::lock_guard lock(m_queue_mutex); + lldbassert(m_queue.front() == &packet_result); + m_queue.pop(); + } + m_queue_cv.notify_all(); + return packet_result; +} +GDBRemoteCommunication::PacketResult +GDBRemoteClientBase::ReadAndValidatePacket(StringExtractorGDBRemote &response) +{ + PacketResult packet_result; const size_t max_response_retries = 3; for (size_t i = 0; i < max_response_retries; ++i) { packet_result = ReadPacket(response, GetPacketTimeoutInMicroSeconds(), true); // Make sure we received a response if (packet_result != PacketResult::Success) - return packet_result; + break; // Make sure our response is valid for the payload that was sent if (response.ValidateResponse()) - return packet_result; + break; // Response says it wasn't valid Log *log = ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PACKETS); if (log) - log->Printf("error: packet with payload \"%.*s\" got invalid response \"%s\": %s", int(payload.size()), - payload.data(), response.GetStringRef().c_str(), + log->Printf("error: got invalid response \"%s\": %s", response.GetStringRef().c_str(), (i == (max_response_retries - 1)) ? "using invalid response and giving up" : "ignoring response and waiting for another"); } @@ -211,7 +235,7 @@ log->Printf("GDBRemoteCommunicationClient::%s ()", __FUNCTION__); // we want to lock down packet sending while we continue - Lock lock(*this, true); + Lock lock(*this, Lock::Exclusive, true); if (log) log->Printf("GDBRemoteCommunicationClient::%s () sending vCont packet: %.*s", __FUNCTION__, int(payload.size()), @@ -330,12 +354,17 @@ // GDBRemoteClientBase::Lock // /////////////////////////////// -GDBRemoteClientBase::Lock::Lock(GDBRemoteClientBase &comm, bool interrupt) - : m_async_lock(comm.m_async_mutex, std::defer_lock), m_comm(comm), m_acquired(false), m_did_interrupt(false) +GDBRemoteClientBase::Lock::Lock(GDBRemoteClientBase &comm, Kind kind, bool interrupt) + : m_comm(comm), m_kind(kind), m_acquired(false), m_did_interrupt(false) { SyncWithContinueThread(interrupt); - if (m_acquired) - m_async_lock.lock(); + if (!m_acquired) + return; + + if (m_kind == Shared) + m_comm.m_async_mutex.lock_shared(); + else + m_comm.m_async_mutex.lock(); } void @@ -376,6 +405,12 @@ { if (!m_acquired) return; + + if (m_kind == Shared) + m_comm.m_async_mutex.unlock_shared(); + else + m_comm.m_async_mutex.unlock(); + { std::unique_lock lock(m_comm.m_mutex); --m_comm.m_async_count; Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h =================================================================== --- source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h +++ source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h @@ -209,19 +209,10 @@ ~History (); - // For single char packets for ack, nack and /x03 void - AddPacket (char packet_char, - PacketType type, - uint32_t bytes_transmitted); + AddPacket(llvm::StringRef packet, PacketType type, uint32_t bytes_transmitted); void - AddPacket (const std::string &src, - uint32_t src_len, - PacketType type, - uint32_t bytes_transmitted); - - void Dump (Stream &strm) const; void @@ -230,6 +221,7 @@ bool DidDumpToLog () const { + std::lock_guard lock(m_mutex); return m_dumped_to_log; } @@ -267,6 +259,7 @@ return i % m_packets.size(); } + mutable std::mutex m_mutex; std::vector m_packets; uint32_t m_curr_idx; uint32_t m_total_packet_count; Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp =================================================================== --- source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp +++ source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp @@ -70,33 +70,14 @@ } void -GDBRemoteCommunication::History::AddPacket (char packet_char, - PacketType type, - uint32_t bytes_transmitted) +GDBRemoteCommunication::History::AddPacket(llvm::StringRef packet, PacketType type, uint32_t bytes_transmitted) { + std::lock_guard lock(m_mutex); const size_t size = m_packets.size(); if (size > 0) { const uint32_t idx = GetNextIndex(); - m_packets[idx].packet.assign (1, packet_char); - m_packets[idx].type = type; - m_packets[idx].bytes_transmitted = bytes_transmitted; - m_packets[idx].packet_idx = m_total_packet_count; - m_packets[idx].tid = Host::GetCurrentThreadID(); - } -} - -void -GDBRemoteCommunication::History::AddPacket (const std::string &src, - uint32_t src_len, - PacketType type, - uint32_t bytes_transmitted) -{ - const size_t size = m_packets.size(); - if (size > 0) - { - const uint32_t idx = GetNextIndex(); - m_packets[idx].packet.assign (src, 0, src_len); + m_packets[idx].packet = packet; m_packets[idx].type = type; m_packets[idx].bytes_transmitted = bytes_transmitted; m_packets[idx].packet_idx = m_total_packet_count; @@ -107,6 +88,7 @@ void GDBRemoteCommunication::History::Dump (Stream &strm) const { + std::lock_guard lock(m_mutex); const uint32_t size = GetNumPacketsInHistory (); const uint32_t first_idx = GetFirstSavedPacketIndex (); const uint32_t stop_idx = m_curr_idx + size; @@ -128,6 +110,7 @@ void GDBRemoteCommunication::History::Dump (Log *log) const { + std::lock_guard lock(m_mutex); if (log && !m_dumped_to_log) { m_dumped_to_log = true; @@ -206,7 +189,7 @@ const size_t bytes_written = Write (&ch, 1, status, NULL); if (log) log->Printf ("<%4" PRIu64 "> send packet: %c", (uint64_t)bytes_written, ch); - m_history.AddPacket (ch, History::ePacketTypeSend, bytes_written); + m_history.AddPacket(llvm::StringRef(&ch, 1), History::ePacketTypeSend, bytes_written); return bytes_written; } @@ -219,7 +202,7 @@ const size_t bytes_written = Write (&ch, 1, status, NULL); if (log) log->Printf("<%4" PRIu64 "> send packet: %c", (uint64_t)bytes_written, ch); - m_history.AddPacket (ch, History::ePacketTypeSend, bytes_written); + m_history.AddPacket(llvm::StringRef(&ch, 1), History::ePacketTypeSend, bytes_written); return bytes_written; } @@ -278,8 +261,7 @@ log->Printf("<%4" PRIu64 "> send packet: %.*s", (uint64_t)bytes_written, (int)packet_length, packet_data); } - m_history.AddPacket (packet.GetString(), packet_length, History::ePacketTypeSend, bytes_written); - + m_history.AddPacket(packet.GetString(), History::ePacketTypeSend, bytes_written); if (bytes_written == packet_length) { @@ -955,7 +937,7 @@ } } - m_history.AddPacket (m_bytes.c_str(), total_length, History::ePacketTypeRecv, total_length); + m_history.AddPacket(m_bytes, History::ePacketTypeRecv, total_length); // Clear packet_str in case there is some existing data in it. packet_str.clear(); Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp =================================================================== --- source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp +++ source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp @@ -253,7 +253,7 @@ GDBRemoteCommunication::ScopedTimeout timeout (*this, std::max (old_timeout, minimum_timeout)); StringExtractorGDBRemote response; - if (SendPacketAndWaitForResponse("QStartNoAckMode", response, false) == PacketResult::Success) + if (SendPacketAndWaitForResponse("QStartNoAckMode", response) == PacketResult::Success) { if (response.IsOKResponse()) { @@ -274,7 +274,7 @@ m_supports_threads_in_stop_reply = eLazyBoolNo; StringExtractorGDBRemote response; - if (SendPacketAndWaitForResponse("QListThreadsInStopReply", response, false) == PacketResult::Success) + if (SendPacketAndWaitForResponse("QListThreadsInStopReply", response) == PacketResult::Success) { if (response.IsOKResponse()) m_supports_threads_in_stop_reply = eLazyBoolYes; @@ -290,7 +290,7 @@ m_attach_or_wait_reply = eLazyBoolNo; StringExtractorGDBRemote response; - if (SendPacketAndWaitForResponse("qVAttachOrWaitSupported", response, false) == PacketResult::Success) + if (SendPacketAndWaitForResponse("qVAttachOrWaitSupported", response) == PacketResult::Success) { if (response.IsOKResponse()) m_attach_or_wait_reply = eLazyBoolYes; @@ -310,7 +310,7 @@ m_prepare_for_reg_writing_reply = eLazyBoolNo; StringExtractorGDBRemote response; - if (SendPacketAndWaitForResponse("qSyncThreadStateSupported", response, false) == PacketResult::Success) + if (SendPacketAndWaitForResponse("qSyncThreadStateSupported", response) == PacketResult::Success) { if (response.IsOKResponse()) m_prepare_for_reg_writing_reply = eLazyBoolYes; @@ -408,9 +408,7 @@ } StringExtractorGDBRemote response; - if (SendPacketAndWaitForResponse(packet.GetData(), - response, - /*send_async=*/false) == PacketResult::Success) + if (SendPacketAndWaitForResponse(packet.GetData(), response) == PacketResult::Success) { const char *response_cstr = response.GetStringRef().c_str(); if (::strstr (response_cstr, "qXfer:auxv:read+")) @@ -508,7 +506,7 @@ { StringExtractorGDBRemote response; m_supports_thread_suffix = eLazyBoolNo; - if (SendPacketAndWaitForResponse("QThreadSuffixSupported", response, false) == PacketResult::Success) + if (SendPacketAndWaitForResponse("QThreadSuffixSupported", response) == PacketResult::Success) { if (response.IsOKResponse()) m_supports_thread_suffix = eLazyBoolYes; @@ -528,7 +526,7 @@ m_supports_vCont_C = eLazyBoolNo; m_supports_vCont_s = eLazyBoolNo; m_supports_vCont_S = eLazyBoolNo; - if (SendPacketAndWaitForResponse("vCont?", response, false) == PacketResult::Success) + if (SendPacketAndWaitForResponse("vCont?", response) == PacketResult::Success) { const char *response_cstr = response.GetStringRef().c_str(); if (::strstr (response_cstr, ";c")) @@ -591,8 +589,8 @@ snprintf(packet, sizeof(packet), "p0;thread:%" PRIx64 ";", tid); else snprintf(packet, sizeof(packet), "p0"); - - if (SendPacketAndWaitForResponse(packet, response, false) == PacketResult::Success) + + if (SendPacketAndWaitForResponse(packet, response) == PacketResult::Success) { if (response.IsNormalResponse()) m_supports_p = eLazyBoolYes; @@ -611,7 +609,7 @@ { StringExtractorGDBRemote response; response.SetResponseValidatorToJSON(); - if (SendPacketAndWaitForResponse("jThreadsInfo", response, false) == PacketResult::Success) + if (SendPacketAndWaitForResponse("jThreadsInfo", response) == PacketResult::Success) { if (response.IsUnsupportedResponse()) { @@ -634,7 +632,7 @@ { StringExtractorGDBRemote response; m_supports_jThreadExtendedInfo = eLazyBoolNo; - if (SendPacketAndWaitForResponse("jThreadExtendedInfo:", response, false) == PacketResult::Success) + if (SendPacketAndWaitForResponse("jThreadExtendedInfo:", response) == PacketResult::Success) { if (response.IsOKResponse()) { @@ -652,7 +650,7 @@ { StringExtractorGDBRemote response; m_supports_jLoadedDynamicLibrariesInfos = eLazyBoolNo; - if (SendPacketAndWaitForResponse("jGetLoadedDynamicLibrariesInfos:", response, false) == PacketResult::Success) + if (SendPacketAndWaitForResponse("jGetLoadedDynamicLibrariesInfos:", response) == PacketResult::Success) { if (response.IsOKResponse()) { @@ -670,7 +668,7 @@ { StringExtractorGDBRemote response; m_supports_jGetSharedCacheInfo = eLazyBoolNo; - if (SendPacketAndWaitForResponse("jGetSharedCacheInfo:", response, false) == PacketResult::Success) + if (SendPacketAndWaitForResponse("jGetSharedCacheInfo:", response) == PacketResult::Success) { if (response.IsOKResponse()) { @@ -690,7 +688,7 @@ m_supports_x = eLazyBoolNo; char packet[256]; snprintf (packet, sizeof (packet), "x0,0"); - if (SendPacketAndWaitForResponse(packet, response, false) == PacketResult::Success) + if (SendPacketAndWaitForResponse(packet, response) == PacketResult::Success) { if (response.IsOKResponse()) m_supports_x = eLazyBoolYes; @@ -706,7 +704,7 @@ std::string &response_string ) { - Lock lock(*this, false); + Lock lock(*this); if (!lock) { Log *log (ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet (GDBR_LOG_PROCESS | GDBR_LOG_PACKETS)); @@ -1097,7 +1095,7 @@ m_qGDBServerVersion_is_valid = eLazyBoolNo; StringExtractorGDBRemote response; - if (SendPacketAndWaitForResponse ("qGDBServerVersion", response, false) == PacketResult::Success) + if (SendPacketAndWaitForResponse("qGDBServerVersion", response) == PacketResult::Success) { if (response.IsNormalResponse()) { @@ -1221,7 +1219,7 @@ { StringExtractorGDBRemote response; std::string packet = "QEnableCompression:type:" + avail_name + ";"; - if (SendPacketAndWaitForResponse (packet.c_str(), response, false) != PacketResult::Success) + if (SendPacketAndWaitForResponse(packet, response) != PacketResult::Success) return; if (response.IsOKResponse()) @@ -1254,7 +1252,7 @@ GDBRemoteCommunicationClient::GetDefaultThreadId (lldb::tid_t &tid) { StringExtractorGDBRemote response; - if (SendPacketAndWaitForResponse("qC",response,false) != PacketResult::Success) + if (SendPacketAndWaitForResponse("qC", response) != PacketResult::Success) return false; if (!response.IsNormalResponse()) @@ -1275,7 +1273,7 @@ { m_qHostInfo_is_valid = eLazyBoolNo; StringExtractorGDBRemote response; - if (SendPacketAndWaitForResponse ("qHostInfo", response, false) == PacketResult::Success) + if (SendPacketAndWaitForResponse("qHostInfo", response) == PacketResult::Success) { if (response.IsNormalResponse()) { @@ -1928,7 +1926,7 @@ GDBRemoteCommunicationClient::GetWorkingDir(FileSpec &working_dir) { StringExtractorGDBRemote response; - if (SendPacketAndWaitForResponse ("qGetWorkingDir", response, false) == PacketResult::Success) + if (SendPacketAndWaitForResponse("qGetWorkingDir", response) == PacketResult::Success) { if (response.IsUnsupportedResponse()) return false; @@ -2134,7 +2132,7 @@ GetHostInfo (); StringExtractorGDBRemote response; - if (SendPacketAndWaitForResponse ("qProcessInfo", response, false) == PacketResult::Success) + if (SendPacketAndWaitForResponse("qProcessInfo", response) == PacketResult::Success) { if (response.IsNormalResponse()) { @@ -2703,7 +2701,7 @@ connection_urls.clear(); StringExtractorGDBRemote response; - if (SendPacketAndWaitForResponse("qQueryGDBServer", response, false) != PacketResult::Success) + if (SendPacketAndWaitForResponse("qQueryGDBServer", response) != PacketResult::Success) return 0; StructuredData::ObjectSP data = StructuredData::ParseJSON(response.GetStringRef()); @@ -2919,7 +2917,7 @@ { thread_ids.clear(); - Lock lock(*this, false); + Lock lock(*this); if (lock) { sequence_mutex_unavailable = false; @@ -2976,11 +2974,11 @@ lldb::addr_t GDBRemoteCommunicationClient::GetShlibInfoAddr() { - Lock lock(*this, false); + Lock lock(*this); if (lock) { StringExtractorGDBRemote response; - if (SendPacketAndWaitForResponse("qShlibInfoAddr", ::strlen ("qShlibInfoAddr"), response, false) == PacketResult::Success) + if (SendPacketAndWaitForResponseNoLock("qShlibInfoAddr", response) == PacketResult::Success) { if (response.IsNormalResponse()) return response.GetHexMaxU64(false, LLDB_INVALID_ADDRESS); @@ -3473,7 +3471,7 @@ bool GDBRemoteCommunicationClient::ReadRegister(lldb::tid_t tid, uint32_t reg, StringExtractorGDBRemote &response) { - Lock lock(*this, false); + Lock lock(*this); if (lock) { const bool thread_suffix_supported = GetThreadSuffixSupported(); @@ -3487,7 +3485,7 @@ else packet_len = ::snprintf (packet, sizeof(packet), "p%x", reg); assert (packet_len < ((int)sizeof(packet) - 1)); - return SendPacketAndWaitForResponse(packet, response, false) == PacketResult::Success; + return SendPacketAndWaitForResponse(packet, response) == PacketResult::Success; } } else if (Log *log = ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_PROCESS | GDBR_LOG_PACKETS)) @@ -3502,7 +3500,7 @@ bool GDBRemoteCommunicationClient::ReadAllRegisters (lldb::tid_t tid, StringExtractorGDBRemote &response) { - Lock lock(*this, false); + Lock lock(*this); if (lock) { const bool thread_suffix_supported = GetThreadSuffixSupported(); @@ -3517,7 +3515,7 @@ else packet_len = ::snprintf (packet, sizeof(packet), "g"); assert (packet_len < ((int)sizeof(packet) - 1)); - return SendPacketAndWaitForResponse(packet, response, false) == PacketResult::Success; + return SendPacketAndWaitForResponseNoLock(packet, response) == PacketResult::Success; } } else if (Log *log = ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_PROCESS | GDBR_LOG_PACKETS)) @@ -3534,7 +3532,7 @@ return false; m_supports_QSaveRegisterState = eLazyBoolYes; - Lock lock(*this, false); + Lock lock(*this); if (lock) { const bool thread_suffix_supported = GetThreadSuffixSupported(); @@ -3548,7 +3546,7 @@ StringExtractorGDBRemote response; - if (SendPacketAndWaitForResponse(packet, response, false) == PacketResult::Success) + if (SendPacketAndWaitForResponseNoLock(packet, response) == PacketResult::Success) { if (response.IsUnsupportedResponse()) { @@ -3582,7 +3580,7 @@ if (m_supports_QSaveRegisterState == eLazyBoolNo) return false; - Lock lock(*this, false); + Lock lock(*this); if (lock) { const bool thread_suffix_supported = GetThreadSuffixSupported(); @@ -3595,8 +3593,8 @@ ::snprintf (packet, sizeof(packet), "QRestoreRegisterState:%u" PRIx64 ";", save_id); StringExtractorGDBRemote response; - - if (SendPacketAndWaitForResponse(packet, response, false) == PacketResult::Success) + + if (SendPacketAndWaitForResponseNoLock(packet, response) == PacketResult::Success) { if (response.IsOKResponse()) { @@ -3731,10 +3729,7 @@ << std::hex << offset << "," << std::hex << size; - GDBRemoteCommunication::PacketResult res = - SendPacketAndWaitForResponse( packet.str().c_str(), - chunk, - false ); + GDBRemoteCommunication::PacketResult res = SendPacketAndWaitForResponse(packet.str(), chunk); if ( res != GDBRemoteCommunication::PacketResult::Success ) { err.SetErrorString( "Error sending $qXfer packet" ); @@ -3818,7 +3813,7 @@ if (m_supports_qSymbol && m_qSymbol_requests_done == false) { - Lock lock(*this, false); + Lock lock(*this); if (lock) { StreamString packet; Index: source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp =================================================================== --- source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp +++ source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp @@ -403,8 +403,7 @@ reg_info->byte_size, // dst length m_reg_data.GetByteOrder())) // dst byte order { - GDBRemoteClientBase::Lock lock(gdb_comm, false); - if (lock) + if (true) { const bool thread_suffix_supported = gdb_comm.GetThreadSuffixSupported(); ProcessSP process_sp (m_thread.GetProcess()); @@ -572,8 +571,7 @@ const bool use_g_packet = gdb_comm.AvoidGPackets ((ProcessGDBRemote *)process) == false; - GDBRemoteClientBase::Lock lock(gdb_comm, false); - if (lock) + if (true) { SyncThreadState(process); @@ -681,8 +679,7 @@ const bool use_g_packet = gdb_comm.AvoidGPackets ((ProcessGDBRemote *)process) == false; StringExtractorGDBRemote response; - GDBRemoteClientBase::Lock lock(gdb_comm, false); - if (lock) + if (true) { const bool thread_suffix_supported = gdb_comm.GetThreadSuffixSupported(); ProcessSP process_sp (m_thread.GetProcess()); Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp =================================================================== --- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -528,7 +528,8 @@ const int packet_len = ::snprintf (packet, sizeof(packet), "qRegisterInfo%x", reg_num); assert (packet_len < (int)sizeof(packet)); StringExtractorGDBRemote response; - if (m_gdb_comm.SendPacketAndWaitForResponse(packet, packet_len, response, false) == GDBRemoteCommunication::PacketResult::Success) + if (m_gdb_comm.SendPacketAndWaitForResponse(llvm::StringRef(packet, packet_len), response) == + GDBRemoteCommunication::PacketResult::Success) { response_type = response.GetResponseType(); if (response_type == StringExtractorGDBRemote::eResponse) @@ -1163,7 +1164,7 @@ for (size_t idx = 0; idx < num_cmds; idx++) { StringExtractorGDBRemote response; - m_gdb_comm.SendPacketAndWaitForResponse (GetExtraStartupCommands().GetArgumentAtIndex(idx), response, false); + m_gdb_comm.SendPacketAndWaitForResponse(GetExtraStartupCommands().GetArgumentAtIndex(idx), response); } return error; } @@ -1647,7 +1648,7 @@ { // Send vStopped StringExtractorGDBRemote response; - m_gdb_comm.SendPacketAndWaitForResponse("vStopped", response, false); + m_gdb_comm.SendPacketAndWaitForResponse("vStopped", response); // OK represents end of signal list if (response.IsOKResponse()) @@ -5030,7 +5031,8 @@ packet.PutCStringAsRawHex8(file_path.c_str()); StringExtractorGDBRemote response; - if (m_gdb_comm.SendPacketAndWaitForResponse(packet.GetString().c_str(), response, false) != GDBRemoteCommunication::PacketResult::Success) + if (m_gdb_comm.SendPacketAndWaitForResponse(packet.GetString(), response) != + GDBRemoteCommunication::PacketResult::Success) return Error("Sending qFileLoadAddress packet failed"); if (response.IsErrorResponse()) @@ -5405,7 +5407,8 @@ const char *packet_cstr = command.GetArgumentAtIndex(0); bool send_async = true; StringExtractorGDBRemote response; - process->GetGDBRemote().SendPacketAndWaitForResponse(packet_cstr, response, send_async); + process->GetGDBRemote().SendPacketAndWaitForResponse(packet_cstr, response, + GDBRemoteClientBase::Lock::Exclusive, send_async); result.SetStatus (eReturnStatusSuccessFinishResult); Stream &output_strm = result.GetOutputStream(); output_strm.Printf (" packet: %s\n", packet_cstr); @@ -5464,7 +5467,8 @@ bool send_async = true; StringExtractorGDBRemote response; - process->GetGDBRemote().SendPacketAndWaitForResponse(packet_cstr, response, send_async); + process->GetGDBRemote().SendPacketAndWaitForResponse(packet_cstr, response, + GDBRemoteClientBase::Lock::Exclusive, send_async); result.SetStatus (eReturnStatusSuccessFinishResult); Stream &output_strm = result.GetOutputStream(); output_strm.Printf (" packet: %s\n", packet_cstr); Index: unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp =================================================================== --- unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp +++ unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp @@ -235,6 +235,7 @@ { StringExtractorGDBRemote continue_response, async_response, response; const bool send_async = true; + const GDBRemoteClientBase::Lock::Kind send_kind = GDBRemoteClientBase::Lock::Exclusive; ContinueFixture fix; if (HasFailure()) return; @@ -247,10 +248,11 @@ fix.WaitForRunEvent(); // Sending without async enabled should fail. - ASSERT_EQ(PacketResult::ErrorSendFailed, fix.client.SendPacketAndWaitForResponse("qTest1", response, !send_async)); + ASSERT_EQ(PacketResult::ErrorSendFailed, + fix.client.SendPacketAndWaitForResponse("qTest1", response, send_kind, !send_async)); std::future async_result = std::async(std::launch::async, [&] { - return fix.client.SendPacketAndWaitForResponse("qTest2", async_response, send_async); + return fix.client.SendPacketAndWaitForResponse("qTest2", async_response, send_kind, send_async); }); // First we'll get interrupted. @@ -341,6 +343,7 @@ { StringExtractorGDBRemote continue_response, async_response, response; const bool send_async = true; + const GDBRemoteClientBase::Lock::Kind send_kind = GDBRemoteClientBase::Lock::Exclusive; ContinueFixture fix; if (HasFailure()) return; @@ -370,7 +373,7 @@ // Packet stream should remain synchronized. std::future send_result = std::async(std::launch::async, [&] { - return fix.client.SendPacketAndWaitForResponse("qTest", async_response, !send_async); + return fix.client.SendPacketAndWaitForResponse("qTest", async_response, send_kind, !send_async); }); ASSERT_EQ(PacketResult::Success, fix.server.GetPacket(response)); ASSERT_EQ("qTest", response.GetStringRef()); @@ -426,3 +429,81 @@ ASSERT_TRUE(async_result.get()); ASSERT_EQ(eStateInvalid, continue_state.get()); } + +TEST(GDBRemoteClientBaseTest, SendTwoPacketsInterleaved) +{ + StringExtractorGDBRemote async_response1, async_response2, response1, response2; + const GDBRemoteClientBase::Lock::Kind send_kind = GDBRemoteClientBase::Lock::Shared; + ContinueFixture fix; + if (HasFailure()) + return; + + std::future async_result1 = std::async(std::launch::async, [&] { + return fix.client.SendPacketAndWaitForResponse("qTest1", async_response1, send_kind); + }); + + std::future async_result2 = std::async(std::launch::async, [&] { + return fix.client.SendPacketAndWaitForResponse("qTest2", async_response2, send_kind); + }); + + // Make sure we can get both requests before we send out the response. The order in which + // they come is non-deterministic. + ASSERT_EQ(PacketResult::Success, fix.server.GetPacket(response1)); + ASSERT_TRUE(response1.GetStringRef() == "qTest1" || response1.GetStringRef() == "qTest2"); + ASSERT_EQ(PacketResult::Success, fix.server.GetPacket(response2)); + ASSERT_TRUE(response2.GetStringRef() == "qTest1" || response2.GetStringRef() == "qTest2"); + ASSERT_NE(response1.GetStringRef(), response2.GetStringRef()); + + // Send both responses (in the correct order). + ASSERT_EQ(PacketResult::Success, fix.server.SendPacket(response1.GetStringRef() + "X")); + ASSERT_EQ(PacketResult::Success, fix.server.SendPacket(response2.GetStringRef() + "X")); + + // And make sure they get received. + ASSERT_EQ(PacketResult::Success, async_result1.get()); + ASSERT_EQ("qTest1X", async_response1.GetStringRef()); + ASSERT_EQ(PacketResult::Success, async_result2.get()); + ASSERT_EQ("qTest2X", async_response2.GetStringRef()); +} + +TEST(GDBRemoteClientBaseTest, SendManyPacketsStress) +{ + StringExtractorGDBRemote async_response1, async_response2, response1, response2; + const GDBRemoteClientBase::Lock::Kind send_kind = GDBRemoteClientBase::Lock::Shared; + ContinueFixture fix; + if (HasFailure()) + return; + + // Fire up the senders. + std::vector packet_threads; + for (unsigned i = 0; i < 4; ++i) + { + packet_threads.emplace_back([i, &fix] { + std::ostringstream packet; + packet << "qTest" << i; + StringExtractorGDBRemote response; + for (unsigned j = 0; j < 10; ++j) + { + ASSERT_EQ(PacketResult::Success, + fix.client.SendPacketAndWaitForResponse(packet.str(), response, send_kind)); + ASSERT_EQ(packet.str(), response.GetStringRef()); + } + }); + } + + // Our "server" will just mirror the packets back at the senders. + std::thread mirror_thread([&] { + PacketResult result; + StringExtractorGDBRemote packet; + while ((result = fix.server.GetPacket(packet)) == PacketResult::Success) + ASSERT_EQ(PacketResult::Success, fix.server.SendPacket(packet.GetStringRef())); + ASSERT_EQ(PacketResult::ErrorDisconnected, result); + }); + + // Let the senders finish. + for (std::thread &t : packet_threads) + t.join(); + + // Close the client connection so the server thread can exit. + fix.client.Disconnect(); + mirror_thread.join(); +}