From 0faf37333c8bf7eaca3500c7d79c7fb99112caa5 Mon Sep 17 00:00:00 2001 From: Pavel Labath Date: Thu, 25 Aug 2016 08:34:57 +0000 Subject: [PATCH] gdb-remote: Make the sequence mutex non-recursive Summary: This is a preparatory commit for D22914, where I'd like to replace this mutex by an R/W lock (which is also not recursive). This required a couple of changes: - The only caller of Read/WriteRegister, GDBRemoteRegisterContext class, was already acquiring the mutex, so these functions do not need to. All functions which now do not take a lock, take an lock argument instead, to remind the caller of this fact. - GetThreadSuffixSupported() was being called from locked and unlocked contexts (including contexts where the process was running, and the call would fail if it did not have the result cached). I have split this into two functions, one which computes the thread suffix support and caches it (this one always takes the lock), and another, which returns the cached value (and never needs to take the lock). This feels quite natural as ProcessGdbRemote was already pre-caching this value at the start. Reviewers: clayborg Subscribers: lldb-commits Differential Revision: https://reviews.llvm.org/D23802 llvm-svn: 279725 --- .../Process/gdb-remote/GDBRemoteClientBase.cpp | 6 +- .../Process/gdb-remote/GDBRemoteClientBase.h | 8 +- .../gdb-remote/GDBRemoteCommunicationClient.cpp | 107 +++++++++++++-------- .../gdb-remote/GDBRemoteCommunicationClient.h | 20 ++-- .../gdb-remote/GDBRemoteRegisterContext.cpp | 66 +++++++------ .../Process/gdb-remote/GDBRemoteRegisterContext.h | 14 +-- .../Process/gdb-remote/ProcessGDBRemote.cpp | 2 +- .../GDBRemoteCommunicationClientTest.cpp | 42 +++++--- 8 files changed, 164 insertions(+), 101 deletions(-) diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp index fde3803..c3991a3 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp @@ -208,12 +208,14 @@ GDBRemoteClientBase::SendPacketAndWaitForResponse(llvm::StringRef payload, Strin return PacketResult::ErrorSendFailed; } - return SendPacketAndWaitForResponseNoLock(payload, response); + return SendPacketAndWaitForResponse(payload, response, lock); } GDBRemoteCommunication::PacketResult -GDBRemoteClientBase::SendPacketAndWaitForResponseNoLock(llvm::StringRef payload, StringExtractorGDBRemote &response) +GDBRemoteClientBase::SendPacketAndWaitForResponse(llvm::StringRef payload, StringExtractorGDBRemote &response, + const Lock &lock) { + assert(lock); PacketResult packet_result = SendPacketNoLock(payload.data(), payload.size()); if (packet_result != PacketResult::Success) return packet_result; diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h index 684ef3e..a3bcc86 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h @@ -73,7 +73,7 @@ public: Lock(GDBRemoteClientBase &comm, bool interrupt); ~Lock(); - explicit operator bool() { return m_acquired; } + explicit operator bool() const { return m_acquired; } // Whether we had to interrupt the continue thread to acquire the connection. bool @@ -83,7 +83,7 @@ public: } private: - std::unique_lock m_async_lock; + std::unique_lock m_async_lock; GDBRemoteClientBase &m_comm; bool m_acquired; bool m_did_interrupt; @@ -94,7 +94,7 @@ public: protected: PacketResult - SendPacketAndWaitForResponseNoLock(llvm::StringRef payload, StringExtractorGDBRemote &response); + SendPacketAndWaitForResponse(llvm::StringRef payload, StringExtractorGDBRemote &response, const Lock &lock); virtual void OnRunPacketSent(bool first); @@ -126,7 +126,7 @@ private: // This handles the synchronization between individual async threads. For now they just use a // simple mutex. - std::recursive_mutex m_async_mutex; + std::mutex m_async_mutex; bool ShouldStop(const UnixSignals &signals, StringExtractorGDBRemote &response); diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp index 038ba30..5f766a7 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp @@ -516,21 +516,27 @@ GDBRemoteCommunicationClient::GetRemoteQSupported () } } -bool -GDBRemoteCommunicationClient::GetThreadSuffixSupported () +void +GDBRemoteCommunicationClient::ComputeThreadSuffixSupport() { - if (m_supports_thread_suffix == eLazyBoolCalculate) + if (m_supports_thread_suffix != eLazyBoolCalculate) + return; + + StringExtractorGDBRemote response; + m_supports_thread_suffix = eLazyBoolNo; + if (SendPacketAndWaitForResponse("QThreadSuffixSupported", response, false) == PacketResult::Success) { - StringExtractorGDBRemote response; - m_supports_thread_suffix = eLazyBoolNo; - if (SendPacketAndWaitForResponse("QThreadSuffixSupported", response, false) == PacketResult::Success) - { - if (response.IsOKResponse()) - m_supports_thread_suffix = eLazyBoolYes; - } + if (response.IsOKResponse()) + m_supports_thread_suffix = eLazyBoolYes; } - return m_supports_thread_suffix; } + +bool +GDBRemoteCommunicationClient::GetThreadSuffixSupported() +{ + return m_supports_thread_suffix == eLazyBoolYes; +} + bool GDBRemoteCommunicationClient::GetVContSupported (char flavor) { @@ -592,26 +598,17 @@ GDBRemoteCommunicationClient::GetVContSupported (char flavor) GDBRemoteCommunication::PacketResult GDBRemoteCommunicationClient::SendThreadSpecificPacketAndWaitForResponse(lldb::tid_t tid, StreamString &&payload, StringExtractorGDBRemote &response, - bool send_async) + const Lock &lock) { - 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)) + if (!SetCurrentThread(tid, lock)) return PacketResult::ErrorSendFailed; } - return SendPacketAndWaitForResponseNoLock(payload.GetString(), response); + return SendPacketAndWaitForResponse(payload.GetString(), response, lock); } // Check if the target supports 'p' packet. It sends out a 'p' @@ -624,11 +621,20 @@ GDBRemoteCommunicationClient::GetpPacketSupported (lldb::tid_t tid) { if (m_supports_p == eLazyBoolCalculate) { + Lock lock(*this, false); + if (!lock) + { + Log *log(ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_PROCESS | GDBR_LOG_PACKETS)); + if (log) + log->Printf("GDBRemoteCommunicationClient::%s failed to get sequence mutex", __FUNCTION__); + return false; + } + m_supports_p = eLazyBoolNo; StreamString payload; payload.PutCString("p0"); StringExtractorGDBRemote response; - if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, false) == + if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, lock) == PacketResult::Success && response.IsNormalResponse()) { @@ -766,7 +772,7 @@ GDBRemoteCommunicationClient::SendPacketsAndConcatenateResponses // Construct payload char sizeDescriptor[128]; snprintf(sizeDescriptor, sizeof(sizeDescriptor), "%x,%x", offset, response_size); - PacketResult result = SendPacketAndWaitForResponseNoLock(payload_prefix_str + sizeDescriptor, this_response); + PacketResult result = SendPacketAndWaitForResponse(payload_prefix_str + sizeDescriptor, this_response, lock); if (result != PacketResult::Success) return result; @@ -2789,7 +2795,7 @@ GDBRemoteCommunicationClient::KillSpawnedProcess (lldb::pid_t pid) } bool -GDBRemoteCommunicationClient::SetCurrentThread (uint64_t tid) +GDBRemoteCommunicationClient::SetCurrentThread(uint64_t tid, const Lock &lock) { if (m_curr_tid == tid) return true; @@ -2802,7 +2808,7 @@ GDBRemoteCommunicationClient::SetCurrentThread (uint64_t tid) packet_len = ::snprintf (packet, sizeof(packet), "Hg%" PRIx64, tid); assert (packet_len + 1 < (int)sizeof(packet)); StringExtractorGDBRemote response; - if (SendPacketAndWaitForResponse(packet, packet_len, response, false) == PacketResult::Success) + if (SendPacketAndWaitForResponse(llvm::StringRef(packet, packet_len), response, lock) == PacketResult::Success) { if (response.IsOKResponse()) { @@ -2963,9 +2969,9 @@ GDBRemoteCommunicationClient::GetCurrentThreadIDs (std::vector &thr StringExtractorGDBRemote response; PacketResult packet_result; - for (packet_result = SendPacketAndWaitForResponseNoLock("qfThreadInfo", response); + for (packet_result = SendPacketAndWaitForResponse("qfThreadInfo", response, lock); packet_result == PacketResult::Success && response.IsNormalResponse(); - packet_result = SendPacketAndWaitForResponseNoLock("qsThreadInfo", response)) + packet_result = SendPacketAndWaitForResponse("qsThreadInfo", response, lock)) { char ch = response.GetChar(); if (ch == 'l') @@ -3496,12 +3502,12 @@ GDBRemoteCommunicationClient::AvoidGPackets (ProcessGDBRemote *process) } DataBufferSP -GDBRemoteCommunicationClient::ReadRegister(lldb::tid_t tid, uint32_t reg) +GDBRemoteCommunicationClient::ReadRegister(lldb::tid_t tid, uint32_t reg, const Lock &lock) { StreamString payload; payload.Printf("p%x", reg); StringExtractorGDBRemote response; - if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, false) != PacketResult::Success || + if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, lock) != PacketResult::Success || !response.IsNormalResponse()) return nullptr; @@ -3511,12 +3517,12 @@ GDBRemoteCommunicationClient::ReadRegister(lldb::tid_t tid, uint32_t reg) } DataBufferSP -GDBRemoteCommunicationClient::ReadAllRegisters(lldb::tid_t tid) +GDBRemoteCommunicationClient::ReadAllRegisters(lldb::tid_t tid, const Lock &lock) { StreamString payload; payload.PutChar('g'); StringExtractorGDBRemote response; - if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, false) != PacketResult::Success || + if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, lock) != PacketResult::Success || !response.IsNormalResponse()) return nullptr; @@ -3526,25 +3532,26 @@ GDBRemoteCommunicationClient::ReadAllRegisters(lldb::tid_t tid) } bool -GDBRemoteCommunicationClient::WriteRegister(lldb::tid_t tid, uint32_t reg_num, llvm::ArrayRef data) +GDBRemoteCommunicationClient::WriteRegister(lldb::tid_t tid, uint32_t reg_num, llvm::ArrayRef data, + const Lock &lock) { StreamString payload; payload.Printf("P%x=", reg_num); payload.PutBytesAsRawHex8(data.data(), data.size(), endian::InlHostByteOrder(), endian::InlHostByteOrder()); StringExtractorGDBRemote response; - return SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, false) == + return SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, lock) == PacketResult::Success && response.IsOKResponse(); } bool -GDBRemoteCommunicationClient::WriteAllRegisters(lldb::tid_t tid, llvm::ArrayRef data) +GDBRemoteCommunicationClient::WriteAllRegisters(lldb::tid_t tid, llvm::ArrayRef data, const Lock &lock) { StreamString payload; payload.PutChar('G'); payload.PutBytesAsRawHex8(data.data(), data.size(), endian::InlHostByteOrder(), endian::InlHostByteOrder()); StringExtractorGDBRemote response; - return SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, false) == + return SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, lock) == PacketResult::Success && response.IsOKResponse(); } @@ -3555,12 +3562,21 @@ GDBRemoteCommunicationClient::SaveRegisterState (lldb::tid_t tid, uint32_t &save save_id = 0; // Set to invalid save ID if (m_supports_QSaveRegisterState == eLazyBoolNo) return false; - + + Lock lock(*this, false); + if (!lock) + { + Log *log(ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_PROCESS | GDBR_LOG_PACKETS)); + if (log) + log->Printf("GDBRemoteCommunicationClient::%s failed to get sequence mutex", __FUNCTION__); + return false; + } + m_supports_QSaveRegisterState = eLazyBoolYes; StreamString payload; payload.PutCString("QSaveRegisterState"); StringExtractorGDBRemote response; - if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, false) != PacketResult::Success) + if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, lock) != PacketResult::Success) return false; if (response.IsUnsupportedResponse()) @@ -3583,10 +3599,19 @@ GDBRemoteCommunicationClient::RestoreRegisterState (lldb::tid_t tid, uint32_t sa if (m_supports_QSaveRegisterState == eLazyBoolNo) return false; + Lock lock(*this, false); + if (!lock) + { + Log *log(ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_PROCESS | GDBR_LOG_PACKETS)); + if (log) + log->Printf("GDBRemoteCommunicationClient::%s failed to get sequence mutex", __FUNCTION__); + return false; + } + StreamString payload; payload.Printf("QRestoreRegisterState:%u", save_id); StringExtractorGDBRemote response; - if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, false) != PacketResult::Success) + if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, lock) != PacketResult::Success) return false; if (response.IsOKResponse()) @@ -3815,7 +3840,7 @@ GDBRemoteCommunicationClient::ServeSymbolLookups(lldb_private::Process *process) StreamString packet; packet.PutCString ("qSymbol::"); StringExtractorGDBRemote response; - while (SendPacketAndWaitForResponseNoLock(packet.GetString(), response) == PacketResult::Success) + while (SendPacketAndWaitForResponse(packet.GetString(), response, lock) == PacketResult::Success) { if (response.IsOKResponse()) { diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h index eeeecb5..a714b40 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h @@ -64,6 +64,9 @@ public: SendPacketsAndConcatenateResponses (const char *send_payload_prefix, std::string &response_string); + void + ComputeThreadSuffixSupport(); + bool GetThreadSuffixSupported(); @@ -395,9 +398,6 @@ public: uint32_t recv_size); bool - SetCurrentThread (uint64_t tid); - - bool SetCurrentThreadForRun (uint64_t tid); bool @@ -488,17 +488,18 @@ public: lldb::DataBufferSP ReadRegister(lldb::tid_t tid, - uint32_t reg_num); // Must be the eRegisterKindProcessPlugin register number + uint32_t reg_num, // Must be the eRegisterKindProcessPlugin register number + const Lock &lock); lldb::DataBufferSP - ReadAllRegisters(lldb::tid_t tid); + ReadAllRegisters(lldb::tid_t tid, const Lock &lock); bool WriteRegister(lldb::tid_t tid, uint32_t reg_num, // eRegisterKindProcessPlugin register number - llvm::ArrayRef data); + llvm::ArrayRef data, const Lock &lock); bool - WriteAllRegisters(lldb::tid_t tid, llvm::ArrayRef data); + WriteAllRegisters(lldb::tid_t tid, llvm::ArrayRef data, const Lock &lock); bool SaveRegisterState(lldb::tid_t tid, uint32_t &save_id); @@ -686,7 +687,10 @@ protected: PacketResult SendThreadSpecificPacketAndWaitForResponse(lldb::tid_t tid, StreamString &&payload, - StringExtractorGDBRemote &response, bool send_async); + StringExtractorGDBRemote &response, const Lock &lock); + + bool + SetCurrentThread(uint64_t tid, const Lock &lock); private: DISALLOW_COPY_AND_ASSIGN (GDBRemoteCommunicationClient); diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp index 78e1956..0f86bf5 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp @@ -119,8 +119,25 @@ GDBRemoteRegisterContext::GetRegisterSet (size_t reg_set) bool GDBRemoteRegisterContext::ReadRegister (const RegisterInfo *reg_info, RegisterValue &value) { + ExecutionContext exe_ctx(CalculateThread()); + + Process *process = exe_ctx.GetProcessPtr(); + Thread *thread = exe_ctx.GetThreadPtr(); + if (process == NULL || thread == NULL) + return false; + + GDBRemoteCommunicationClient &gdb_comm(((ProcessGDBRemote *)process)->GetGDBRemote()); + + GDBRemoteClientBase::Lock lock(gdb_comm, false); + if (!lock) + { + if (Log *log = ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_THREAD | GDBR_LOG_PACKETS)) + log->Printf("GDBRemoteRegisterContext::%s failed to get packet sequence mutex", __FUNCTION__); + return false; + } + // Read the register - if (ReadRegisterBytes (reg_info, m_reg_data)) + if (ReadRegisterBytes(reg_info, m_reg_data, gdb_comm, lock)) { const bool partial_data_ok = false; Error error (value.SetValueFromData(reg_info, m_reg_data, reg_info->byte_offset, partial_data_ok)); @@ -204,30 +221,23 @@ GDBRemoteRegisterContext::PrivateSetRegisterValue (uint32_t reg, uint64_t new_re // Helper function for GDBRemoteRegisterContext::ReadRegisterBytes(). bool -GDBRemoteRegisterContext::GetPrimordialRegister(const RegisterInfo *reg_info, - GDBRemoteCommunicationClient &gdb_comm) +GDBRemoteRegisterContext::GetPrimordialRegister(const RegisterInfo *reg_info, GDBRemoteCommunicationClient &gdb_comm, + const GDBRemoteCommunicationClient::Lock &lock) { const uint32_t lldb_reg = reg_info->kinds[eRegisterKindLLDB]; const uint32_t remote_reg = reg_info->kinds[eRegisterKindProcessPlugin]; StringExtractorGDBRemote response; - if (DataBufferSP buffer_sp = gdb_comm.ReadRegister(m_thread.GetProtocolID(), remote_reg)) + if (DataBufferSP buffer_sp = gdb_comm.ReadRegister(m_thread.GetProtocolID(), remote_reg, lock)) return PrivateSetRegisterValue(lldb_reg, llvm::ArrayRef(buffer_sp->GetBytes(), buffer_sp->GetByteSize())); return false; } bool -GDBRemoteRegisterContext::ReadRegisterBytes (const RegisterInfo *reg_info, DataExtractor &data) +GDBRemoteRegisterContext::ReadRegisterBytes(const RegisterInfo *reg_info, DataExtractor &data, + GDBRemoteCommunicationClient &gdb_comm, + const GDBRemoteCommunicationClient::Lock &lock) { - ExecutionContext exe_ctx (CalculateThread()); - - Process *process = exe_ctx.GetProcessPtr(); - Thread *thread = exe_ctx.GetThreadPtr(); - if (process == NULL || thread == NULL) - return false; - - GDBRemoteCommunicationClient &gdb_comm (((ProcessGDBRemote *)process)->GetGDBRemote()); - InvalidateIfNeeded(false); const uint32_t reg = reg_info->kinds[eRegisterKindLLDB]; @@ -236,7 +246,7 @@ GDBRemoteRegisterContext::ReadRegisterBytes (const RegisterInfo *reg_info, DataE { if (m_read_all_at_once) { - if (DataBufferSP buffer_sp = gdb_comm.ReadAllRegisters(m_thread.GetProtocolID())) + if (DataBufferSP buffer_sp = gdb_comm.ReadAllRegisters(m_thread.GetProtocolID(), lock)) { memcpy(const_cast(m_reg_data.GetDataStart()), buffer_sp->GetBytes(), std::min(buffer_sp->GetByteSize(), m_reg_data.GetByteSize())); @@ -269,7 +279,7 @@ GDBRemoteRegisterContext::ReadRegisterBytes (const RegisterInfo *reg_info, DataE { // Read the containing register if it hasn't already been read if (!GetRegisterIsValid(prim_reg)) - success = GetPrimordialRegister(prim_reg_info, gdb_comm); + success = GetPrimordialRegister(prim_reg_info, gdb_comm, lock); } } @@ -283,7 +293,7 @@ GDBRemoteRegisterContext::ReadRegisterBytes (const RegisterInfo *reg_info, DataE else { // Get each register individually - GetPrimordialRegister(reg_info, gdb_comm); + GetPrimordialRegister(reg_info, gdb_comm, lock); } // Make sure we got a valid register value after reading it @@ -325,8 +335,8 @@ GDBRemoteRegisterContext::WriteRegister (const RegisterInfo *reg_info, // Helper function for GDBRemoteRegisterContext::WriteRegisterBytes(). bool -GDBRemoteRegisterContext::SetPrimordialRegister(const RegisterInfo *reg_info, - GDBRemoteCommunicationClient &gdb_comm) +GDBRemoteRegisterContext::SetPrimordialRegister(const RegisterInfo *reg_info, GDBRemoteCommunicationClient &gdb_comm, + const GDBRemoteCommunicationClient::Lock &lock) { StreamString packet; StringExtractorGDBRemote response; @@ -336,7 +346,7 @@ GDBRemoteRegisterContext::SetPrimordialRegister(const RegisterInfo *reg_info, return gdb_comm.WriteRegister( m_thread.GetProtocolID(), reg_info->kinds[eRegisterKindProcessPlugin], - {m_reg_data.PeekData(reg_info->byte_offset, reg_info->byte_size), reg_info->byte_size}); + {m_reg_data.PeekData(reg_info->byte_offset, reg_info->byte_size), reg_info->byte_size}, lock); } bool @@ -383,7 +393,7 @@ GDBRemoteRegisterContext::WriteRegisterBytes (const RegisterInfo *reg_info, Data // Set all registers in one packet if (gdb_comm.WriteAllRegisters(m_thread.GetProtocolID(), - {m_reg_data.GetDataStart(), size_t(m_reg_data.GetByteSize())})) + {m_reg_data.GetDataStart(), size_t(m_reg_data.GetByteSize())}, lock)) { SetAllRegisterValid (false); @@ -413,13 +423,13 @@ GDBRemoteRegisterContext::WriteRegisterBytes (const RegisterInfo *reg_info, Data if (value_reg_info == NULL) success = false; else - success = SetPrimordialRegister(value_reg_info, gdb_comm); + success = SetPrimordialRegister(value_reg_info, gdb_comm, lock); } } else { // This is an actual register, write it - success = SetPrimordialRegister(reg_info, gdb_comm); + success = SetPrimordialRegister(reg_info, gdb_comm, lock); } // Check if writing this register will invalidate any other register values? @@ -525,7 +535,7 @@ GDBRemoteRegisterContext::ReadAllRegisterValues (lldb::DataBufferSP &data_sp) if (gdb_comm.SyncThreadState(m_thread.GetProtocolID())) InvalidateAllRegisters(); - if (use_g_packet && (data_sp = gdb_comm.ReadAllRegisters(m_thread.GetProtocolID()))) + if (use_g_packet && (data_sp = gdb_comm.ReadAllRegisters(m_thread.GetProtocolID(), lock))) return true; // We're going to read each register @@ -536,7 +546,7 @@ GDBRemoteRegisterContext::ReadAllRegisterValues (lldb::DataBufferSP &data_sp) { if (reg_info->value_regs) // skip registers that are slices of real registers continue; - ReadRegisterBytes(reg_info, m_reg_data); + ReadRegisterBytes(reg_info, m_reg_data, gdb_comm, lock); // ReadRegisterBytes saves the contents of the register in to the m_reg_data buffer } data_sp.reset(new DataBufferHeap(m_reg_data.GetDataStart(), m_reg_info.GetRegisterDataByteSize())); @@ -587,7 +597,7 @@ GDBRemoteRegisterContext::WriteAllRegisterValues (const lldb::DataBufferSP &data if (use_g_packet) { if (gdb_comm.WriteAllRegisters(m_thread.GetProtocolID(), - {data_sp->GetBytes(), size_t(data_sp->GetByteSize())})) + {data_sp->GetBytes(), size_t(data_sp->GetByteSize())}, lock)) return true; uint32_t num_restored = 0; @@ -674,7 +684,7 @@ GDBRemoteRegisterContext::WriteAllRegisterValues (const lldb::DataBufferSP &data { SetRegisterIsValid(reg, false); if (gdb_comm.WriteRegister(m_thread.GetProtocolID(), reg_info->kinds[eRegisterKindProcessPlugin], - {restore_src, reg_byte_size})) + {restore_src, reg_byte_size}, lock)) ++num_restored; } } @@ -713,7 +723,7 @@ GDBRemoteRegisterContext::WriteAllRegisterValues (const lldb::DataBufferSP &data SetRegisterIsValid(reg_info, false); if (gdb_comm.WriteRegister(m_thread.GetProtocolID(), reg_info->kinds[eRegisterKindProcessPlugin], - {data_sp->GetBytes() + reg_info->byte_offset, reg_info->byte_size})) + {data_sp->GetBytes() + reg_info->byte_offset, reg_info->byte_size}, lock)) ++num_restored; } return num_restored > 0; diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h index c1d8249..96a2172 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h @@ -98,8 +98,8 @@ protected: friend class ThreadGDBRemote; bool - ReadRegisterBytes (const RegisterInfo *reg_info, - DataExtractor &data); + ReadRegisterBytes(const RegisterInfo *reg_info, DataExtractor &data, GDBRemoteCommunicationClient &gdb_comm, + const GDBRemoteCommunicationClient::Lock &lock); bool WriteRegisterBytes (const RegisterInfo *reg_info, @@ -150,11 +150,13 @@ protected: private: // Helper function for ReadRegisterBytes(). - bool GetPrimordialRegister(const RegisterInfo *reg_info, - GDBRemoteCommunicationClient &gdb_comm); + bool + GetPrimordialRegister(const RegisterInfo *reg_info, GDBRemoteCommunicationClient &gdb_comm, + const GDBRemoteCommunicationClient::Lock &lock); // Helper function for WriteRegisterBytes(). - bool SetPrimordialRegister(const RegisterInfo *reg_info, - GDBRemoteCommunicationClient &gdb_comm); + bool + SetPrimordialRegister(const RegisterInfo *reg_info, GDBRemoteCommunicationClient &gdb_comm, + const GDBRemoteCommunicationClient::Lock &lock); DISALLOW_COPY_AND_ASSIGN (GDBRemoteRegisterContext); }; diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp index 67fa0ee..dacc5ba 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -1150,7 +1150,7 @@ ProcessGDBRemote::ConnectToDebugserver (const char *connect_url) GetTarget().SetNonStopModeEnabled (m_gdb_comm.SetNonStopMode(true)); m_gdb_comm.GetEchoSupported (); - m_gdb_comm.GetThreadSuffixSupported (); + m_gdb_comm.ComputeThreadSuffixSupport(); m_gdb_comm.GetListThreadsInStopReplySupported (); m_gdb_comm.GetHostInfo (); m_gdb_comm.GetVContSupported ('c'); diff --git a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp index 0b1f60d..95bd5cd 100644 --- a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp +++ b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp @@ -76,17 +76,22 @@ TEST_F(GDBRemoteCommunicationClientTest, WriteRegister) if (HasFailure()) return; + std::future suffix_result = std::async(std::launch::async, [&] { client.ComputeThreadSuffixSupport(); }); + Handle_QThreadSuffixSupported(server, true); + suffix_result.get(); + + GDBRemoteCommunicationClient::Lock lock(client, false); + ASSERT_TRUE(bool(lock)); + const lldb::tid_t tid = 0x47; const uint32_t reg_num = 4; std::future write_result = - std::async(std::launch::async, [&] { return client.WriteRegister(tid, reg_num, one_register); }); - - Handle_QThreadSuffixSupported(server, true); + std::async(std::launch::async, [&] { return client.WriteRegister(tid, reg_num, one_register, lock); }); HandlePacket(server, "P4=" + one_register_hex + ";thread:0047;", "OK"); ASSERT_TRUE(write_result.get()); - write_result = std::async(std::launch::async, [&] { return client.WriteAllRegisters(tid, all_registers); }); + write_result = std::async(std::launch::async, [&] { return client.WriteAllRegisters(tid, all_registers, lock); }); HandlePacket(server, "G" + all_registers_hex + ";thread:0047;", "OK"); ASSERT_TRUE(write_result.get()); @@ -100,17 +105,23 @@ TEST_F(GDBRemoteCommunicationClientTest, WriteRegisterNoSuffix) if (HasFailure()) return; + std::future suffix_result = std::async(std::launch::async, [&] { client.ComputeThreadSuffixSupport(); }); + Handle_QThreadSuffixSupported(server, false); + suffix_result.get(); + + GDBRemoteCommunicationClient::Lock lock(client, false); + ASSERT_TRUE(bool(lock)); + const lldb::tid_t tid = 0x47; const uint32_t reg_num = 4; std::future write_result = - std::async(std::launch::async, [&] { return client.WriteRegister(tid, reg_num, one_register); }); + std::async(std::launch::async, [&] { return client.WriteRegister(tid, reg_num, one_register, lock); }); - Handle_QThreadSuffixSupported(server, false); HandlePacket(server, "Hg47", "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); }); + write_result = std::async(std::launch::async, [&] { return client.WriteAllRegisters(tid, all_registers, lock); }); HandlePacket(server, "G" + all_registers_hex, "OK"); ASSERT_TRUE(write_result.get()); @@ -124,21 +135,27 @@ TEST_F(GDBRemoteCommunicationClientTest, ReadRegister) if (HasFailure()) return; + std::future suffix_result = std::async(std::launch::async, [&] { client.ComputeThreadSuffixSupport(); }); + Handle_QThreadSuffixSupported(server, true); + suffix_result.get(); + 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;", one_register_hex); ASSERT_TRUE(async_result.get()); + GDBRemoteCommunicationClient::Lock lock(client, false); + ASSERT_TRUE(bool(lock)); + std::future read_result = - std::async(std::launch::async, [&] { return client.ReadRegister(tid, reg_num); }); + std::async(std::launch::async, [&] { return client.ReadRegister(tid, reg_num, lock); }); HandlePacket(server, "p4;thread:0047;", "41424344"); auto buffer_sp = read_result.get(); ASSERT_TRUE(bool(buffer_sp)); ASSERT_EQ(0, memcmp(buffer_sp->GetBytes(), one_register, sizeof one_register)); - read_result = std::async(std::launch::async, [&] { return client.ReadAllRegisters(tid); }); + read_result = std::async(std::launch::async, [&] { return client.ReadAllRegisters(tid, lock); }); HandlePacket(server, "g;thread:0047;", all_registers_hex); buffer_sp = read_result.get(); ASSERT_TRUE(bool(buffer_sp)); @@ -153,11 +170,14 @@ TEST_F(GDBRemoteCommunicationClientTest, SaveRestoreRegistersNoSuffix) if (HasFailure()) return; + std::future suffix_result = std::async(std::launch::async, [&] { client.ComputeThreadSuffixSupport(); }); + Handle_QThreadSuffixSupported(server, false); + suffix_result.get(); + 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()); -- 2.7.4