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

[mgorny]

Summary

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

[labath]

Cool stuff. Would it be possible to test for the two paths into a single function (with some ifs, if necessary)? That would make it clearer what (if any) are the differences between the two modes of operation.

[mgorny]

In D132217#3734749, @labath wrote:

Cool stuff. Would it be possible to test for the two paths into a single function (with some ifs, if necessary)? That would make it clearer what (if any) are the differences between the two modes of operation.

I suppose this makes sense (why didn't I think of it?!). I was thinking of using parametrized tests but they seemed a bit too complex for two cases.

[mgorny]

Use a common test function.

[mgorny]

@labath, any chance you could take a look at the failures on Windows? I'm wondering if this is because "pipes don't work like that on Windows" or if there's a way to make them work. For now I'm just going to skip these tests on win32.

https://lab.llvm.org/buildbot/#/builders/83/builds/22672

[labath]

In D132217#3735180, @mgorny wrote:

@labath, any chance you could take a look at the failures on Windows? I'm wondering if this is because "pipes don't work like that on Windows" or if there's a way to make them work. For now I'm just going to skip these tests on win32.

https://lab.llvm.org/buildbot/#/builders/83/builds/22672

I'm not sure why the first write fails, but I believe that even if we fixed that, that we would have problems with the read operation, as the pipe FD will not work with select. I think the safest/simplest way to fix this would be to change the test to use sockets instead.

lldb/source/Core/Communication.cpp

size_t Communication::Read(void *dst, size_t dst_len,
   LLDB_LOG(
       log,
       "this = {0}, dst = {1}, dst_len = {2}, timeout = {3}, connection = {4}",
       this, dst, dst_len, timeout, m_connection_sp.get());
 
   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)
         error_ptr->SetErrorString("Invalid connection.");
       status = eConnectionStatusNoConnection;
       return 0;
     }
 
     ListenerSP listener_sp(Listener::MakeListener("Communication::Read"));
     listener_sp->StartListeningForEvents(
         this, eBroadcastBitReadThreadGotBytes | eBroadcastBitReadThreadDidExit);
     EventSP event_sp;
     while (listener_sp->GetEvent(event_sp, timeout)) {
       const uint32_t event_type = event_sp->GetType();
       if (event_type & eBroadcastBitReadThreadGotBytes) {
         return GetCachedBytes(dst, 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;
   }
 
   // We aren't using a read thread, just read the data synchronously in this
   // thread.
   return ReadFromConnection(dst, dst_len, timeout, status, error_ptr);
 }
 

lldb/unittests/Core/CommunicationTest.cpp

 #include <thread>
 
 #if LLDB_ENABLE_POSIX
 #include <fcntl.h>
 #endif
 
 using namespace lldb_private;
 
+TEST(CommunicationTest, Read) {
+  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);
+
+  // 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, ReadThread) {
+  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);
+  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());
+
+  EXPECT_TRUE(comm.JoinReadThread());
+}
+
 #ifndef _WIN32
 TEST(CommunicationTest, SynchronizeWhileClosing) {
   // Set up a communication object reading from a pipe.
   Pipe pipe;
   ASSERT_THAT_ERROR(pipe.CreateNew(/*child_process_inherit=*/false).ToError(),
                     llvm::Succeeded());
 
   Communication comm("test");