labath created this revision.
labath added a reviewer: clayborg.
labath added a subscriber: lldb-commits.
Herald added subscribers: danalbert, tberghammer.

This change adds the ability to the client to send multiple packets without 
waiting for the
response to the first one. The individual senders use a queue to register 
themselves when they
have sent a packet. When they reach the front of the queue it's their turn to 
read a packet.

This does not change the protocol in any way -- a well-behaved stub which just 
sits in a
ReadPacketAndSendResponse() loop will not notice any difference. It does 
however make a huge
difference on connections with a non-negligible latency (anything over 10ms 
certainly fits the
bill), because we will have multiple packets in flight at the same time. The 
queueing system
avoids the need for a separate thread doing the reading/writing and should add 
no overhead to the
common case of sequential packet streams.

No code except the unit test actually uses the new functionality yet, as the 
default is for all
senders to acquire an exclusive connection lock. Any packets using this will 
need to be enabled
on a one-off basis, making sure that there can be no bad interactions between 
other packets which
can get sent concurrently. Some obvious cases where this can *not* be enabled 
are:
- AckMode: when using unreliable connections, we can never be sure that the 
other side has
  recieved our packet and we may need to do retransmissions.
- packets which need to be sent as an atomic sequence (e.g. if we don't have 
thread suffix
  enabled, then we need to send Hg plus another packet together).

The first packet which I would try to enable is qModuleInfo, which is a 
read-only packet with no
side-effects, and we need to send a lot of them (over 150 when attaching to a 
typical Android
application), so it can give a big speed boost there. However, I can see this 
being useful in
other places as well (parallel thread backtrace computation?).

To make the code thread-sanitizer clean, I've needed to add a lock to the 
packet history class,
as now we can be sending and receiving a packet simultaneously.

---------- WIP ----------
This code does not actually work yet (apart from the unittests), because the 
read/write lock does
not like to be locked recursively. I'll need to weed those out first (which 
should be pretty
orthogonal to the changes here).
---------- WIP ----------

https://reviews.llvm.org/D22914

Files:
  source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp

Index: unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
===================================================================
--- unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
+++ unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
@@ -229,6 +229,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;
@@ -243,10 +244,11 @@
                                                         event_sp);
 
     // 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<PacketResult> 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.
@@ -307,6 +309,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;
@@ -338,7 +341,7 @@
 
     // Packet stream should remain synchronized.
     std::future<PacketResult> 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());
@@ -396,3 +399,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<PacketResult> async_result1 = std::async(std::launch::async, [&] {
+        return fix.client.SendPacketAndWaitForResponse("qTest1", async_response1, send_kind);
+    });
+
+    std::future<PacketResult> 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<std::thread> 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();
+}
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)
@@ -1143,7 +1144,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;
 }
