[Lldb-commits] [lldb] r221181 - The change previously committed as 220983 broke large binary memory reads. I kept the "idx - 1" fix from 220983, but reverted the while loop that was incorrectly added.

[Greg Clayton]

Author: gclayton
Date: Mon Nov  3 15:02:54 2014
New Revision: 221181

URL: http://llvm.org/viewvc/llvm-project?rev=221181&view=rev
Log:
The change previously committed as 220983 broke large binary memory reads. I kept the "idx - 1" fix from 220983, but reverted the while loop that was incorrectly added.

The details are: large packets (like large memory reads (m packets) or large binary memory reads (x packet)) can get responses that come in across multiple read() calls. The while loop that was added meant that if only a partial packet came in (like only "$abc" coming for a response) GDBRemoteCommunication::CheckForPacket() was called, it would deadlock in the while loop because no more data is going to come in as this function needs to be called again with more data from another read. So the original fix will need to be corrected and resubmitted.

<rdar://problem/18853744>


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

Modified: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp?rev=221181&r1=221180&r2=221181&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp (original)
+++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp Mon Nov  3 15:02:54 2014
@@ -411,77 +411,74 @@ GDBRemoteCommunication::CheckForPacket (
         // it off with an invalid value that is the same as the current
         // index.
         size_t content_start = 0;
-        size_t content_length = std::string::npos;
+        size_t content_length = 0;
         size_t total_length = 0;
         size_t checksum_idx = std::string::npos;
 
-        while (!m_bytes.empty() && content_length == std::string::npos)
+        switch (m_bytes[0])
         {
-            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 '$':
-                    // Look for a standard gdb packet?
+            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 '$':
+                // Look for a standard gdb packet?
+                {
+                    size_t hash_pos = m_bytes.find('#');
+                    if (hash_pos != std::string::npos)
                     {
-                        size_t hash_pos = m_bytes.find('#');
-                        if (hash_pos != std::string::npos)
+                        if (hash_pos + 2 < m_bytes.size())
                         {
-                            if (hash_pos + 2 < m_bytes.size())
-                            {
-                                checksum_idx = hash_pos + 1;
-                                // Skip the dollar sign
-                                content_start = 1;
-                                // Don't include the # in the content or the $ in the content length
-                                content_length = hash_pos - 1;
-
-                                total_length = hash_pos + 3; // Skip the # and the two hex checksum bytes
-                            }
-                            else
-                            {
-                                // Checksum bytes aren't all here yet
-                                content_length = std::string::npos;
-                            }
+                            checksum_idx = hash_pos + 1;
+                            // Skip the dollar sign
+                            content_start = 1; 
+                            // Don't include the # in the content or the $ in the content length
+                            content_length = hash_pos - 1;  
+                            
+                            total_length = hash_pos + 3; // Skip the # and the two hex checksum bytes
+                        }
+                        else
+                        {
+                            // Checksum bytes aren't all here yet
+                            content_length = std::string::npos;
                         }
                     }
-                    break;
+                }
+                break;
 
-                default:
+            default:
+                {
+                    // We have an unexpected byte and we need to flush all bad 
+                    // data that is in m_bytes, so we need to find the first
+                    // byte that is a '+' (ACK), '-' (NACK), \x03 (CTRL+C interrupt),
+                    // or '$' character (start of packet header) or of course,
+                    // the end of the data in m_bytes...
+                    const size_t bytes_len = m_bytes.size();
+                    bool done = false;
+                    uint32_t idx;
+                    for (idx = 1; !done && idx < bytes_len; ++idx)
                     {
-                        // We have an unexpected byte and we need to flush all bad
-                        // data that is in m_bytes, so we need to find the first
-                        // byte that is a '+' (ACK), '-' (NACK), \x03 (CTRL+C interrupt),
-                        // or '$' character (start of packet header) or of course,
-                        // the end of the data in m_bytes...
-                        const size_t bytes_len = m_bytes.size();
-                        bool done = false;
-                        uint32_t idx;
-                        for (idx = 1; !done && idx < bytes_len; ++idx)
+                        switch (m_bytes[idx])
                         {
-                            switch (m_bytes[idx])
-                            {
-                            case '+':
-                            case '-':
-                            case '\x03':
-                            case '$':
-                                done = true;
-                                break;
-
-                            default:
-                                break;
-                            }
+                        case '+':
+                        case '-':
+                        case '\x03':
+                        case '$':
+                            done = true;
+                            break;
+                                
+                        default:
+                            break;
                         }
-                        if (log)
-                            log->Printf ("GDBRemoteCommunication::%s tossing %u junk bytes: '%.*s'",
-                                         __FUNCTION__, idx - 1, idx - 1, m_bytes.c_str());
-                        m_bytes.erase(0, idx - 1);
                     }
-                    break;
-            }
+                    if (log)
+                        log->Printf ("GDBRemoteCommunication::%s tossing %u junk bytes: '%.*s'",
+                                     __FUNCTION__, idx - 1, idx - 1, m_bytes.c_str());
+                    m_bytes.erase(0, idx - 1);
+                }
+                break;
         }
 
         if (content_length == std::string::npos)