Author: Michał Górny Date: 2022-08-19T15:36:03+02:00 New Revision: d6e1e01da949c8cb4b869cee1953cf19a6d072c0
URL: https://github.com/llvm/llvm-project/commit/d6e1e01da949c8cb4b869cee1953cf19a6d072c0 DIFF: https://github.com/llvm/llvm-project/commit/d6e1e01da949c8cb4b869cee1953cf19a6d072c0.diff LOG: [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 Added: Modified: lldb/source/Core/Communication.cpp lldb/unittests/Core/CommunicationTest.cpp Removed: ################################################################################ diff --git a/lldb/source/Core/Communication.cpp b/lldb/source/Core/Communication.cpp index d5f8f8922f5e1..56f0990d8c7f7 100644 --- 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 bb57a7b14670f..024684a486c2d 100644 --- 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. _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
