[lldb] [gdb-remote] Clean up the API of GDBRemoteCommunication

[mgorny]

Summary

Make the public & protected fields and methods of GDBRemoteCommunication
private if they are not used elsewhere. This also includes the GetAck()
method that was used in unittests but did not seem essential to the test
at hand.

Remove the unused m_is_platform field.

Make ExpandRLE() a static function rather than static class method.

This is a first step towards separating the low-level packet operations
from high-level public API that is intended to be used by
ProcessGDBRemote.

Sponsored by: The FreeBSD Foundation

[mgorny]

Actually, this makes little sense without un-publicizing the inherited methods first.

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

private:
     // and won't need to restore it.
     bool m_timeout_modified;
   };
 
   GDBRemoteCommunication(const char *comm_name, const char *listener_name);
 
   ~GDBRemoteCommunication() override;
 
-  PacketResult GetAck();
-
   size_t SendAck();
 
-  size_t SendNack();
-
-  char CalculcateChecksum(llvm::StringRef payload);
-
-  PacketType CheckForPacket(const uint8_t *src, size_t src_len,
-                            StringExtractorGDBRemote &packet);
-
-  bool GetSendAcks() { return m_send_acks; }
-
   // Set the global packet timeout.
   //
   // For clients, this is the timeout that gets used when sending
   // packets and waiting for responses. For servers, this is used when waiting
   // for ACKs.
   std::chrono::seconds SetPacketTimeout(std::chrono::seconds packet_timeout) {
     const auto old_packet_timeout = m_packet_timeout;
     m_packet_timeout = packet_timeout;
public:
 
   void DumpHistory(Stream &strm);
 
   void SetPacketRecorder(repro::PacketRecorder *recorder);
 
   static llvm::Error ConnectLocally(GDBRemoteCommunication &client,
                                     GDBRemoteCommunication &server);
 
-  /// Expand GDB run-length encoding.
-  static std::string ExpandRLE(std::string);
-
 protected:
-  std::chrono::seconds m_packet_timeout;
-  uint32_t m_echo_number;
   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;
 
   PacketResult SendPacketNoLock(llvm::StringRef payload);
   PacketResult SendNotificationPacketNoLock(llvm::StringRef notify_type,
                                             std::deque<std::string>& queue,
                                             llvm::StringRef payload);
-  PacketResult SendRawPacketNoLock(llvm::StringRef payload,
-                                   bool skip_ack = false);
 
   PacketResult ReadPacket(StringExtractorGDBRemote &response,
                           Timeout<std::micro> timeout, bool sync_on_timeout);
 
   PacketResult ReadPacketWithOutputSupport(
       StringExtractorGDBRemote &response, Timeout<std::micro> timeout,
       bool sync_on_timeout,
       llvm::function_ref<void(llvm::StringRef)> output_callback);
 
+  Status StartListenThread(const char *hostname = "127.0.0.1",
+                           uint16_t port = 0);
+
+private:
+  std::chrono::seconds m_packet_timeout;
+  uint32_t m_echo_number;
+  GDBRemoteCommunicationHistory m_history;
+
+  // Promise used to grab the port number from listening thread
+  std::promise<uint16_t> m_port_promise;
+
+  HostThread m_listen_thread;
+  std::string m_listen_url;
+
+#if defined(HAVE_LIBCOMPRESSION)
+  CompressionType m_decompression_scratch_type = CompressionType::None;
+  void *m_decompression_scratch = nullptr;
+#endif
+
+  GDBRemoteCommunication(const GDBRemoteCommunication &) = delete;
+  const GDBRemoteCommunication &
+  operator=(const GDBRemoteCommunication &) = delete;
+
+  PacketResult GetAck();
+
+  size_t SendNack();
+
+  char CalculcateChecksum(llvm::StringRef payload);
+
+  PacketType CheckForPacket(const uint8_t *src, size_t src_len,
+                            StringExtractorGDBRemote &packet);
+
+  bool GetSendAcks() { return m_send_acks; }
+
+  PacketResult SendRawPacketNoLock(llvm::StringRef payload,
+                                   bool skip_ack = false);
+
   PacketResult WaitForPacketNoLock(StringExtractorGDBRemote &response,
                                    Timeout<std::micro> timeout,
                                    bool sync_on_timeout);
 
   bool CompressionIsEnabled() {
     return m_compression_type != CompressionType::None;
   }
 
   // If compression is enabled, decompress the packet in m_bytes and update
   // m_bytes with the uncompressed version.
   // Returns 'true' packet was decompressed and m_bytes is the now-decompressed
   // text.
   // Returns 'false' if unable to decompress or if the checksum was invalid.
   //
   // NB: Once the packet has been decompressed, checksum cannot be computed
   // based
   // on m_bytes.  The checksum was for the compressed packet.
   bool DecompressPacket();
 
-  Status StartListenThread(const char *hostname = "127.0.0.1",
-                           uint16_t port = 0);
-
   bool JoinListenThread();
 
   lldb::thread_result_t ListenThread();
-
-private:
-  // Promise used to grab the port number from listening thread
-  std::promise<uint16_t> m_port_promise;
-
-  HostThread m_listen_thread;
-  std::string m_listen_url;
-
-#if defined(HAVE_LIBCOMPRESSION)
-  CompressionType m_decompression_scratch_type = CompressionType::None;
-  void *m_decompression_scratch = nullptr;
-#endif
-
-  GDBRemoteCommunication(const GDBRemoteCommunication &) = delete;
-  const GDBRemoteCommunication &
-  operator=(const GDBRemoteCommunication &) = delete;
 };
 
 } // namespace process_gdb_remote
 } // namespace lldb_private
 
 namespace llvm {
 template <>
 struct format_provider<
     lldb_private::process_gdb_remote::GDBRemoteCommunication::PacketResult> {
   static void format(const lldb_private::process_gdb_remote::
                          GDBRemoteCommunication::PacketResult &state,
                      raw_ostream &Stream, StringRef Style);
 };
 } // namespace llvm
 
 #endif // LLDB_SOURCE_PLUGINS_PROCESS_GDB_REMOTE_GDBREMOTECOMMUNICATION_H

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

 
 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)
