Add a read_full_buffer argument to ConnectionFileDescriptor::Read

Summary:
AdbClient was attempting to handle the case where the socket input arrived in pieces, but it was
failing to handle the case where the connection was closed before that happened. In this case, it
would just spin in an infinite loop calling Connection::Read. (This was also the cause of the
spurious timeouts on the darwin->android buildbot. The exact cause of the premature EOF remains
to be investigated, but is likely a server bug.)

Since this wait-for-a-certain-number-of-bytes seems like a useful functionality to have, I am
moving it (with the infinite loop fixed) to the Connection class, and adding an
appropriate test for it.

Reviewers: clayborg, zturner, ovyalov

Subscribers: tberghammer, danalbert, lldb-commits

Differential Revision: http://reviews.llvm.org/D19533

llvm-svn: 268380

5 files changed, 50 insertions, 41 deletions

diff --git a/lldb/source/Core/Communication.cpp b/lldb/source/Core/Communication.cpp
index 557fb95df69..7d88a63eb23 100644
--- a/lldb/source/Core/Communication.cpp
+++ b/lldb/source/Core/Communication.cpp
@@ -191,7 +191,8 @@ Communication::Read (void *dst, size_t dst_len, uint32_t timeout_usec, Connectio
     lldb::ConnectionSP connection_sp (m_connection_sp);
     if (connection_sp)
     {
-        return connection_sp->Read (dst, dst_len, timeout_usec, status, error_ptr);
+        const bool read_full_buffer = false;
+        return connection_sp->Read(dst, dst_len, timeout_usec, read_full_buffer, status, error_ptr);
     }
 
     if (error_ptr)
@@ -326,7 +327,8 @@ Communication::ReadFromConnection (void *dst,
                                    Error *error_ptr)
 {
     lldb::ConnectionSP connection_sp(m_connection_sp);
-    return (connection_sp ? connection_sp->Read(dst, dst_len, timeout_usec, status, error_ptr) : 0);
+    const bool read_full_buffer = false;
+    return (connection_sp ? connection_sp->Read(dst, dst_len, read_full_buffer, timeout_usec, status, error_ptr) : 0);
 }
 
 bool
diff --git a/lldb/source/Core/ConnectionSharedMemory.cpp b/lldb/source/Core/ConnectionSharedMemory.cpp
index 707644d36b1..857a280ebb1 100644
--- a/lldb/source/Core/ConnectionSharedMemory.cpp
+++ b/lldb/source/Core/ConnectionSharedMemory.cpp
@@ -94,11 +94,8 @@ ConnectionSharedMemory::Disconnect (Error *error_ptr)
 }
 
 size_t
-ConnectionSharedMemory::Read (void *dst, 
-                              size_t dst_len, 
-                              uint32_t timeout_usec,
-                              ConnectionStatus &status, 
-                              Error *error_ptr)
+ConnectionSharedMemory::Read(void *dst, size_t dst_len, uint32_t timeout_usec, bool read_full_buffer,
+                             ConnectionStatus &status, Error *error_ptr)
 {
     status = eConnectionStatusSuccess;
     return 0;
diff --git a/lldb/source/Host/common/Editline.cpp b/lldb/source/Host/common/Editline.cpp
index 4640154c6cb..3842dc1a956 100644
--- a/lldb/source/Host/common/Editline.cpp
+++ b/lldb/source/Host/common/Editline.cpp
@@ -580,6 +580,7 @@ Editline::GetCharacter (EditLineCharType * c)
     // Read an actual character
     while (true)
     {
+        const bool read_full_buffer = false; // Doesn't really matter, we're reading one byte only.
         lldb::ConnectionStatus status = lldb::eConnectionStatusSuccess;
         char ch = 0;
 
@@ -588,12 +589,12 @@ Editline::GetCharacter (EditLineCharType * c)
         // for someone to interrupt us. After Read returns, immediately lock the mutex again and
         // check if we were interrupted.
         m_output_mutex.Unlock();
-        int read_count = m_input_connection.Read(&ch, 1, UINT32_MAX, status, NULL);
+        int read_count = m_input_connection.Read(&ch, 1, UINT32_MAX, read_full_buffer, status, NULL);
         m_output_mutex.Lock();
         if (m_editor_status == EditorStatus::Interrupted)
         {
             while (read_count > 0 && status == lldb::eConnectionStatusSuccess)
-                read_count = m_input_connection.Read(&ch, 1, UINT32_MAX, status, NULL);
+                read_count = m_input_connection.Read(&ch, 1, UINT32_MAX, read_full_buffer, status, NULL);
             lldbassert(status == lldb::eConnectionStatusInterrupted);
             return 0;
         }
diff --git a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
index dbbd5a1dcc3..a2d99f166d0 100644
--- a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -411,7 +411,8 @@ ConnectionFileDescriptor::Disconnect(Error *error_ptr)
 }
 
 size_t
-ConnectionFileDescriptor::Read(void *dst, size_t dst_len, uint32_t timeout_usec, ConnectionStatus &status, Error *error_ptr)
+ConnectionFileDescriptor::Read(void *dst, size_t dst_len, uint32_t timeout_usec, bool read_full_buffer,
+                               ConnectionStatus &status, Error *error_ptr)
 {
     Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION));
 
