gdb-remote: Make the sequence mutex non-recursive

[labath]

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:

• Read/WriteRegister expect the caller to obtain the mutex and are renamed to reflect that. The only caller of these functions is the GDBRemoteRegisterContext class, which already acquired that mutex (for the most part), so the changes there are minimal. (It is not clear to me whether this function actually needs to take the lock. The thing which this lock seems to protect is the state of the register context if it is being accessed from multiple threads, which, if true, sounds like it could be done by a separate mutex.)
• 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.

[clayborg]

The old mutex was there so that you could send a packet and get a result without worrying about other threads getting your response. It was recursive for the case where we needed to send two packets as one (set thread ID, then send packet), and for reading all registers (as you have found), so your patch does work, but it now only for these special cases. Now there is no way to easily take the mutex and send 5 different packets without going and adding new NoLock variants of each call that you might want to send as a single stream of packets. Can we think slightly cleaner way to do this? Before you could stake the sequence mutex in your main function that wanted to make N calls, and each call would then recursively lock the mutex and everything just worked. Now you will deadlock if you do things incorrectly (granted you will need to do a "GDBRemoteClientBase::Lock lock(gdb_comm, false);" followed by a "m_gdb_comm.SendSomePacket()".

[labath]

In D23802#523244, @clayborg wrote:

The old mutex was there so that you could send a packet and get a result without worrying about other threads getting your response. It was recursive for the case where we needed to send two packets as one (set thread ID, then send packet), and for reading all registers (as you have found), so your patch does work, but it now only for these special cases. Now there is no way to easily take the mutex and send 5 different packets without going and adding new NoLock variants of each call that you might want to send as a single stream of packets. Can we think slightly cleaner way to do this? Before you could stake the sequence mutex in your main function that wanted to make N calls, and each call would then recursively lock the mutex and everything just worked. Now you will deadlock if you do things incorrectly (granted you will need to do a "GDBRemoteClientBase::Lock lock(gdb_comm, false);" followed by a "m_gdb_comm.SendSomePacket()".

I see what you mean. I have considered a couple of other options, but none of them struck me as obviously best. Alternatives I see are:

• have all functions (or just those that we need right now) take an optional lock boolean parameter (probably defaulting to true). If it is set the function takes a lock, if not, it doesn't. The main downside I see here is that then the locking behaviour would depend on the run-time parameter, which is not completely ideal. OTOH, it would sort of sit along nicely with the "send_async" parameter, which also controls behavior in a similar way.
• just make all functions NoLock and require the user to lock manually. The tricky part here is that unlike a normal lock, locking the packet lock can fail, so you end up with code like Lock lock; if(!lock) {if(log) log->Printf("getting registers failed");} else { ... }, which doesn't look nice but we could make it better. If we move the logging part into the Lock object, then this would end up with: Lock lock(gdb_comm, false, "getting registers"); if (!lock) return; ... which I don't think is too bad.
• like before, but only do it for functions which are used from locked and unlocked contexts. Then the policy would be that if you need a NoLock version of a function, instead of creating a new one, you rename the current one and change all callers (I don't expect that there will be so many of them) to lock explicitly. This does not require any major rewrite, but it is a bit messy because you have two kinds of functions. (This is not really an alternative, just an extension of the current approach.)
• write our own implementation of a recursive read/write lock. This may not be so difficult even (although I can see how some edge cases might be tricky -- e.g. we have a read lock (so we can share the connection with other threads), but then we need to send a packet which wants exclusive access), and it would nicely compartmentalize all the ugliness. However, I am kinda against inventing locking primitives (even though the Lock class is such a thing, but there I think it was justified).

What do you think?

[clayborg]

Maybe instead of "NoLock" on the functions we those functions take an extra argument? Then someone can't accidentally run one of those without locking. We need to somehow enforce that locking must occur. Maybe passing an extra:

const GDBRemoteClientBase::Lock &lock

Making it const can stop people from locking it in the NoLock functions, and we can maybe assert:

bool
GDBRemoteCommunicationClient::SetCurrentThread(uint64_t tid, const GDBRemoteClientBase::Lock &lock)
{
    assert(lock.IsAcquired());

Then IsAcquired() can be const, but this at least would enforce that someone needs to lock before sending the packet. We probably need to document these no locking commands and let users know they must lock and pass in the lock for verification.

[labath]

Sounds good. I like that this enables us to get rid of the NoLock suffix.

Maybe eventually we could event replace the runtime assert with some sort of a static lock
discipline checking framework, where you annotate functions with what locks it expects (I have no
experience with it, but I understand clang supports something like that).

The IsAcquired function is already present in the form of the explicit operator bool (I just needed to make it const).

[clayborg]

Looks good.

[nitesh.jain]

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp

131

Hi labath,

This patch cause deadlock when we try to run "reg read f0". The GDBRemoteRegisterContext::ReadRegister is call twice which result in deadlock since earlier acquire lock has not been release.

1. To Evaluate Dwarf expression so that floating register size can be determine at run time. It add overhead of reading one more register which cause lock to acquire and forgot to release at the end.

2. To read register f0. This time it try to acquire lock which is not been release resulting in deadlock.

-NJ

[nitesh.jain]

Forgot to mention that this case has been observed for MIPS architecture. Since for MIPS, the floating point register size is calculated at runtime.

-NJ

[labath]

Thanks for the heads up. I'll need to give this some thought. I've pulled the change out until I can figure out what to do.

BTW, have you guys considered putting up a mips buildbot to detect these kinds of things?

[nitesh.jain]

In D23802#528804, @labath wrote:

Thanks for the heads up. I'll need to give this some thought. I've pulled the change out until I can figure out what to do.

BTW, have you guys considered putting up a mips buildbot to detect these kinds of things?

Thanks. Right now we are trying to fix big-endain failures and then will setup buildbot for both.

[ted]

I'm seeing the same issue as Nitesh, except in a different spot. The Hexagon Simulator doesn't support QSaveRegisterState, so lldb calls GDBRemoteRegisterContext::ReadAllRegisterValues. It gets a lock on the communication client, then calls ReadAllRegisters, which tries to get a lock on the communication client, and fails (hangs on Linux, crashes on Windows).

The revert fixed the problem.

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h

public:
     SendvContPacket(llvm::StringRef payload, StringExtractorGDBRemote &response);
 
     class Lock
     {
     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
         DidInterrupt() const
         {
             return m_did_interrupt;
         }
 
     private:
-        std::unique_lock<std::recursive_mutex> m_async_lock;
+        std::unique_lock<std::mutex> m_async_lock;
         GDBRemoteClientBase &m_comm;
         bool m_acquired;
         bool m_did_interrupt;
 
         void
         SyncWithContinueThread(bool interrupt);
     };
 
 protected:
     PacketResult
-    SendPacketAndWaitForResponseNoLock(llvm::StringRef payload, StringExtractorGDBRemote &response);
+    SendPacketAndWaitForResponse(llvm::StringRef payload, StringExtractorGDBRemote &response, const Lock &lock);
 
     virtual void
     OnRunPacketSent(bool first);
 
 private:
     // Variables handling synchronization between the Continue thread and any other threads
     // wishing to send packets over the connection. Either the continue thread has control over
     // the connection (m_is_running == true) or the connection is free for an arbitrary number of
private:
     std::chrono::time_point<std::chrono::steady_clock> m_interrupt_time;
     uint32_t m_async_count;
     bool m_is_running;
     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;
+    std::mutex m_async_mutex;
 
     bool
     ShouldStop(const UnixSignals &signals, StringExtractorGDBRemote &response);
 
     class ContinueLock
     {
     public:
         enum class LockResult

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp

GDBRemoteClientBase::SendPacketAndWaitForResponse(llvm::StringRef payload, StringExtractorGDBRemote &response,
     if (!lock)
     {
         if (Log *log = ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS))
             log->Printf("GDBRemoteClientBase::%s failed to get mutex, not sending packet '%.*s' (send_async=%d)",
                         __FUNCTION__, int(payload.size()), payload.data(), send_async);
         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;
 
     const size_t max_response_retries = 3;
     for (size_t i = 0; i < max_response_retries; ++i)
     {
         packet_result = ReadPacket(response, GetPacketTimeoutInMicroSeconds(), true);

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

public:
     // and continues for 0x1000 bytes").
     // Concatenate the resulting server response packets together and
     // return in response_string.  If any packet fails, the return value
     // indicates that failure and the returned string value is undefined.
     PacketResult
     SendPacketsAndConcatenateResponses (const char *send_payload_prefix,
                                         std::string &response_string);
 
+    void
+    ComputeThreadSuffixSupport();
+
     bool
     GetThreadSuffixSupported();
 
     // This packet is usually sent first and the boolean return value
     // indicates if the packet was send and any response was received
     // even in the response is UNIMPLEMENTED. If the packet failed to
     // get a response, then false is returned. This quickly tells us
     // if we were able to connect and communicate with the remote GDB
public:
     // measure the packet speed without any other work being done on the
     // other end and avoids any of that work affecting the packet send
     // and response times.
     bool
     SendSpeedTestPacket (uint32_t send_size, 
                          uint32_t recv_size);
     
     bool
-    SetCurrentThread (uint64_t tid);
-    
-    bool
     SetCurrentThreadForRun (uint64_t tid);
 
     bool
     GetQXferAuxvReadSupported ();
 
     bool
     GetQXferLibrariesReadSupported ();
 
RunShellCommand(const char *command,           // Shouldn't be nullptr
                     std::string *command_output,   // Pass nullptr if you don't want the command output
                     uint32_t timeout_sec);         // Timeout in seconds to wait for shell program to finish
 
     bool
     CalculateMD5 (const FileSpec& file_spec, uint64_t &high, uint64_t &low);
 
     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<uint8_t> data);
+                  llvm::ArrayRef<uint8_t> data, const Lock &lock);
 
     bool
-    WriteAllRegisters(lldb::tid_t tid, llvm::ArrayRef<uint8_t> data);
+    WriteAllRegisters(lldb::tid_t tid, llvm::ArrayRef<uint8_t> data, const Lock &lock);
 
     bool
     SaveRegisterState(lldb::tid_t tid, uint32_t &save_id);
 
     bool
     RestoreRegisterState (lldb::tid_t tid, uint32_t save_id);
 
     bool
protected:
     DecodeProcessInfoResponse (StringExtractorGDBRemote &response, 
                                ProcessInstanceInfo &process_info);
 
     void
     OnRunPacketSent(bool first) override;
 
     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);
 };
 
 } // namespace process_gdb_remote
 } // namespace lldb_private
 
 #endif // liblldb_GDBRemoteCommunicationClient_h_

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

if (SendPacketAndWaitForResponse(packet.GetData(),
                 Log *log (ProcessGDBRemoteLog::GetLogIfAllCategoriesSet (GDBR_LOG_PROCESS));
                 if (log)
                     log->Printf ("Garbled PacketSize spec in qSupported response");
             }
         }
     }
 }
 