@@ -1630,7 +1631,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())
@@ -4996,7 +4997,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())
@@ -5371,7 +5373,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);
@@ -5430,7 +5433,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: source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
===================================================================
--- source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
@@ -395,7 +395,7 @@
                                   reg_info->byte_size,          // dst length
                                   m_reg_data.GetByteOrder()))   // dst byte order
     {
-        GDBRemoteClientBase::Lock lock(gdb_comm, false);
+        GDBRemoteClientBase::Lock lock(gdb_comm);
         if (lock)
         {
             const bool thread_suffix_supported = gdb_comm.GetThreadSuffixSupported();
@@ -564,7 +564,7 @@
 
     const bool use_g_packet = gdb_comm.AvoidGPackets ((ProcessGDBRemote *)process) == false;
 
-    GDBRemoteClientBase::Lock lock(gdb_comm, false);
+    GDBRemoteClientBase::Lock lock(gdb_comm);
     if (lock)
     {
         SyncThreadState(process);
@@ -673,7 +673,7 @@
     const bool use_g_packet = gdb_comm.AvoidGPackets ((ProcessGDBRemote *)process) == false;
 
     StringExtractorGDBRemote response;
-    GDBRemoteClientBase::Lock lock(gdb_comm, false);
+    GDBRemoteClientBase::Lock lock(gdb_comm);
     if (lock)
     {
         const bool thread_suffix_supported = gdb_comm.GetThreadSuffixSupported();
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));
@@ -1098,7 +1096,7 @@
         m_qGDBServerVersion_is_valid = eLazyBoolNo;
 
         StringExtractorGDBRemote response;
-        if (SendPacketAndWaitForResponse ("qGDBServerVersion", response, false) == PacketResult::Success)
+        if (SendPacketAndWaitForResponse("qGDBServerVersion", response) == PacketResult::Success)
         {
             if (response.IsNormalResponse())
             {
@@ -1222,7 +1220,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())
@@ -1255,7 +1253,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())
@@ -1276,7 +1274,7 @@
     {
         m_qHostInfo_is_valid = eLazyBoolNo;
         StringExtractorGDBRemote response;
-        if (SendPacketAndWaitForResponse ("qHostInfo", response, false) == PacketResult::Success)
+        if (SendPacketAndWaitForResponse("qHostInfo", response) == PacketResult::Success)
         {
             if (response.IsNormalResponse())
             {
@@ -1929,7 +1927,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;
@@ -2135,7 +2133,7 @@
     GetHostInfo ();
 
     StringExtractorGDBRemote response;
-    if (SendPacketAndWaitForResponse ("qProcessInfo", response, false) == PacketResult::Success)
+    if (SendPacketAndWaitForResponse("qProcessInfo", response) == PacketResult::Success)
     {
         if (response.IsNormalResponse())
         {
@@ -2704,7 +2702,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());
@@ -2920,7 +2918,7 @@
 {
     thread_ids.clear();
 
-    Lock lock(*this, false);
+    Lock lock(*this);
     if (lock)
     {
         sequence_mutex_unavailable = false;
@@ -2977,7 +2975,7 @@
 lldb::addr_t
 GDBRemoteCommunicationClient::GetShlibInfoAddr()
 {
-    Lock lock(*this, false);
+    Lock lock(*this);
     if (lock)
     {
         StringExtractorGDBRemote response;
@@ -3474,7 +3472,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();
@@ -3488,7 +3486,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))
@@ -3503,7 +3501,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();
@@ -3518,7 +3516,7 @@
             else
                 packet_len = ::snprintf (packet, sizeof(packet), "g");
             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))
@@ -3535,7 +3533,7 @@
         return false;
     
     m_supports_QSaveRegisterState = eLazyBoolYes;
-    Lock lock(*this, false);
+    Lock lock(*this);
     if (lock)
     {
         const bool thread_suffix_supported = GetThreadSuffixSupported();
@@ -3549,7 +3547,7 @@
             
             StringExtractorGDBRemote response;
 
-            if (SendPacketAndWaitForResponse(packet, response, false) == PacketResult::Success)
+            if (SendPacketAndWaitForResponse(packet, response) == PacketResult::Success)
             {
                 if (response.IsUnsupportedResponse())
                 {
@@ -3583,7 +3581,7 @@
     if (m_supports_QSaveRegisterState == eLazyBoolNo)
         return false;
 
-    Lock lock(*this, false);
+    Lock lock(*this);
     if (lock)
     {
         const bool thread_suffix_supported = GetThreadSuffixSupported();
@@ -3596,8 +3594,8 @@
                 ::snprintf (packet, sizeof(packet), "QRestoreRegisterState:%u" PRIx64 ";", save_id);
             
             StringExtractorGDBRemote response;
-            
-            if (SendPacketAndWaitForResponse(packet, response, false) == PacketResult::Success)
+
+            if (SendPacketAndWaitForResponse(packet, response) == PacketResult::Success)
             {
                 if (response.IsOKResponse())
                 {
@@ -3732,10 +3730,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" );
@@ -3819,7 +3814,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/GDBRemoteCommunication.h
===================================================================
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
@@ -208,27 +208,19 @@
         
         ~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
         Dump (Log *log) const;
 
         bool
         DidDumpToLog () const
         {
+            std::lock_guard<std::mutex> lock(m_mutex);
             return m_dumped_to_log;
         }
     
@@ -266,6 +258,7 @@
             return i % m_packets.size();
         }
 
+        mutable std::mutex m_mutex;
         std::vector<Entry> 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
@@ -69,33 +69,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<std::mutex> 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;
@@ -106,6 +87,7 @@
 void
 GDBRemoteCommunication::History::Dump (Stream &strm) const
 {
+    std::lock_guard<std::mutex> lock(m_mutex);
     const uint32_t size = GetNumPacketsInHistory ();
     const uint32_t first_idx = GetFirstSavedPacketIndex ();
     const uint32_t stop_idx = m_curr_idx + size;
@@ -127,6 +109,7 @@
 void
 GDBRemoteCommunication::History::Dump (Log *log) const
 {
+    std::lock_guard<std::mutex> 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)
         {
@@ -953,7 +935,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/GDBRemoteClientBase.h
===================================================================
--- source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h
+++ source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h
@@ -14,6 +14,8 @@
 
 #include <condition_variable>
 
+#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,15 +57,40 @@
         }
 
     private:
-        std::unique_lock<std::recursive_mutex> m_async_lock;
         GDBRemoteClientBase &m_comm;
+        Kind m_kind;
         bool m_acquired;
         bool m_did_interrupt;
 
         void
         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:
     void
     HandleAsyncStdout(ContinueDelegate &delegate, StringExtractorGDBRemote &payload);
@@ -117,13 +126,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<const void *> 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
@@ -132,7 +132,7 @@
 bool
 GDBRemoteClientBase::SendAsyncSignal(int signo)
 {
-    Lock lock(*this, true);
+    Lock lock(*this, Lock::Exclusive, true);
     if (!lock || !lock.DidInterrupt())
         return false;
 
@@ -145,17 +145,17 @@
 bool
 GDBRemoteClientBase::Interrupt()
 {
-    Lock lock(*this, true);
+    Lock lock(*this, Lock::Exclusive, true);
     if (!lock.DidInterrupt())
         return false;
     m_should_stop = true;
     return true;
 }
 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))
@@ -170,25 +170,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<std::mutex> 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<std::mutex> 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");
     }
@@ -203,7 +227,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()),
@@ -329,12 +353,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
@@ -375,6 +404,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<std::mutex> lock(m_comm.m_mutex);
         --m_comm.m_async_count;
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to