@@ -434,26 +435,36 @@ ConnectionFileDescriptor::Read(void *dst, size_t dst_len, uint32_t timeout_usec,
         return 0;
     }
 
-    status = BytesAvailable(timeout_usec, error_ptr);
-    if (status != eConnectionStatusSuccess)
-        return 0;
-
+    size_t total_bytes_read = 0;
+    char *dst_buf = static_cast<char *>(dst);
+    auto now = std::chrono::steady_clock::now();
+    const auto deadline = now + std::chrono::microseconds(timeout_usec);
     Error error;
-    size_t bytes_read = dst_len;
-    error = m_read_sp->Read(dst, bytes_read);
-
-    if (log)
+    do
     {
-        log->Printf("%p ConnectionFileDescriptor::Read()  fd = %" PRIu64 ", dst = %p, dst_len = %" PRIu64 ") => %" PRIu64 ", error = %s",
-                    static_cast<void *>(this), static_cast<uint64_t>(m_read_sp->GetWaitableHandle()), static_cast<void *>(dst),
-                    static_cast<uint64_t>(dst_len), static_cast<uint64_t>(bytes_read), error.AsCString());
-    }
+        timeout_usec = std::chrono::duration_cast<std::chrono::microseconds>(deadline - now).count();
+        status = BytesAvailable(timeout_usec, error_ptr);
+        if (status != eConnectionStatusSuccess)
+            return 0;
 
-    if (bytes_read == 0)
-    {
-        error.Clear(); // End-of-file.  Do not automatically close; pass along for the end-of-file handlers.
-        status = eConnectionStatusEndOfFile;
-    }
+        size_t bytes_read = dst_len - total_bytes_read;
+        error = m_read_sp->Read(dst_buf + total_bytes_read, bytes_read);
+        if (log)
+        {
+            log->Printf("%p ConnectionFileDescriptor::Read()  fd = %" PRIu64 ", dst = %p, dst_len = %" PRIu64
+                        ") => %" PRIu64 ", error = %s",
+                        this, static_cast<uint64_t>(m_read_sp->GetWaitableHandle()), dst,
+                        static_cast<uint64_t>(dst_len), static_cast<uint64_t>(bytes_read), error.AsCString());
+        }
+        total_bytes_read += bytes_read;
+        if (bytes_read == 0)
+        {
+            // End-of-file.  Do not automatically close; pass along for the end-of-file handlers.
+            error.Clear();
+            status = eConnectionStatusEndOfFile;
+        }
+        now = std::chrono::steady_clock::now();
+    } while (read_full_buffer && total_bytes_read < dst_len && status == eConnectionStatusSuccess && now < deadline);
 
     if (error_ptr)
         *error_ptr = error;
@@ -509,7 +520,7 @@ ConnectionFileDescriptor::Read(void *dst, size_t dst_len, uint32_t timeout_usec,
 
         return 0;
     }
-    return bytes_read;
+    return total_bytes_read;
 }
 
 size_t
diff --git a/lldb/source/Plugins/Platform/Android/AdbClient.cpp b/lldb/source/Plugins/Platform/Android/AdbClient.cpp
index 736447fd22d..b4de0a44a4b 100644
--- a/lldb/source/Plugins/Platform/Android/AdbClient.cpp
+++ b/lldb/source/Plugins/Platform/Android/AdbClient.cpp
@@ -34,7 +34,7 @@ using namespace lldb_private::platform_android;
 
 namespace {
 
-const uint32_t kReadTimeout = 1000000; // 1 second
+const uint32_t kReadTimeout = 4000000; // 4 seconds
 const char * kOKAY = "OKAY";
 const char * kFAIL = "FAIL";
 const char * kDATA = "DATA";
@@ -251,7 +251,9 @@ AdbClient::ReadMessageStream (std::vector<char>& message, uint32_t timeout_ms)
         if (elapsed_time >= timeout_ms)
             return Error("Timed out");
 
-        size_t n = m_conn.Read(buffer, sizeof(buffer), 1000 * (timeout_ms - elapsed_time), status, &error);
+        const bool read_full_buffer = true;
+        size_t n =
+            m_conn.Read(buffer, sizeof(buffer), 1000 * (timeout_ms - elapsed_time), read_full_buffer, status, &error);
         if (n > 0)
             message.insert(message.end(), &buffer[0], &buffer[n]);
     }
@@ -490,19 +492,15 @@ AdbClient::ReadSyncHeader (std::string &response_id, uint32_t &data_len)
 Error
 AdbClient::ReadAllBytes (void *buffer, size_t size)
 {
+    const bool read_full_buffer = true;
     Error error;
     ConnectionStatus status;
-    char *read_buffer = static_cast<char*>(buffer);
-
-    size_t tota_read_bytes = 0;
-    while (tota_read_bytes < size)
-    {
-        auto read_bytes = m_conn.Read (read_buffer + tota_read_bytes, size - tota_read_bytes, kReadTimeout, status, &error);
-        if (error.Fail ())
-            return error;
-        tota_read_bytes += read_bytes;
-    }
-    return error;
+    size_t read_bytes = m_conn.Read(buffer, size, kReadTimeout, read_full_buffer, status, &error);
+    if (error.Fail())
+        return error;
+    if (read_bytes < size)
+        return Error("Unable to read full buffer.");
+    return Error();
 }
 
 Error