[lldb] [Core] Harmonize Communication::Read() returns w/ thread

Harmonize the status and error values of Communication::Read() when
running with and without read thread.  Prior to this change, Read()
would return eConnectionStatusSuccess if read thread was enabled
and the read timed out or reached end-of-file, rather than
the respective states that are returned if read thread was disabled.
Now, it correctly returns eConnectionStatusTimedOut
and eConnectionStatusEndOfFile, and sets the error respectively.

Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.llvm.org/D132217

diff --git a/lldb/source/Core/Communication.cpp b/lldb/source/Core/Communication.cpp
index d5f8f89..56f0990 100644 (file)
--- a/lldb/source/Core/Communication.cpp
+++ b/lldb/source/Core/Communication.cpp
@@ -132,10 +132,16 @@ size_t Communication::Read(void *dst, size_t dst_len,
   if (m_read_thread_enabled) {
     // We have a dedicated read thread that is getting data for us
     size_t cached_bytes = GetCachedBytes(dst, dst_len);
-    if (cached_bytes > 0 || (timeout && timeout->count() == 0)) {
+    if (cached_bytes > 0) {
       status = eConnectionStatusSuccess;
       return cached_bytes;
     }
+    if (timeout && timeout->count() == 0) {
+      if (error_ptr)
+        error_ptr->SetErrorString("Timed out.");
+      status = eConnectionStatusTimedOut;
+      return 0;
+    }
 
     if (!m_connection_sp) {
       if (error_ptr)
@@ -155,11 +161,25 @@ size_t Communication::Read(void *dst, size_t dst_len,
       }
 
       if (event_type & eBroadcastBitReadThreadDidExit) {
+        // If the thread exited of its own accord, it either means it
+        // hit an end-of-file condition or lost connection
+        // (we verified that we had an connection above).
+        if (!m_connection_sp) {
+          if (error_ptr)
+            error_ptr->SetErrorString("Lost connection.");
+          status = eConnectionStatusLostConnection;
+        } else
+          status = eConnectionStatusEndOfFile;
+
         if (GetCloseOnEOF())
           Disconnect(nullptr);
-        break;
+        return 0;
       }
     }
+
+    if (error_ptr)
+      error_ptr->SetErrorString("Timed out.");
+    status = eConnectionStatusTimedOut;
     return 0;
   }
 

diff --git a/lldb/unittests/Core/CommunicationTest.cpp b/lldb/unittests/Core/CommunicationTest.cpp
index bb57a7b..024684a 100644 (file)
--- a/lldb/unittests/Core/CommunicationTest.cpp
+++ b/lldb/unittests/Core/CommunicationTest.cpp
@@ -21,6 +21,72 @@
 
 using namespace lldb_private;
 
+static void CommunicationReadTest(bool use_read_thread) {
+  Pipe pipe;
+  ASSERT_THAT_ERROR(pipe.CreateNew(/*child_process_inherit=*/false).ToError(),
+                    llvm::Succeeded());
+
+  Status error;
+  size_t bytes_written;
+  ASSERT_THAT_ERROR(pipe.Write("test", 4, bytes_written).ToError(),
+                    llvm::Succeeded());
+  ASSERT_EQ(bytes_written, 4U);
+
+  Communication comm("test");
+  comm.SetConnection(std::make_unique<ConnectionFileDescriptor>(
+      pipe.ReleaseReadFileDescriptor(), /*owns_fd=*/true));
+  comm.SetCloseOnEOF(true);
+
+  if (use_read_thread)
+    ASSERT_TRUE(comm.StartReadThread());
+
+  // This read should wait for the data to become available and return it.
+  lldb::ConnectionStatus status = lldb::eConnectionStatusSuccess;
+  char buf[16];
+  error.Clear();
+  EXPECT_EQ(
+      comm.Read(buf, sizeof(buf), std::chrono::seconds(5), status, &error), 4U);
+  EXPECT_EQ(status, lldb::eConnectionStatusSuccess);
+  EXPECT_THAT_ERROR(error.ToError(), llvm::Succeeded());
+  buf[4] = 0;
+  EXPECT_STREQ(buf, "test");
+
+  // These reads should time out as there is no more data.
+  error.Clear();
+  EXPECT_EQ(comm.Read(buf, sizeof(buf), std::chrono::microseconds(10), status,
+                      &error),
+            0U);
+  EXPECT_EQ(status, lldb::eConnectionStatusTimedOut);
+  EXPECT_THAT_ERROR(error.ToError(), llvm::Failed());
+
+  // 0 is special-cased, so we test it separately.
+  error.Clear();
+  EXPECT_EQ(
+      comm.Read(buf, sizeof(buf), std::chrono::seconds(0), status, &error), 0U);
+  EXPECT_EQ(status, lldb::eConnectionStatusTimedOut);
+  EXPECT_THAT_ERROR(error.ToError(), llvm::Failed());
+
+  // This read should return EOF.
+  pipe.CloseWriteFileDescriptor();
+  error.Clear();
+  EXPECT_EQ(
+      comm.Read(buf, sizeof(buf), std::chrono::seconds(5), status, &error), 0U);
+  EXPECT_EQ(status, lldb::eConnectionStatusEndOfFile);
+  EXPECT_THAT_ERROR(error.ToError(), llvm::Succeeded());
+
+  // JoinReadThread() should just return immediately since there was no read
+  // thread started.
+  EXPECT_TRUE(comm.JoinReadThread());
+}
+
+TEST(CommunicationTest, Read) {
+  CommunicationReadTest(/*use_thread=*/false);
+}
+
+TEST(CommunicationTest, ReadThread) {
+  CommunicationReadTest(/*use_thread=*/true);
+}
+
 #ifndef _WIN32
 TEST(CommunicationTest, SynchronizeWhileClosing) {
   // Set up a communication object reading from a pipe.