-bool
-GDBRemoteCommunicationClient::GetThreadSuffixSupported ()
-{
-    if (m_supports_thread_suffix == eLazyBoolCalculate)
-    {
-        StringExtractorGDBRemote response;
-        m_supports_thread_suffix = eLazyBoolNo;
-        if (SendPacketAndWaitForResponse("QThreadSuffixSupported", response, false) == PacketResult::Success)
-        {
-            if (response.IsOKResponse())
-                m_supports_thread_suffix = eLazyBoolYes;
-        }
-    }
-    return m_supports_thread_suffix;
+void
+GDBRemoteCommunicationClient::ComputeThreadSuffixSupport()
+    if (m_supports_thread_suffix != eLazyBoolCalculate)
+        return;
+
+
+bool
+GDBRemoteCommunicationClient::GetThreadSuffixSupported()
+{
+    return m_supports_thread_suffix == eLazyBoolYes;
 }
+
 bool
 GDBRemoteCommunicationClient::GetVContSupported (char flavor)
 {
     if (m_supports_vCont_c == eLazyBoolCalculate)
     {
         StringExtractorGDBRemote response;
         m_supports_vCont_any = eLazyBoolNo;
         m_supports_vCont_all = eLazyBoolNo;
GDBRemoteCommunicationClient::GetVContSupported (char flavor)
     default: break;
     }
     return false;
 }
 
 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'
 // 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)
     {
+        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())
         {
             m_supports_p = eLazyBoolYes;
         }
     }
     return m_supports_p;
 }
)
     }
 
     for (unsigned int offset = 0; true; offset += response_size)
     {
         StringExtractorGDBRemote this_response;
         // 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;
 
         const std::string &this_string = this_response.GetStringRef();
 
         // Check for m or l as first character; l seems to mean this is the last chunk
         char first_char = *this_string.c_str();
         if (first_char != 'm' && first_char != 'l')
GDBRemoteCommunicationClient::KillSpawnedProcess (lldb::pid_t pid)
     {
         if (response.IsOKResponse())
             return true;
     }
     return false;
 }
 
 bool