-    : Communication(comm_name),
+    : Communication(comm_name), m_supports_qEcho(eLazyBoolCalculate),
+      m_send_acks(true), m_compression_type(CompressionType::None),
 #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_is_platform(false),
-      m_compression_type(CompressionType::None), m_listen_url() {
+      m_echo_number(0), m_history(512), m_listen_url() {
 }
 
 // Destructor
 GDBRemoteCommunication::~GDBRemoteCommunication() {
   if (IsConnected()) {
     Disconnect();
   }
 
#endif
 
   m_bytes.replace(0, size_of_first_packet, new_packet.data(),
                   new_packet.size());
 
   free(decompressed_buffer);
   return true;
 }
 
+static std::string ExpandRLE(std::string packet) {
+  // Reserve enough byte for the most common case (no RLE used).
+  std::string decoded;
+  decoded.reserve(packet.size());
+  for (std::string::const_iterator c = packet.begin(); c != packet.end(); ++c) {
+    if (*c == '*') {
+      // '*' indicates RLE. Next character will give us the repeat count and
+      // previous character is what is to be repeated.
+      char char_to_repeat = decoded.back();
+      // Number of time the previous character is repeated.
+      int repeat_count = *++c + 3 - ' ';
+      // We have the char_to_repeat and repeat_count. Now push it in the
+      // packet.
+      for (int i = 0; i < repeat_count; ++i)
+        decoded.push_back(char_to_repeat);
+    } else if (*c == 0x7d) {
+      // 0x7d is the escape character.  The next character is to be XOR'd with
+      // 0x20.
+      char escapee = *++c ^ 0x20;
+      decoded.push_back(escapee);
+    } else {
+      decoded.push_back(*c);
+    }
+  }
+  return decoded;
+}
+
 GDBRemoteCommunication::PacketType
 GDBRemoteCommunication::CheckForPacket(const uint8_t *src, size_t src_len,
                                        StringExtractorGDBRemote &packet) {
   // Put the packet data into the buffer in a thread safe fashion
   std::lock_guard<std::recursive_mutex> guard(m_bytes_mutex);
 
   Log *log = GetLog(GDBRLog::Packets);
 
void llvm::format_provider<GDBRemoteCommunication::PacketResult>::format(
   case PacketResult::ErrorDisconnected:
     Stream << "ErrorDisconnected";
     break;
   case PacketResult::ErrorNoSequenceLock:
     Stream << "ErrorNoSequenceLock";
     break;
   }
 }
-
-std::string GDBRemoteCommunication::ExpandRLE(std::string packet) {
-  // Reserve enough byte for the most common case (no RLE used).
-  std::string decoded;
-  decoded.reserve(packet.size());
-  for (std::string::const_iterator c = packet.begin(); c != packet.end(); ++c) {
-    if (*c == '*') {
-      // '*' indicates RLE. Next character will give us the repeat count and
-      // previous character is what is to be repeated.
-      char char_to_repeat = decoded.back();
-      // Number of time the previous character is repeated.
-      int repeat_count = *++c + 3 - ' ';
-      // We have the char_to_repeat and repeat_count. Now push it in the
-      // packet.
-      for (int i = 0; i < repeat_count; ++i)
-        decoded.push_back(char_to_repeat);
-    } else if (*c == 0x7d) {
-      // 0x7d is the escape character.  The next character is to be XOR'd with
-      // 0x20.
-      char escapee = *++c ^ 0x20;
-      decoded.push_back(escapee);
-    } else {
-      decoded.push_back(*c);
-    }
-  }
-  return decoded;
-}

lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp

static constexpr TestCase Tests[] = {
       {{"-$#00"}, {""}},            // Spurious NAK
   };
   for (const auto &Test : Tests) {
     SCOPED_TRACE(Test.Packet + " -> " + Test.Payload);
     StringExtractorGDBRemote response;
     ASSERT_TRUE(Write(Test.Packet));
     ASSERT_EQ(PacketResult::Success, client.ReadPacket(response));
     ASSERT_EQ(Test.Payload, response.GetStringRef());
-    ASSERT_EQ(PacketResult::Success, server.GetAck());
   }
 }