[Lldb-commits] [PATCH] D112169: [lldb] [Communication] Add a WriteAll() method that resumes writing

[Michał Górny via Phabricator via lldb-commits]

mgorny created this revision.
mgorny added reviewers: labath, krytarowski, emaste, teemperor.
mgorny requested review of this revision.

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.


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


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
@@ -26,6 +26,7 @@
 #include <chrono>
 #include <cstring>
 #include <memory>
+#include <thread>
 
 #include <cerrno>
 #include <cinttypes>
@@ -189,6 +190,22 @@
   return 0;
 }
 
+size_t Communication::WriteAll(const void *src, size_t src_len,
+                               ConnectionStatus &status, Status *error_ptr) {
+  size_t total_written = 0;
+  while (1) {
+    size_t written = Write(static_cast<const char *>(src) + total_written, src_len - total_written, status, error_ptr);
+    total_written += written;
+    if (status != eConnectionStatusSuccess || total_written >= src_len)
+      break;
+    // sleep a short while to avoid wasting CPU cycles while we are unlikely
+    // to be able to write again immediately
+    // FIXME: improve this to use select()
+    std::this_thread::sleep_for(std::chrono::milliseconds(100));
+  }
+  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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D112169.381061.patch
Type: text/x-patch
Size: 3948 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20211020/4512b2e8/attachment.bin>