Change ProcessGDBRemote last stop packet to a container

[EwanCrawford]

Summary

In ProcessGDBRemote we currently have a single packet, m_last_stop_packet, used to set the thread stop info.
However in non-stop mode we can receive several stop reply packets in a sequence for different threads. As a result we need to use a container to hold them before they are processed.

This patch also changes the return type of CheckPacket() so we can detect async notification packets.

[labath]

Hi. Could you upload the patch with full context next time (git diff -U9999, or equivalent). This makes it easier to review changes in phabricator.

[EwanCrawford]

No problem, if anyone's interested in svn you can do 'svn diff --diff-cmd diff -x -U9999'

[EwanCrawford]

Friendly ping

[clayborg]

Looks fine.

source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h

     {
         eBroadcastBitRunPacketSent = kLoUserBroadcastBit
     };
-    
+    enum class PacketType
+    {
+        Invalid = 0,
+        Standard,
+        Notify
+    };
+
     enum class PacketResult
     {
         Success = 0,        // Success

     bool
     GetSequenceMutex (Mutex::Locker& locker, const char *failure_message = NULL);
 
-    bool
+    PacketType
     CheckForPacket (const uint8_t *src, 
                     size_t src_len, 
                     StringExtractorGDBRemote &packet);

source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp

     Log *log (ProcessGDBRemoteLog::GetLogIfAllCategoriesSet (GDBR_LOG_PACKETS | GDBR_LOG_VERBOSE));
 
     // Check for a packet from our cache first without trying any reading...
-    if (CheckForPacket (NULL, 0, packet))
+    if (CheckForPacket(NULL, 0, packet) != PacketType::Invalid)
         return PacketResult::Success;
 
     bool timed_out = false;

 
         if (bytes_read > 0)
         {
-            if (CheckForPacket (buffer, bytes_read, packet))
+            if (CheckForPacket(buffer, bytes_read, packet) != PacketType::Invalid)
                 return PacketResult::Success;
         }
         else

         return PacketResult::ErrorReplyFailed;
 }
 
-bool
+GDBRemoteCommunication::PacketType
 GDBRemoteCommunication::CheckForPacket (const uint8_t *src, size_t src_len, StringExtractorGDBRemote &packet)
 {
     // Put the packet data into the buffer in a thread safe fashion

         m_bytes.append ((const char *)src, src_len);
     }
 
+    bool isNotifyPacket = false;
+
     // Parse up the packets into gdb remote packets
     if (!m_bytes.empty())
     {

                 break;
 
             case '%': // Async notify packet
+                isNotifyPacket = true;
+                // Intentional fall through
+
             case '$':
                 // Look for a standard gdb packet?
                 {

         if (content_length == std::string::npos)
         {
             packet.Clear();
-            return false;
+            return GDBRemoteCommunication::PacketType::Invalid;
         }
         else if (total_length > 0)
         {

             
             m_bytes.erase(0, total_length);
             packet.SetFilePos(0);
-            return success;
+
+            if (isNotifyPacket)
+                return GDBRemoteCommunication::PacketType::Notify;
+            else
+                return GDBRemoteCommunication::PacketType::Standard;
         }
     }
     packet.Clear();
-    return false;
+    return GDBRemoteCommunication::PacketType::Invalid;
 }
 
 Error

source/Plugins/Process/gdb-remote/ProcessGDBRemote.h

     Flags m_flags;            // Process specific flags (see eFlags enums)
     GDBRemoteCommunicationClient m_gdb_comm;
     std::atomic<lldb::pid_t> m_debugserver_pid;
-    StringExtractorGDBRemote m_last_stop_packet;
+    std::vector<StringExtractorGDBRemote> m_stop_packet_stack;  // The stop packet stack replaces the last stop packet variable
     Mutex m_last_stop_packet_mutex;
     GDBRemoteDynamicRegisterInfo m_register_info;
     Broadcaster m_async_broadcaster;

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

     m_flags (0),
     m_gdb_comm (),
     m_debugserver_pid (LLDB_INVALID_PROCESS_ID),
-    m_last_stop_packet (),
     m_last_stop_packet_mutex (Mutex::eMutexTypeNormal),
     m_register_info (),
     m_async_broadcaster (NULL, "lldb.process.gdb-remote.async-broadcaster"),

         // We have a valid process
         SetID (pid);
         GetThreadList();
-        if (m_gdb_comm.GetStopReply(m_last_stop_packet))
+        StringExtractorGDBRemote response;
+        if (m_gdb_comm.GetStopReply(response))
         {
+            SetLastStopPacket(response);
 
             // '?' Packets must be handled differently in non-stop mode
             if (GetTarget().GetNonStopModeEnabled())

                 }
             }
 
-            const StateType state = SetThreadStopInfo (m_last_stop_packet);
+            const StateType state = SetThreadStopInfo (response);
             if (state == eStateStopped)
             {
                 SetPrivateState (state);

                 return error;
             }
 
-            if (m_gdb_comm.GetStopReply(m_last_stop_packet))
+            StringExtractorGDBRemote response;
+            if (m_gdb_comm.GetStopReply(response))
             {
-
+                SetLastStopPacket(response);
                 // '?' Packets must be handled differently in non-stop mode
                 if (GetTarget().GetNonStopModeEnabled())
                     HandleStopReplySequence();

                         m_target.MergeArchitecture(host_arch);
                 }
 
-                SetPrivateState (SetThreadStopInfo (m_last_stop_packet));
+                SetPrivateState (SetThreadStopInfo (response));
                 
                 if (!disable_stdio)
                 {

     // Set the thread stop info. It might have a "threads" key whose value is
     // a list of all thread IDs in the current process, so m_thread_ids might
     // get set.
-    SetThreadStopInfo (m_last_stop_packet);
+
+    // Scope for the lock
+    {
+        // Lock the thread stack while we access it
+        Mutex::Locker stop_stack_lock(m_last_stop_packet_mutex);
+        // Get the number of stop packets on the stack
+        int nItems = m_stop_packet_stack.size();
+        // Iterate over them
+        for (int i = 0; i < nItems; i++)
+        {
+            // Get the thread stop info
+            StringExtractorGDBRemote stop_info = m_stop_packet_stack[i];
+            // Process thread stop info
+            SetThreadStopInfo(stop_info);
+        }
+        // Clear the thread stop stack
+        m_stop_packet_stack.clear();
+    }
+
     // Check to see if SetThreadStopInfo() filled in m_thread_ids?
     if (m_thread_ids.empty())
     {

 void
 ProcessGDBRemote::SetLastStopPacket (const StringExtractorGDBRemote &response)
 {
-    Mutex::Locker locker (m_last_stop_packet_mutex);
     const bool did_exec = response.GetStringRef().find(";reason:exec;") != std::string::npos;
     if (did_exec)
     {

         BuildDynamicRegisterInfo (true);
         m_gdb_comm.ResetDiscoverableSettings();
     }
-    m_last_stop_packet = response;
+
+    // Scope the lock
+    {
+        // Lock the thread stack while we access it
+        Mutex::Locker stop_stack_lock(m_last_stop_packet_mutex);
+        // Add this stop packet to the stop packet stack
+        // This stack will get popped and examined when we switch to the
+        // Stopped state
+        m_stop_packet_stack.push_back(response);
+    }
 }