Rewrite gdb-remote's SendContinuePacketAndWaitForResponse

SendContinuePacketAndWaitForResponse was huge function with very complex interactions with
several other functions (SendAsyncSignal, SendInterrupt, SendPacket). This meant that making any
changes to how packet sending functions and threads interact was very difficult and error-prone.

This change does not add any functionality yet, it merely paves the way for future changes. In a
follow-up, I plan to add the ability to have multiple query packets in flight (i.e.,
request,request,response,response instead of the usual request,response sequences) and use that
to speed up qModuleInfo packet processing.

Here, I introduce two special kinds of locks: ContinueLock, which is used by the continue thread,
and Lock, which is used by everyone else. ContinueLock (atomically) sends a continue packet, and
blocks any other async threads from accessing the connection. Other threads create an instance of
the Lock object when they want to access the connection. This object, while in scope prevents the
continue from being send. Optionally, it can also interrupt the process to gain access to the
connection for async processing.

Most of the syncrhonization logic is encapsulated within these two classes. Some of it still
had to bleed over into the SendContinuePacketAndWaitForResponse, but the function is still much
more manageable than before -- partly because of most of the work is done in the ContinueLock
class, and partly because I have factored out a lot of the packet processing code separate
functions (this also makes the functionality more easily testable). Most importantly, there is
none of syncrhonization code in the async thread users -- as far as they are concerned, they just
need to declare a Lock object, and they are good to go (SendPacketAndWaitForResponse is now a
very thin wrapper around the NoLock version of the function, whereas previously it had over 100
lines of synchronization code).  This will make my follow up changes there easy.

I have written a number of unit tests for the new code and I have ran the test suite on linux and
osx with no regressions.

Subscribers: tberghammer

Differential Revision: https://reviews.llvm.org/D22629

llvm-svn: 277139

1 files changed, 146 insertions, 17 deletions

diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index a14c8254041..b9223e73604 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -24,6 +24,7 @@
 #include <algorithm>
 #include <map>
 #include <mutex>
+#include <sstream>
 
 #include "lldb/Breakpoint/Watchpoint.h"
 #include "lldb/Interpreter/Args.h"
@@ -2713,9 +2714,6 @@ ProcessGDBRemote::DoHalt (bool &caused_stop)
 {
     Error error;
 
-    bool timed_out = false;
-    std::unique_lock<std::recursive_mutex> lock;
-
     if (m_public_state.GetValue() == eStateAttaching)
     {
         // We are being asked to halt during an attach. We need to just close
@@ -2723,17 +2721,7 @@ ProcessGDBRemote::DoHalt (bool &caused_stop)
         m_gdb_comm.Disconnect();
     }
     else
-    {
-        if (!m_gdb_comm.SendInterrupt(lock, 2, timed_out))
-        {
-            if (timed_out)
-                error.SetErrorString("timed out sending interrupt packet");
-            else
-                error.SetErrorString("unknown error sending interrupt packet");
-        }
-
-        caused_stop = m_gdb_comm.GetInterruptWasSent ();
-    }
+        caused_stop = m_gdb_comm.Interrupt();
     return error;
 }
 
