[Lldb-commits] [lldb] f279e50 - [lldb] [Communication] Add a WriteAll() method that resumes writing
Michał Górny via lldb-commits
lldb-commits at lists.llvm.org
Tue Oct 26 03:45:53 PDT 2021
Author: Michał Górny
Date: 2021-10-26T12:45:45+02:00
New Revision: f279e50fd0f0035e0205f4d36e7e5a8e0112fc24
URL: https://github.com/llvm/llvm-project/commit/f279e50fd0f0035e0205f4d36e7e5a8e0112fc24
DIFF: https://github.com/llvm/llvm-project/commit/f279e50fd0f0035e0205f4d36e7e5a8e0112fc24.diff
LOG: [lldb] [Communication] Add a WriteAll() method that resumes writing
Add a Communication::WriteAll() that resumes Write() if the initial call
did not write all data. Use it in GDBRemoteCommunication when sending
packets in order to fix handling partial writes (i.e. just resume/retry
them rather than erring out). This fixes LLDB failures when writing
large packets to a pty.
Differential Revision: https://reviews.llvm.org/D112169
Added:
Modified:
lldb/include/lldb/Core/Communication.h
lldb/source/Core/Communication.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
lldb/unittests/Core/CommunicationTest.cpp
Removed:
################################################################################
diff --git a/lldb/include/lldb/Core/Communication.h b/lldb/include/lldb/Core/Communication.h
index 930e927f6783..fdcb6c5fb982 100644
--- a/lldb/include/lldb/Core/Communication.h
+++ b/lldb/include/lldb/Core/Communication.h
@@ -209,6 +209,22 @@ class Communication : public Broadcaster {
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
diff erent connections it
diff --git a/lldb/source/Core/Communication.cpp b/lldb/source/Core/Communication.cpp
index fd20b58d18b4..0ad2751f24f0 100644
--- a/lldb/source/Core/Communication.cpp
+++ b/lldb/source/Core/Communication.cpp
@@ -189,6 +189,16 @@ size_t Communication::Write(const void *src, size_t src_len,
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();
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
index adc8d6f1e8d0..ae2141d30220 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -101,7 +101,7 @@ size_t GDBRemoteCommunication::SendAck() {
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 @@ size_t GDBRemoteCommunication::SendNack() {
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 @@ GDBRemoteCommunication::SendRawPacketNoLock(llvm::StringRef packet,
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:")) ==
diff --git a/lldb/unittests/Core/CommunicationTest.cpp b/lldb/unittests/Core/CommunicationTest.cpp
index 3ddc78d4a5af..c9aba8bb1f1a 100644
--- a/lldb/unittests/Core/CommunicationTest.cpp
+++ b/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 @@ TEST(CommunicationTest, SynchronizeWhileClosing) {
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
More information about the lldb-commits
mailing list