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.

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

 
 class GDBRemoteCommunication : public Communication
 {
 public:
     enum
     {
         eBroadcastBitRunPacketSent = kLoUserBroadcastBit
     };
-    
+    enum class PacketType
+    {
+        Invalid = 0,
+        Standard,
+        Notify
+    };
+
     enum class PacketResult
     {
         Success = 0,        // Success
         ErrorSendFailed,    // Error sending the packet
         ErrorSendAck,       // Didn't get an ack back after sending a packet
         ErrorReplyFailed,   // Error getting the reply
         ErrorReplyTimeout,  // Timed out waiting for reply
         ErrorReplyInvalid,  // Got a reply but it wasn't valid for the packet that was sent
public:
 
     char
     CalculcateChecksum (const char *payload,
                         size_t payload_length);
 
     bool
     GetSequenceMutex (Mutex::Locker& locker, const char *failure_message = NULL);
 
-    bool
+    PacketType
     CheckForPacket (const uint8_t *src, 
                     size_t src_len, 
                     StringExtractorGDBRemote &packet);
     bool
     IsRunning() const
     {
         return m_public_is_running.GetValue();
     }

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

 GDBRemoteCommunication::WaitForPacketWithTimeoutMicroSecondsNoLock (StringExtractorGDBRemote &packet, uint32_t timeout_usec)
 {
     uint8_t buffer[8192];
     Error error;
 
     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;
     bool disconnected = false;
     while (IsConnected() && !timed_out)
     {
         lldb::ConnectionStatus status = eConnectionStatusNoConnection;
         size_t bytes_read = Read (buffer, sizeof(buffer), timeout_usec, status, &error);
         
         if (log)
             log->Printf ("%s: Read (buffer, (sizeof(buffer), timeout_usec = 0x%x, status = %s, error = %s) => bytes_read = %" PRIu64,
                          __PRETTY_FUNCTION__,
                          timeout_usec, 
                          Communication::ConnectionStatusAsCString (status),
                          error.AsCString(), 
                          (uint64_t)bytes_read);
 
         if (bytes_read > 0)
         {
-            if (CheckForPacket (buffer, bytes_read, packet))
+            if (CheckForPacket(buffer, bytes_read, packet) != PacketType::Invalid)
                 return PacketResult::Success;
         }
         else
         {
             switch (status)
             {
             case eConnectionStatusTimedOut:
             case eConnectionStatusInterrupted:
GDBRemoteCommunication::WaitForPacketWithTimeoutMicroSecondsNoLock (StringExtractorGDBRemote &packet, uint32_t timeout_usec)
     if (disconnected)
         return PacketResult::ErrorDisconnected;
     if (timed_out)
         return PacketResult::ErrorReplyTimeout;
     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
     Mutex::Locker locker(m_bytes_mutex);
     
     Log *log (ProcessGDBRemoteLog::GetLogIfAllCategoriesSet (GDBR_LOG_PACKETS));
 
     if (src && src_len > 0)
     {
         if (log && log->GetVerbose())
         {
             StreamString s;
             log->Printf ("GDBRemoteCommunication::%s adding %u bytes: %.*s",
                          __FUNCTION__, 
                          (uint32_t)src_len, 
                          (uint32_t)src_len, 
                          src);
         }
         m_bytes.append ((const char *)src, src_len);
     }
 
+    bool isNotifyPacket = false;
+
     // Parse up the packets into gdb remote packets
     if (!m_bytes.empty())
     {
         // end_idx must be one past the last valid packet byte. Start
         // it off with an invalid value that is the same as the current
         // index.
         size_t content_start = 0;
         size_t content_length = 0;
         size_t total_length = 0;
         size_t checksum_idx = std::string::npos;
 
         switch (m_bytes[0])
         {
             case '+':       // Look for ack
             case '-':       // Look for cancel
             case '\x03':    // ^C to halt target
                 content_length = total_length = 1;  // The command is one byte long...
                 break;
 
             case '%': // Async notify packet
+                isNotifyPacket = true;
+                // Intentional fall through
+
             case '$':
                 // Look for a standard gdb packet?
                 {
                     size_t hash_pos = m_bytes.find('#');
                     if (hash_pos != std::string::npos)
                     {
                         if (hash_pos + 2 < m_bytes.size())
                         {
if (!m_bytes.empty())
                     m_bytes.erase(0, idx - 1);
                 }
                 break;
         }
 
         if (content_length == std::string::npos)
         {
             packet.Clear();
-            return false;
+            return GDBRemoteCommunication::PacketType::Invalid;
         }
         else if (total_length > 0)
         {
 
             // We have a valid packet...
             assert (content_length <= m_bytes.size());
             assert (total_length <= m_bytes.size());
             assert (content_length <= total_length);
if (!m_bytes.empty())
                     success = false;
                     if (log)
                         log->Printf ("error: invalid checksum in packet: '%s'\n", m_bytes.c_str());
                 }
             }
             
             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
 GDBRemoteCommunication::StartListenThread (const char *hostname, uint16_t port)
 {
     Error error;
     if (m_listen_thread.IsJoinable())
     {

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

enum
         eBroadcastBitAsyncContinue                  = (1 << 0),
         eBroadcastBitAsyncThreadShouldExit          = (1 << 1),
         eBroadcastBitAsyncThreadDidExit             = (1 << 2)
     };
     
     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;
     HostThread m_async_thread;
     Mutex m_async_thread_state_mutex;
     typedef std::vector<lldb::tid_t> tid_collection;
     typedef std::vector< std::pair<lldb::tid_t,int> > tid_sig_collection;
     typedef std::map<lldb::addr_t, lldb::addr_t> MMapMap;

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

 //----------------------------------------------------------------------
 // ProcessGDBRemote constructor
 //----------------------------------------------------------------------
 ProcessGDBRemote::ProcessGDBRemote(Target& target, Listener &listener) :
     Process (target, listener),
     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"),
     m_async_thread_state_mutex(Mutex::eMutexTypeRecursive),
     m_thread_ids (),
     m_continue_c_tids (),
     m_continue_C_tids (),
     m_continue_s_tids (),
if (pid == LLDB_INVALID_PROCESS_ID)
         // listings...
         SetPrivateState (eStateConnected);
     }
     else
     {
         // 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())
                 HandleStopReplySequence();
 
             if (!m_target.GetArchitecture().IsValid()) 
             {
                 if (m_gdb_comm.GetProcessArchitecture().IsValid())
                 {
                     m_target.SetArchitecture(m_gdb_comm.GetProcessArchitecture());
                 }
                 else
                 {
                     m_target.SetArchitecture(m_gdb_comm.GetHostArchitecture());
                 }
             }
 
-            const StateType state = SetThreadStopInfo (m_last_stop_packet);
+            const StateType state = SetThreadStopInfo (response);
             if (state == eStateStopped)
             {
                 SetPrivateState (state);
             }
             else
                 error.SetErrorStringWithFormat ("Process %" PRIu64 " was reported after connecting to '%s', but state was not stopped: %s", pid, remote_url, StateAsCString (state));
         }
         else
if (object_file)
             if (GetID() == LLDB_INVALID_PROCESS_ID)
             {
                 if (log)
                     log->Printf("failed to connect to debugserver: %s", error.AsCString());
                 KillDebugserverProcess ();
                 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();
 
                 const ArchSpec &process_arch = m_gdb_comm.GetProcessArchitecture();
 
                 if (process_arch.IsValid())
                 {
                     m_target.MergeArchitecture(process_arch);
                 }
                 else
                 {
                     const ArchSpec &host_arch = m_gdb_comm.GetHostArchitecture();
                     if (host_arch.IsValid())
                         m_target.MergeArchitecture(host_arch);
                 }
 
-                SetPrivateState (SetThreadStopInfo (m_last_stop_packet));
+                SetPrivateState (SetThreadStopInfo (response));
                 
                 if (!disable_stdio)
                 {
                     if (pty.GetMasterFileDescriptor() != lldb_utility::PseudoTerminal::invalid_fd)
                         SetSTDIOFileDescriptor (pty.ReleaseMasterFileDescriptor());
                 }
             }
         }

 void
 ProcessGDBRemote::RefreshStateAfterStop ()
 {
     Mutex::Locker locker(m_thread_list_real.GetMutex());
     m_thread_ids.clear();
     // 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())
     {
         // No, we need to fetch the thread list manually
         UpdateThreadIDList();
     }
 
     // If we have queried for a default thread id
#endif
     StopAsyncThread ();
     KillDebugserverProcess ();
     return error;
 }
 
 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)
     {
         Log *log (ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
         if (log)
             log->Printf ("ProcessGDBRemote::SetLastStopPacket () - detected exec");
 
         m_thread_list_real.Clear();
         m_thread_list.Clear();
         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);
+    }
 }
 
 
 //------------------------------------------------------------------
 // Process Queries
 //------------------------------------------------------------------
 
 bool