Add setting to artifically delay communication with the debug server, in order ro simulate slow network conditions

[augusto2112]

This patch introduces a new setting on ProcessGDBRemote to allow artificially slowing down the communication betwenn lldb and the server. The new setting sleeps PacketDelayMultiplier nanoseconds per byte read or written in GDBRemoteCommunication.

I'm not 100% sure on some choices I made, which I'd like to ask you all:

• In order to pass the delay seeting from ProcessGDBRemote to GDBRemoteCommunication, I've added a ivar to the latter, and am passing it through its constructor. This isn't ideal since once the object is build, updating the setting won't affect LLDB, but GDBRemoteCommunication currently has no knowledge of ProcessGDBRemote and the settings class (PluginProperties) is declared inside ProcessGDBRemote.cpp. Can I move the declaration of PluginProperties to ProcessGDBRemote.h and import that in GDBRemoteCommunication.cpp?
• I've made Read and Write virtual in Communication.h. It seems that this was the initial intention (at least for Write), as its documentation states that "Subclasses must override this function". Is this all right?

[jasonmolenda]

Hi Augusto, this is an interesting idea. This would simulate a slow serial connection well, but over a modern USB or Wifi or Bluetooth connection, we find the packet size is nearly irrelevant to performance -- but the number of packets is absolutely critical. If we need to read 100 kbytes, sending ten 10kb read packets is *much* slower than a single 100kb packet. For fun, I built a native debugserver that claims it supported a maximum 1k packet size, and then rebuilt it claiming it supports a maximum 512k packet size. It turns out lldb won't actually use more than a 128kbyte packet size (v. ProcessGDBRemote::GetMaxMemorySize) but anyway,

cat a.c
#include <stdlib.h>
int main() { const int bufsize = 768 * 1024; char *buf = (char* )malloc(bufsize);
    return buf[0]; }

and ran to the return and read the buffer,

(lldb) mem rea -b -c `bufsize` --force -o /tmp/f buf
786432 bytes written to '/tmp/f'

When debugserver said it only supports 1024 packets,

1618455297.760255098 <  45> send packet: $qSupported:xmlRegisters=i386,arm,mips,arc#74
1618455297.760361910 <  46> read packet: $qXfer:features:read+;PacketSize=400;qEcho+#00

it took 2092 memory read packets and 0.537 seconds to complete. When I used 512k byte packets (lldb capped it to 128k arbitrarily), it took 24 packets and 0.340 seconds to complete.