@@ -3886,7 +3874,8 @@ ProcessGDBRemote::AsyncThread (void *arg)
                                 if (process->GetTarget().GetNonStopModeEnabled())
                                 {
                                     // send the vCont packet
-                                    if (!process->GetGDBRemote().SendvContPacket(process, continue_cstr, continue_cstr_len, response))
+                                    if (!process->GetGDBRemote().SendvContPacket(
+                                            llvm::StringRef(continue_cstr, continue_cstr_len), response))
                                     {
                                         // Something went wrong
                                         done = true;
@@ -3896,7 +3885,9 @@ ProcessGDBRemote::AsyncThread (void *arg)
                                 // If in All-Stop-Mode
                                 else
                                 {
-                                    StateType stop_state = process->GetGDBRemote().SendContinuePacketAndWaitForResponse (process, continue_cstr, continue_cstr_len, response);
+                                    StateType stop_state = process->GetGDBRemote().SendContinuePacketAndWaitForResponse(
+                                        *process, *process->GetUnixSignals(),
+                                        llvm::StringRef(continue_cstr, continue_cstr_len), response);
 
                                     // We need to immediately clear the thread ID list so we are sure to get a valid list of threads.
                                     // The thread ID list might be contained within the "response", or the stop reply packet that
@@ -5038,6 +5029,144 @@ ProcessGDBRemote::ModulesDidLoad (ModuleList &module_list)
     m_gdb_comm.ServeSymbolLookups(this);
 }
 
+void
+ProcessGDBRemote::HandleAsyncStdout(llvm::StringRef out)
+{
+    AppendSTDOUT(out.data(), out.size());
+}
+
+static const char *end_delimiter = "--end--;";
+static const int end_delimiter_len = 8;
+
+void
+ProcessGDBRemote::HandleAsyncMisc(llvm::StringRef data)
+{
+    std::string input = data.str(); // '1' to move beyond 'A'
+    if (m_partial_profile_data.length() > 0)
+    {
+        m_partial_profile_data.append(input);
+        input = m_partial_profile_data;
+        m_partial_profile_data.clear();
+    }
+
+    size_t found, pos = 0, len = input.length();
+    while ((found = input.find(end_delimiter, pos)) != std::string::npos)
+    {
+        StringExtractorGDBRemote profileDataExtractor(input.substr(pos, found).c_str());
+        std::string profile_data = HarmonizeThreadIdsForProfileData(profileDataExtractor);
+        BroadcastAsyncProfileData(profile_data);
+
+        pos = found + end_delimiter_len;
+    }
+
+    if (pos < len)
+    {
+        // Last incomplete chunk.
+        m_partial_profile_data = input.substr(pos);
+    }
+}
+
+std::string
+ProcessGDBRemote::HarmonizeThreadIdsForProfileData(StringExtractorGDBRemote &profileDataExtractor)
+{
+    std::map<uint64_t, uint32_t> new_thread_id_to_used_usec_map;
+    std::stringstream final_output;
+    std::string name, value;
+
+    // Going to assuming thread_used_usec comes first, else bail out.
+    while (profileDataExtractor.GetNameColonValue(name, value))
+    {
+        if (name.compare("thread_used_id") == 0)
+        {
+            StringExtractor threadIDHexExtractor(value.c_str());
+            uint64_t thread_id = threadIDHexExtractor.GetHexMaxU64(false, 0);
+
+            bool has_used_usec = false;
+            uint32_t curr_used_usec = 0;
+            std::string usec_name, usec_value;
+            uint32_t input_file_pos = profileDataExtractor.GetFilePos();
+            if (profileDataExtractor.GetNameColonValue(usec_name, usec_value))
+            {
+                if (usec_name.compare("thread_used_usec") == 0)
+                {
+                    has_used_usec = true;
+                    curr_used_usec = strtoull(usec_value.c_str(), NULL, 0);
+                }
+                else
+                {
+                    // We didn't find what we want, it is probably
+                    // an older version. Bail out.
+                    profileDataExtractor.SetFilePos(input_file_pos);
+                }
+            }
+
+            if (has_used_usec)
+            {
+                uint32_t prev_used_usec = 0;
+                std::map<uint64_t, uint32_t>::iterator iterator = m_thread_id_to_used_usec_map.find(thread_id);
+                if (iterator != m_thread_id_to_used_usec_map.end())
+                {
+                    prev_used_usec = m_thread_id_to_used_usec_map[thread_id];
+                }
+
+                uint32_t real_used_usec = curr_used_usec - prev_used_usec;
+                // A good first time record is one that runs for at least 0.25 sec
+                bool good_first_time = (prev_used_usec == 0) && (real_used_usec > 250000);
+                bool good_subsequent_time =
+                    (prev_used_usec > 0) && ((real_used_usec > 0) || (HasAssignedIndexIDToThread(thread_id)));
+
+                if (good_first_time || good_subsequent_time)
+                {
+                    // We try to avoid doing too many index id reservation,
+                    // resulting in fast increase of index ids.
+
+                    final_output << name << ":";
+                    int32_t index_id = AssignIndexIDToThread(thread_id);
+                    final_output << index_id << ";";
+
+                    final_output << usec_name << ":" << usec_value << ";";
+                }
+                else
+                {
+                    // Skip past 'thread_used_name'.
+                    std::string local_name, local_value;
+                    profileDataExtractor.GetNameColonValue(local_name, local_value);
+                }
+
+                // Store current time as previous time so that they can be compared later.
+                new_thread_id_to_used_usec_map[thread_id] = curr_used_usec;
+            }
+            else
+            {
+                // Bail out and use old string.
+                final_output << name << ":" << value << ";";
+            }
+        }
+        else
+        {
+            final_output << name << ":" << value << ";";
+        }
+    }
+    final_output << end_delimiter;
+    m_thread_id_to_used_usec_map = new_thread_id_to_used_usec_map;
+
+    return final_output.str();
+}
+
+void
+ProcessGDBRemote::HandleStopReply()
+{
+    if (GetStopID() != 0)
+        return;
+
+    if (GetID() == LLDB_INVALID_PROCESS_ID)
+    {
+        lldb::pid_t pid = m_gdb_comm.GetCurrentProcessID();
+        if (pid != LLDB_INVALID_PROCESS_ID)
+            SetID(pid);
+    }
+    BuildDynamicRegisterInfo(true);
+}
 
 class CommandObjectProcessGDBRemoteSpeedTest: public CommandObjectParsed
 {
@@ -5245,7 +5374,7 @@ public:
 
                 if (strstr(packet_cstr, "qGetProfileData") != NULL)
                 {
-                    response_str = process->GetGDBRemote().HarmonizeThreadIdsForProfileData(process, response);
+                    response_str = process->HarmonizeThreadIdsForProfileData(response);
                 }
 
                 if (response_str.empty())