[lldb] [Communication] Add a WriteAll() method that resumes writing

[mgorny]

Summary

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.

The solution used utilizes a short sleep in order to wait for the write
buffer to be freed. Using select() would be better but it would require
much deeper changes to the code and does not seem justified at the time.

[mgorny]

clang-format

[labath]

I think it would be worth at least spending a bit of time to understand the reason for using non-blocking i/o here. Given that all Reads should go through the select(2) in ConnectionFileDescriptor::BytesAvailable, it's possible we could avoid it altogether.

[mgorny]

I'm not against that. However, unless I'm mistaken — even with blocking I/O, we need to account for the write() being interrupted in the middle of operation and returning less than src_len.

[labath]

In D112169#3083380, @mgorny wrote:

I'm not against that. However, unless I'm mistaken — even with blocking I/O, we need to account for the write() being interrupted in the middle of operation and returning less than src_len.

You're not mistaken. However, we can get rid of the funky sleep.

[mgorny]

sleep() is no longer necessary now that we're operating in blocking mode.

[labath]

Maybe a quick unit test which send like 1MB of data in a single call or something similar?

lldb/include/lldb/Core/Communication.h

195

It might be worthwhile to emphasize that this will happily return after a partial write and point the reader to WriteAll if he wants to write everything.

lldb/source/Core/Communication.cpp

29

delete

199

It looks like this could be a do-while loop.

[mgorny]

Implemented requested changes. Added a minimal-ish unittest.

lldb/source/Core/Communication.cpp

199

Ah, yes, without the sleep it makes sense.

[mgorny]

lldb/unittests/Core/CommunicationTest.cpp

80

If we want to be super-strict, I can add an another Write() call above to verify that a partial write happens — but I'm not sure if we should fail on that.

lldb/include/lldb/Core/Communication.h

public:
   /// \return
   ///     The number of bytes actually read.
   ///
   /// \see size_t Connection::Read (void *, size_t);
   size_t Read(void *dst, size_t dst_len, const Timeout<std::micro> &timeout,
               lldb::ConnectionStatus &status, Status *error_ptr);
 
   /// The actual write function that attempts to write to the communications
   /// protocol.
   ///
   /// Subclasses must override this function.
   ///
   /// \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 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
   /// allows a single communication interface to negotiate and change its
   /// connection without any interruption to the client. It also allows the
   /// Communication class to be subclassed for packet based communication.
   ///
   /// \param[in] connection

lldb/source/Core/Communication.cpp

 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/Compiler.h"
 
 #include <algorithm>
 #include <chrono>
 #include <cstring>
 #include <memory>
 
 #include <cerrno>
 #include <cinttypes>
 #include <cstdio>
 
 using namespace lldb;
 using namespace lldb_private;
 
 ConstString &Communication::GetStaticBroadcasterClass() {
if (connection_sp)
     return connection_sp->Write(src, src_len, status, error_ptr);
 
   if (error_ptr)
     error_ptr->SetErrorString("Invalid connection.");
   status = eConnectionStatusNoConnection;
   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();
 
   if (m_read_thread.IsJoinable())
     return true;
 
   LLDB_LOG(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_COMMUNICATION),

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp

char GDBRemoteCommunication::CalculcateChecksum(llvm::StringRef payload) {
 
   return checksum & 255;
 }
 
 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;
 }
 
 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;
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunication::SendPacketNoLock(llvm::StringRef payload) {
   StreamString packet(0, 4, eByteOrderBig);

 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunication::SendRawPacketNoLock(llvm::StringRef packet,
                                             bool skip_ack) {
   if (IsConnected()) {
     Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PACKETS));
     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:")) ==
           0) {
         const char *first_comma = strchr(packet_data, ',');
         if (first_comma) {
           const char *second_comma = strchr(first_comma + 1, ',');
           if (second_comma)

lldb/unittests/Core/CommunicationTest.cpp

 //===-- CommunicationTest.cpp ---------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #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
 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");
   comm.SetConnection(std::make_unique<ConnectionFileDescriptor>(
       pipe.ReleaseReadFileDescriptor(), /*owns_fd=*/true));
   comm.SetCloseOnEOF(true);
   ASSERT_TRUE(comm.StartReadThread());
 
   // Ensure that we can safely synchronize with the read thread while it is
   // closing the read end (in response to us closing the write end).
   pipe.CloseWriteFileDescriptor();
   comm.SynchronizeWithReadThread();
 
   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