the same amount of data was transferred (except for packet header/footer etc but that's nothing) but it was a lot slower. This was on a native system, but as soon as we have USB or WiFi or Bluetooth in the middle, this effect is exaggerated.

If we were debugging over a serial cable, slowing it down per character would be exactly right. But for instance, debugging to an Apple Watch over Bluetooth (at least with the original watch) could be as slow as ~40 packets per second. It basically didn't matter how big those packets are, that's what we can get out of it. (I haven't benchmarked this in YEARS, I'm sure it's faster now, but the general character of the connection is still true).

It's an interesting idea to embed a tester facility like this into lldb, but this isn't (IMO) what we want to model.

FWIW in the past I've used sloxy when I needed to simulate a slow connection over the internet https://github.com/jakob/sloxy . Usually when I'm doing performance work, I enable the gdb-remote packet logging and count the packets and look at the timestamps. I've never reduced the # of packets to accomplish a task, and turned out to have made a mistake in doing so. :) Testing with a real device is always best if you need to be really sure (or want to measure the actual perf improvement of a change), but I don't see a *ton* of benefit in my work to introducing an artificially slow connection like this. (the bug report I used sloxy on was reporting a tricky race condition in lldb that I've put off figuring out how to fix and not to do with performance ;)

[augusto2112]

Hey @jasonmolenda thanks for the input, and nice empirical experiment! Do you think simply adding a setting that sleeps by a constant amount would be enough to model this problem?

[aprantl]

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemoteProperties.td

23

To simulate slower connections for testing purposes...

lldb/include/lldb/Core/Communication.h

 //===-- Communication.h -----------------------------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_CORE_COMMUNICATION_H
 #define LLDB_CORE_COMMUNICATION_H
 
 #include "lldb/Host/HostThread.h"
 #include "lldb/Utility/Broadcaster.h"
 #include "lldb/Utility/Timeout.h"
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-forward.h"
 #include "lldb/lldb-types.h"
 
 #include <atomic>
public:
   ///
   /// \param[in] timeout
   ///     A timeout value or llvm::None for no timeout.
   ///
   /// \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,
+  virtual 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);
+  virtual size_t Write(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.
   ///

lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h

struct ContinueDelegate {
 
     /// Process asynchronously-received structured data.
     ///
     /// \param[in] data
     ///   The complete data packet, expected to start with JSON-async.
     virtual void HandleAsyncStructuredDataPacket(llvm::StringRef data) = 0;
   };
 
-  GDBRemoteClientBase(const char *comm_name, const char *listener_name);
+  GDBRemoteClientBase(const char *comm_name, const char *listener_name,
+                      std::chrono::nanoseconds delay_multiplier =
+                          std::chrono::nanoseconds::zero());
 
   bool SendAsyncSignal(int signo);
 
   bool Interrupt();
 
   lldb::StateType SendContinuePacketAndWaitForResponse(
       ContinueDelegate &delegate, const UnixSignals &signals,
       llvm::StringRef payload, StringExtractorGDBRemote &response);

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

 static const seconds kInterruptTimeout(5);
 
 /////////////////////////
 // GDBRemoteClientBase //
 /////////////////////////
 
 GDBRemoteClientBase::ContinueDelegate::~ContinueDelegate() = default;
 
-GDBRemoteClientBase::GDBRemoteClientBase(const char *comm_name,
-                                         const char *listener_name)
-    : GDBRemoteCommunication(comm_name, listener_name), m_async_count(0),
-      m_is_running(false), m_should_stop(false) {}
+GDBRemoteClientBase::GDBRemoteClientBase(
+    const char *comm_name, const char *listener_name,
+    std::chrono::nanoseconds delay_multiplier)
+    : GDBRemoteCommunication(comm_name, listener_name, delay_multiplier),
+      m_async_count(0), m_is_running(false), m_should_stop(false) {}
 
 StateType GDBRemoteClientBase::SendContinuePacketAndWaitForResponse(
     ContinueDelegate &delegate, const UnixSignals &signals,
     llvm::StringRef payload, StringExtractorGDBRemote &response) {
   Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
   response.Clear();
 
   {

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

private:
     GDBRemoteCommunication &m_gdb_comm;
     std::chrono::seconds m_saved_timeout;
     // Don't ever reduce the timeout for a packet, only increase it. If the
     // requested timeout if less than the current timeout, we don't set it
     // and won't need to restore it.
     bool m_timeout_modified;
   };
 
-  GDBRemoteCommunication(const char *comm_name, const char *listener_name);
+  GDBRemoteCommunication(const char *comm_name, const char *listener_name,
+                         std::chrono::nanoseconds delay_multiplier =
+                             std::chrono::nanoseconds::zero());
 
   ~GDBRemoteCommunication() override;
 
   PacketResult GetAck();
 
   size_t SendAck();
 
   size_t SendNack();
protected:
   LazyBool m_supports_qEcho;
   GDBRemoteCommunicationHistory m_history;
   bool m_send_acks;
   bool m_is_platform; // Set to true if this class represents a platform,
                       // false if this class represents a debug session for
                       // a single process
 
   CompressionType m_compression_type;
+  std::chrono::nanoseconds m_delay_multiplier;
+
+  size_t Read(void *dst, size_t dst_len, const Timeout<std::micro> &timeout,
+              lldb::ConnectionStatus &status, Status *error_ptr) override;
+
+  size_t Write(const void *src, size_t src_len, lldb::ConnectionStatus &status,
+               Status *error_ptr) override;
 
   PacketResult SendPacketNoLock(llvm::StringRef payload);
   PacketResult SendRawPacketNoLock(llvm::StringRef payload,
                                    bool skip_ack = false);
 
   PacketResult ReadPacket(StringExtractorGDBRemote &response,
                           Timeout<std::micro> timeout, bool sync_on_timeout);
 

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

 #include <zlib.h>
 #endif
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::process_gdb_remote;
 
 // GDBRemoteCommunication constructor
 GDBRemoteCommunication::GDBRemoteCommunication(const char *comm_name,
-                                               const char *listener_name)
+                                               const char *listener_name,
+                                               std::chrono::nanoseconds delay_multiplier)
     : Communication(comm_name),
 #ifdef LLDB_CONFIGURATION_DEBUG
       m_packet_timeout(1000),
 #else
       m_packet_timeout(1),
 #endif
       m_echo_number(0), m_supports_qEcho(eLazyBoolCalculate), m_history(512),
       m_send_acks(true), m_compression_type(CompressionType::None),
-      m_listen_url() {
+      m_delay_multiplier(delay_multiplier), m_listen_url() {
 }
 
 // Destructor
 GDBRemoteCommunication::~GDBRemoteCommunication() {
   if (IsConnected()) {
     Disconnect();
   }
 
char GDBRemoteCommunication::CalculcateChecksum(llvm::StringRef payload) {
   int checksum = 0;
 
   for (char c : payload)
     checksum += c;
 
   return checksum & 255;
 }
 
+size_t GDBRemoteCommunication::Read(void *dst, size_t dst_len,
+                                    const Timeout<std::micro> &timeout,
+                                    lldb::ConnectionStatus &status,
+                                    Status *error_ptr) {
+  size_t size = Communication::Read(dst, dst_len, timeout, status, error_ptr);
+  std::this_thread::sleep_for(m_delay_multiplier * size);
+  return size;
+}
+
+size_t GDBRemoteCommunication::Write(const void *src, size_t src_len,
+                            ConnectionStatus &status, Status *error_ptr) {
+  size_t size = Communication::Write(src, src_len, status, error_ptr);
+  std::this_thread::sleep_for(m_delay_multiplier * size);
+  return size;
+
+}
 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);
   LLDB_LOGF(log, "<%4" PRIu64 "> send packet: %c", (uint64_t)bytes_written, ch);
   m_history.AddPacket(ch, GDBRemotePacket::ePacketTypeSend, bytes_written);
   return bytes_written;

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h

 };
 inline bool operator==(const QOffsets &a, const QOffsets &b) {
   return a.segments == b.segments && a.offsets == b.offsets;
 }
 llvm::raw_ostream &operator<<(llvm::raw_ostream &os, const QOffsets &offsets);
 
 class GDBRemoteCommunicationClient : public GDBRemoteClientBase {
 public:
-  GDBRemoteCommunicationClient();
+  GDBRemoteCommunicationClient(std::chrono::nanoseconds delay_multiplier =
+                                   std::chrono::nanoseconds::zero());
 
   ~GDBRemoteCommunicationClient() override;
 
   // After connecting, send the handshake to the server to make sure
   // we are communicating with it.
   bool HandshakeWithServer(Status *error_ptr);
 
   // For packets which specify a range of output to be returned,

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

 llvm::raw_ostream &process_gdb_remote::operator<<(llvm::raw_ostream &os,
                                                   const QOffsets &offsets) {
   return os << llvm::formatv(
              "QOffsets({0}, [{1:@[x]}])", offsets.segments,
              llvm::make_range(offsets.offsets.begin(), offsets.offsets.end()));
 }
 
 // GDBRemoteCommunicationClient constructor
-GDBRemoteCommunicationClient::GDBRemoteCommunicationClient()
-    : GDBRemoteClientBase("gdb-remote.client", "gdb-remote.client.rx_packet"),
+GDBRemoteCommunicationClient::GDBRemoteCommunicationClient(
+    std::chrono::nanoseconds delay_multiplier)
+    : GDBRemoteClientBase("gdb-remote.client", "gdb-remote.client.rx_packet",
+                          delay_multiplier),
       m_supports_not_sending_acks(eLazyBoolCalculate),
       m_supports_thread_suffix(eLazyBoolCalculate),
       m_supports_threads_in_stop_reply(eLazyBoolCalculate),
       m_supports_vCont_all(eLazyBoolCalculate),
       m_supports_vCont_any(eLazyBoolCalculate),
       m_supports_vCont_c(eLazyBoolCalculate),
       m_supports_vCont_C(eLazyBoolCalculate),
       m_supports_vCont_s(eLazyBoolCalculate),

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

 //===-- ProcessGDBRemote.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/Host/Config.h"
 
 #include <errno.h>
 #include <stdlib.h>
 #if LLDB_ENABLE_POSIX
 #include <netinet/in.h>
 #include <sys/mman.h>
 #include <sys/socket.h>
 #include <unistd.h>
return m_collection_sp->GetPropertyAtIndexAsBoolean(
         nullptr, idx,
         g_processgdbremote_properties[idx].default_uint_value != 0);
   }
 
   bool GetUseGPacketForReading() const {
     const uint32_t idx = ePropertyUseGPacketForReading;
     return m_collection_sp->GetPropertyAtIndexAsBoolean(nullptr, idx, true);
   }
+
+  uint64_t GetPacketDelayMultiplier() const {
+    const uint32_t idx = ePropertyPacketDelayMultiplier;
+    return m_collection_sp->GetPropertyAtIndexAsUInt64(
+        nullptr, idx, g_processgdbremote_properties[idx].default_uint_value);
+  }
 };
 
 typedef std::shared_ptr<PluginProperties> ProcessKDPPropertiesSP;
 
 static const ProcessKDPPropertiesSP &GetGlobalPluginProperties() {
   static ProcessKDPPropertiesSP g_settings_sp;
   if (!g_settings_sp)
     g_settings_sp = std::make_shared<PluginProperties>();
bool ProcessGDBRemote::CanDebug(lldb::TargetSP target_sp,
   // be preparing to attach.
   return true;
 }
 
 // ProcessGDBRemote constructor
 ProcessGDBRemote::ProcessGDBRemote(lldb::TargetSP target_sp,
                                    ListenerSP listener_sp)
     : Process(target_sp, listener_sp),
+      m_gdb_comm(std::chrono::nanoseconds(
+          GetGlobalPluginProperties()->GetPacketDelayMultiplier())),
       m_debugserver_pid(LLDB_INVALID_PROCESS_ID), m_last_stop_packet_mutex(),
       m_register_info_sp(nullptr),
       m_async_broadcaster(nullptr, "lldb.process.gdb-remote.async-broadcaster"),
       m_async_listener_sp(
           Listener::MakeListener("lldb.process.gdb-remote.async-listener")),
       m_async_thread_state_mutex(), m_thread_ids(), m_thread_pcs(),
       m_jstopinfo_sp(), m_jthreadsinfo_sp(), m_continue_c_tids(),
       m_continue_C_tids(), m_continue_s_tids(), m_continue_S_tids(),

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemoteProperties.td

let Definition = "processgdbremote" in {
   def UseSVR4: Property<"use-libraries-svr4", "Boolean">,
     Global,
     DefaultTrue,
     Desc<"If true, the libraries-svr4 feature will be used to get a hold of the process's loaded modules. This setting is only effective if lldb was build with xml support.">;
   def UseGPacketForReading: Property<"use-g-packet-for-reading", "Boolean">,
     Global,
     DefaultFalse,
     Desc<"Specify if the server should use 'g' packets to read registers.">;
+  def PacketDelayMultiplier: Property<"packet-delay-multiplier", "UInt64">,
+    Global,
+    DefaultUnsignedValue<0>,
+    Desc<"If greater than 0, sleep for packet-delay-multiplier * packet size nanoseconds for every packet sent or received.">;
 }