mgorny updated this revision to Diff 382227.
mgorny marked 2 inline comments as done.
mgorny added a comment.
Implemented requested changes. Added a minimal-ish unittest.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112169/new/
https://reviews.llvm.org/D112169
Files:
lldb/include/lldb/Core/Communication.h
lldb/source/Core/Communication.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
lldb/unittests/Core/CommunicationTest.cpp
Index: lldb/unittests/Core/CommunicationTest.cpp
===================================================================
--- lldb/unittests/Core/CommunicationTest.cpp
+++ lldb/unittests/Core/CommunicationTest.cpp
@@ -7,11 +7,18 @@
//===----------------------------------------------------------------------===//
#include "lldb/Core/Communication.h"
+#include "lldb/Host/Config.h"
#include "lldb/Host/ConnectionFileDescriptor.h"
#include "lldb/Host/Pipe.h"
#include "llvm/Testing/Support/Error.h"
#include "gtest/gtest.h"
+#include <thread>
+
+#if LLDB_ENABLE_POSIX
+#include <fcntl.h>
+#endif
+
using namespace lldb_private;
#ifndef _WIN32
@@ -35,3 +42,48 @@
ASSERT_TRUE(comm.StopReadThread());
}
#endif
+
+#if LLDB_ENABLE_POSIX
+TEST(CommunicationTest, WriteAll) {
+ Pipe pipe;
+ ASSERT_THAT_ERROR(pipe.CreateNew(/*child_process_inherit=*/false).ToError(),
+ llvm::Succeeded());
+
+ // Make the write end non-blocking in order to easily reproduce a partial
+ // write.
+ int write_fd = pipe.ReleaseWriteFileDescriptor();
+ int flags = fcntl(write_fd, F_GETFL);
+ ASSERT_NE(flags, -1);
+ ASSERT_NE(fcntl(write_fd, F_SETFL, flags | O_NONBLOCK), -1);
+
+ ConnectionFileDescriptor read_conn{pipe.ReleaseReadFileDescriptor(),
+ /*owns_fd=*/true};
+ Communication write_comm("test");
+ write_comm.SetConnection(
+ std::make_unique<ConnectionFileDescriptor>(write_fd, /*owns_fd=*/true));
+
+ std::thread read_thread{[&read_conn]() {
+ // Read using a smaller buffer to increase chances of partial write.
+ char buf[128 * 1024];
+ lldb::ConnectionStatus conn_status;
+
+ do {
+ read_conn.Read(buf, sizeof(buf), std::chrono::seconds(1), conn_status,
+ nullptr);
+ } while (conn_status != lldb::eConnectionStatusEndOfFile);
+ }};
+
+ // Write 1 MiB of data into the pipe.
+ lldb::ConnectionStatus conn_status;
+ Status error;
+ std::vector<char> data(1024 * 1024, 0x80);
+ EXPECT_EQ(write_comm.WriteAll(data.data(), data.size(), conn_status, &error),
+ data.size());
+ EXPECT_EQ(conn_status, lldb::eConnectionStatusSuccess);
+ EXPECT_FALSE(error.Fail());
+
+ // Close the write end in order to trigger EOF.
+ write_comm.Disconnect();
+ read_thread.join();
+}
+#endif
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -101,7 +101,7 @@
Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PACKETS));
ConnectionStatus status = eConnectionStatusSuccess;
char ch = '+';
- const size_t bytes_written = Write(&ch, 1, status, nullptr);
+ const size_t bytes_written = WriteAll(&ch, 1, status, nullptr);
LLDB_LOGF(log, "<%4" PRIu64 "> send packet: %c", (uint64_t)bytes_written, ch);
m_history.AddPacket(ch, GDBRemotePacket::ePacketTypeSend, bytes_written);
return bytes_written;
@@ -111,7 +111,7 @@
Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PACKETS));
ConnectionStatus status = eConnectionStatusSuccess;
char ch = '-';
- const size_t bytes_written = Write(&ch, 1, status, nullptr);
+ const size_t bytes_written = WriteAll(&ch, 1, status, nullptr);
LLDB_LOGF(log, "<%4" PRIu64 "> send packet: %c", (uint64_t)bytes_written, ch);
m_history.AddPacket(ch, GDBRemotePacket::ePacketTypeSend, bytes_written);
return bytes_written;
@@ -137,7 +137,7 @@
ConnectionStatus status = eConnectionStatusSuccess;
const char *packet_data = packet.data();
const size_t packet_length = packet.size();
- size_t bytes_written = Write(packet_data, packet_length, status, nullptr);
+ size_t bytes_written = WriteAll(packet_data, packet_length, status, nullptr);
if (log) {
size_t binary_start_offset = 0;
if (strncmp(packet_data, "$vFile:pwrite:", strlen("$vFile:pwrite:")) ==
Index: lldb/source/Core/Communication.cpp
===================================================================
--- lldb/source/Core/Communication.cpp
+++ lldb/source/Core/Communication.cpp
@@ -189,6 +189,16 @@
return 0;
}
+size_t Communication::WriteAll(const void *src, size_t src_len,
+ ConnectionStatus &status, Status *error_ptr) {
+ size_t total_written = 0;
+ do
+ total_written += Write(static_cast<const char *>(src) + total_written,
+ src_len - total_written, status, error_ptr);
+ while (status == eConnectionStatusSuccess && total_written < src_len);
+ return total_written;
+}
+
bool Communication::StartReadThread(Status *error_ptr) {
if (error_ptr)
error_ptr->Clear();
Index: lldb/include/lldb/Core/Communication.h
===================================================================
--- lldb/include/lldb/Core/Communication.h
+++ lldb/include/lldb/Core/Communication.h
@@ -209,6 +209,22 @@
size_t Write(const void *src, size_t src_len, lldb::ConnectionStatus &status,
Status *error_ptr);
+ /// Repeatedly attempt writing until either \a src_len bytes are written
+ /// or a permanent failure occurs.
+ ///
+ /// \param[in] src
+ /// A source buffer that must be at least \a src_len bytes
+ /// long.
+ ///
+ /// \param[in] src_len
+ /// The number of bytes to attempt to write, and also the
+ /// number of bytes are currently available in \a src.
+ ///
+ /// \return
+ /// The number of bytes actually Written.
+ size_t WriteAll(const void *src, size_t src_len,
+ lldb::ConnectionStatus &status, Status *error_ptr);
+
/// Sets the connection that it to be used by this class.
///
/// By making a communication class that uses different connections it
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