-GDBRemoteCommunicationClient::SetCurrentThread (uint64_t tid)
+GDBRemoteCommunicationClient::SetCurrentThread(uint64_t tid, const Lock &lock)
 {
     if (m_curr_tid == tid)
         return true;
 
     char packet[32];
     int packet_len;
     if (tid == UINT64_MAX)
         packet_len = ::snprintf (packet, sizeof(packet), "Hg-1");
     else
         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())
         {
             m_curr_tid = tid;
             return true;
         }
 
         /*
GDBRemoteCommunicationClient::GetCurrentThreadIDs (std::vector<lldb::tid_t> &thread_ids,
 
     Lock lock(*this, false);
     if (lock)
     {
         sequence_mutex_unavailable = false;
         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')
                 break;
             if (ch == 'm')
             {
                 do
                 {
if (m_avoid_g_packets == eLazyBoolCalculate)
                 }
             }
         }
     }
     return m_avoid_g_packets == eLazyBoolYes;
 }
 
 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;
 
     DataBufferSP buffer_sp(new DataBufferHeap(response.GetStringRef().size() / 2, 0));
     response.GetHexBytes(buffer_sp->GetBytes(), buffer_sp->GetByteSize(), '\xcc');
     return buffer_sp;
 }
 
 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;
 
     DataBufferSP buffer_sp(new DataBufferHeap(response.GetStringRef().size() / 2, 0));
     response.GetHexBytes(buffer_sp->GetBytes(), buffer_sp->GetByteSize(), '\xcc');
     return buffer_sp;
 }
 
 bool
-GDBRemoteCommunicationClient::WriteRegister(lldb::tid_t tid, uint32_t reg_num, llvm::ArrayRef<uint8_t> data)
+GDBRemoteCommunicationClient::WriteRegister(lldb::tid_t tid, uint32_t reg_num, llvm::ArrayRef<uint8_t> 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<uint8_t> data)
+GDBRemoteCommunicationClient::WriteAllRegisters(lldb::tid_t tid, llvm::ArrayRef<uint8_t> 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();
 }
 
 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;
-    
+    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())
         m_supports_QSaveRegisterState = eLazyBoolNo;
 
     const uint32_t response_save_id = response.GetU32(0);
     if (response_save_id == 0)
         return false;
 
     save_id = response_save_id;
     return true;
 }
 
 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)
+    {
+        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())
         return true;
 
     if (response.IsUnsupportedResponse())
         m_supports_QSaveRegisterState = eLazyBoolNo;
     return false;
GDBRemoteCommunicationClient::ServeSymbolLookups(lldb_private::Process *process)
     if (m_supports_qSymbol && m_qSymbol_requests_done == false)
     {
         Lock lock(*this, false);
         if (lock)
         {
             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())
                 {
                     if (symbol_response_provided || first_qsymbol_query)
                     {
                         m_qSymbol_requests_done = true;
                     }
 

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h

public:
 
     uint32_t
     ConvertRegisterKindToRegisterNumber (lldb::RegisterKind kind, uint32_t num) override;
 
 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,
                         DataExtractor &data, 
                         uint32_t data_offset);
 
     bool
     PrivateSetRegisterValue(uint32_t reg, llvm::ArrayRef<uint8_t> data);
#endif
 
     GDBRemoteDynamicRegisterInfo &m_reg_info;
     std::vector<bool> m_reg_valid;
     DataExtractor m_reg_data;
     bool m_read_all_at_once;
 
 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);
 };
 
 } // namespace process_gdb_remote
 } // namespace lldb_private
 
 #endif // lldb_GDBRemoteRegisterContext_h_

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp

GDBRemoteRegisterContext::GetRegisterSet (size_t reg_set)
     return m_reg_info.GetRegisterSet (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));
         return error.Success();
     }
     return false;
 }
 
if (data.CopyByteOrderedData (0,                            // src offset
         SetRegisterIsValid (reg, true);
         return true;
     }
     return false;
 }
 
 // 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<uint8_t>(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];
 
     if (!GetRegisterIsValid(reg))
     {
         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<uint8_t *>(m_reg_data.GetDataStart()), buffer_sp->GetBytes(),
                        std::min(buffer_sp->GetByteSize(), m_reg_data.GetByteSize()));
                 if (buffer_sp->GetByteSize() >= m_reg_data.GetByteSize())
                 {
                     SetAllRegisterValid(true);
                     return true;
                 }
if (!GetRegisterIsValid(reg))
                 // Grab the corresponding register info.
                 const RegisterInfo *prim_reg_info = GetRegisterInfoAtIndex(prim_reg);
                 if (prim_reg_info == NULL)
                     success = false;
                 else
                 {
                     // 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);
                 }
             }
 
             if (success)
             {
                 // If we reach this point, all primordial register requests have succeeded.
                 // Validate this composite register.
                 SetRegisterIsValid (reg_info, true);
             }
         }
         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
         if (!GetRegisterIsValid(reg))
             return false;
     }
 
     if (&data != &m_reg_data)
GDBRemoteRegisterContext::WriteRegister (const RegisterInfo *reg_info,
     DataExtractor data;
     if (value.GetData (data))
         return WriteRegisterBytes (reg_info, data, 0);
     return false;
 }
 
 // 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;
     const uint32_t reg = reg_info->kinds[eRegisterKindLLDB];
     // Invalidate just this register
     SetRegisterIsValid(reg, false);
 
     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
 GDBRemoteRegisterContext::WriteRegisterBytes (const RegisterInfo *reg_info, DataExtractor &data, uint32_t data_offset)
 {
     ExecutionContext exe_ctx (CalculateThread());
 
     Process *process = exe_ctx.GetProcessPtr();
if (data.CopyByteOrderedData (data_offset,                  // src offset
         {
                 if (m_read_all_at_once)
                 {
                     // Invalidate all register values
                     InvalidateIfNeeded (true);
 
                     // 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);
                         return true;
                     }
                 }
                 else
                 {
if (data.CopyByteOrderedData (data_offset,                  // src offset
                             if (reg == LLDB_INVALID_REGNUM)
                                 break;
                             // We have a valid primordial register as our constituent.
                             // Grab the corresponding register info.
                             const RegisterInfo *value_reg_info = GetRegisterInfoAtIndex(reg);
                             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?
                     // If so, invalidate them
                     if (reg_info->invalidate_regs)
                     {
                         for (uint32_t idx = 0, reg = reg_info->invalidate_regs[0];
                              reg != LLDB_INVALID_REGNUM;
GDBRemoteRegisterContext::ReadAllRegisterValues (lldb::DataBufferSP &data_sp)
     const bool use_g_packet = gdb_comm.AvoidGPackets ((ProcessGDBRemote *)process) == false;
 
     GDBRemoteClientBase::Lock lock(gdb_comm, false);
     if (lock)
     {
         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
         // individually and store them as binary data in a buffer.
         const RegisterInfo *reg_info;
 
         for (uint32_t i = 0; (reg_info = GetRegisterInfoAtIndex(i)) != NULL; i++)
         {
             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()));
         return true;
     }
     else
     {
 
GDBRemoteRegisterContext::WriteAllRegisterValues (const lldb::DataBufferSP &data_sp)
 
     GDBRemoteClientBase::Lock lock(gdb_comm, false);
     if (lock)
     {
         // The data_sp contains the G response packet.
         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;
             // We need to manually go through all of the registers and
             // restore them manually
             DataExtractor restore_data(data_sp, m_reg_data.GetByteOrder(), m_reg_data.GetAddressByteSize());
 
             const RegisterInfo *reg_info;
if (lock)
 
                 const uint32_t reg_byte_size = reg_info->byte_size;
 
                 const uint8_t *restore_src = restore_data.PeekData(register_offset, reg_byte_size);
                 if (restore_src)
                 {
                     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;
                 }
             }
             return num_restored > 0;
         }
         else
         {
             // For the use_g_packet == false case, we're going to write each register
if (lock)
                 // register context corruption when writing fpsr/fpcr.
                 if (arm64_debugserver && (strcmp(reg_info->name, "fpsr") == 0 || strcmp(reg_info->name, "fpcr") == 0))
                 {
                     continue;
                 }
 
                 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;
         }
     }
     else
     {
         Log *log (ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet (GDBR_LOG_THREAD | GDBR_LOG_PACKETS));

lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

if (!m_gdb_comm.HandshakeWithServer (&error))
         return error;
     }
 
     // Send $QNonStop:1 packet on startup if required
     if (GetTarget().GetNonStopModeEnabled())
         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');
     m_gdb_comm.GetVAttachOrWaitSupported();
 
     // Ask the remote server for the default thread id
     if (GetTarget().GetNonStopModeEnabled())
         m_gdb_comm.GetDefaultThreadId(m_initial_tid);

lldb/trunk/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp

 TEST_F(GDBRemoteCommunicationClientTest, WriteRegister)
 {
     TestClient client;
     MockServer server;
     Connect(client, server);
     if (HasFailure())
         return;
 
+    std::future<void> 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<bool> 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());
 }
 
 TEST_F(GDBRemoteCommunicationClientTest, WriteRegisterNoSuffix)
 {
     TestClient client;
     MockServer server;
     Connect(client, server);
     if (HasFailure())
         return;
 
+    std::future<void> 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<bool> 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());
 }
 
 TEST_F(GDBRemoteCommunicationClientTest, ReadRegister)
 {
     TestClient client;
     MockServer server;
     Connect(client, server);
     if (HasFailure())
         return;
 
+    std::future<void> 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<bool> 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<DataBufferSP> 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));
     ASSERT_EQ(0, memcmp(buffer_sp->GetBytes(), all_registers, sizeof all_registers));
 }
 
 TEST_F(GDBRemoteCommunicationClientTest, SaveRestoreRegistersNoSuffix)
 {
     TestClient client;
     MockServer server;
     Connect(client, server);
     if (HasFailure())
         return;
 
+    std::future<void> 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<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());