[lldb] [gdb-remote server] Refactor handling qSupported

[mgorny]

Summary

Refactor handling qSupported to use a virtual HandleFeatures() method.
The client-provided features are split into an array and passed
to the method. The method returns an array of server features that are
concatenated into the qSupported response to the server.

The base implementation of HandleFeatures()
in GDBRemoteCommunicationServerCommon now includes only flags common
to both platform server and llgs, while llgs-specific flags are inserted
in GDBRemoteCommunicationServerLLGS.

[mgorny]

This is split from D99864 as a minimal independent change.

[rovka]

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

843

This seems to have disappeared, shouldn't it be in GDBRemoteCommunicationServerCommon::HandleFeatures?

[mgorny]

Fix missing QListThreadsInStopReply+.

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

843

My mistake, sorry. Fixed now.

[mgorny]

clang-format

[labath]

cool

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

835

remove variable used only once?

1310–1311

I think that these two should also be llgs-specific.

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

151

this const is useless

[mgorny]

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

1310–1311

I actually left them here because the respective Handle_QThreadSuffixSupported and Handle_QListThreadsInStopReply methods are also in common.

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

151

Yeah, I probably meant to do const ...& but I guess it doesn't matter much for ArrayRef.

[labath]

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

1310–1311

Fair enough. I'll see what I can do about that once this patch lands.

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

 //===-- GDBRemoteCommunicationServerCommon.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_SOURCE_PLUGINS_PROCESS_GDB_REMOTE_GDBREMOTECOMMUNICATIONSERVERCOMMON_H
 #define LLDB_SOURCE_PLUGINS_PROCESS_GDB_REMOTE_GDBREMOTECOMMUNICATIONSERVERCOMMON_H
 
 #include <string>
 
 #include "lldb/Host/ProcessLaunchInfo.h"
 #include "lldb/lldb-private-forward.h"
 
 #include "GDBRemoteCommunicationServer.h"
 #include "GDBRemoteCommunicationServerCommon.h"
 
 class StringExtractorGDBRemote;
 
 namespace lldb_private {
protected:
   /// \return
   ///     An Status object indicating the success or failure of the
   ///     launch.
   virtual Status LaunchProcess() = 0;
 
   virtual FileSpec FindModuleFile(const std::string &module_path,
                                   const ArchSpec &arch);
 
+  // Process client_features (qSupported) and return an array of server features
+  // to be returned in response.
+  virtual std::vector<std::string>
+  HandleFeatures(const llvm::ArrayRef<llvm::StringRef> client_features);
+
 private:
   ModuleSpec GetModuleInfo(llvm::StringRef module_path, llvm::StringRef triple);
 };
 
 } // namespace process_gdb_remote
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_PROCESS_GDB_REMOTE_GDBREMOTECOMMUNICATIONSERVERCOMMON_H

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

if (packet.GetChar() == ',') {
     return SendPacketNoLock(response.GetString());
   }
   return SendErrorResponse(19);
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerCommon::Handle_qSupported(
     StringExtractorGDBRemote &packet) {
-  StreamGDBRemote response;
-
-  // Features common to lldb-platform and llgs.
-  uint32_t max_packet_size = 128 * 1024; // 128KBytes is a reasonable max packet
-                                         // size--debugger can always use less
-  response.Printf("PacketSize=%x", max_packet_size);
-
-  response.PutCString(";QStartNoAckMode+");
-  response.PutCString(";QThreadSuffixSupported+");
-  response.PutCString(";QListThreadsInStopReply+");
-  response.PutCString(";qEcho+");
-  response.PutCString(";qXfer:features:read+");
-#if defined(__linux__) || defined(__NetBSD__) || defined(__FreeBSD__)
-  response.PutCString(";QPassSignals+");
-  response.PutCString(";qXfer:auxv:read+");
-  response.PutCString(";qXfer:libraries-svr4:read+");
-#endif
-  response.PutCString(";multiprocess+");
-
-  return SendPacketNoLock(response.GetString());
+  // Parse client-indicated features.
+  llvm::StringRef view = packet.GetStringRef();
+  llvm::SmallVector<llvm::StringRef, 4> client_features;
+  view.split(client_features, ';');
+  return SendPacketNoLock(llvm::join(HandleFeatures(client_features), ";"));
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerCommon::Handle_QThreadSuffixSupported(
     StringExtractorGDBRemote &packet) {
   m_thread_suffix_supported = true;
   return SendOKResponse();
 }
if (!ObjectFile::GetModuleSpecifications(module_path_spec, 0, 0,
     return ModuleSpec();
 
   ModuleSpec matched_module_spec;
   if (!module_specs.FindMatchingModuleSpec(module_spec, matched_module_spec))
     return ModuleSpec();
 
   return matched_module_spec;
 }
+
+std::vector<std::string> GDBRemoteCommunicationServerCommon::HandleFeatures(
+    const llvm::ArrayRef<llvm::StringRef> client_features) {
+  // 128KBytes is a reasonable max packet size--debugger can always use less.
+  constexpr uint32_t max_packet_size = 128 * 1024;
+
+  // Features common to platform server and llgs.
+  return {
+      llvm::formatv("PacketSize={0}", max_packet_size),
+      "QStartNoAckMode+",
+      "QThreadSuffixSupported+",
+      "QListThreadsInStopReply+",
+      "qEcho+",
+  };
+}

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

 //===-- GDBRemoteCommunicationServerLLGS.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_SOURCE_PLUGINS_PROCESS_GDB_REMOTE_GDBREMOTECOMMUNICATIONSERVERLLGS_H
 #define LLDB_SOURCE_PLUGINS_PROCESS_GDB_REMOTE_GDBREMOTECOMMUNICATIONSERVERLLGS_H
 
 #include <mutex>
 #include <unordered_map>
 
 #include "lldb/Core/Communication.h"
 #include "lldb/Host/MainLoop.h"
 #include "lldb/Host/common/NativeProcessProtocol.h"
 #include "lldb/lldb-private-forward.h"
 
 #include "GDBRemoteCommunicationServerCommon.h"
 
 class StringExtractorGDBRemote;
 
protected:
   FileSpec FindModuleFile(const std::string &module_path,
                           const ArchSpec &arch) override;
 
   llvm::Expected<std::unique_ptr<llvm::MemoryBuffer>>
   ReadXferObject(llvm::StringRef object, llvm::StringRef annex);
 
   static std::string XMLEncodeAttributeValue(llvm::StringRef value);
 
+  virtual std::vector<std::string>
+  HandleFeatures(const llvm::ArrayRef<llvm::StringRef> client_features) override;
+
 private:
   llvm::Expected<std::unique_ptr<llvm::MemoryBuffer>> BuildTargetXml();
 
   void HandleInferiorState_Exited(NativeProcessProtocol *process);
 
   void HandleInferiorState_Stopped(NativeProcessProtocol *process);
 
   NativeThreadProtocol *GetThreadFromSuffix(StringExtractorGDBRemote &packet);

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

 //===-- GDBRemoteCommunicationServerLLGS.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 <errno.h>
 
 #include "lldb/Host/Config.h"
 
 
 #include <chrono>
 #include <cstring>
 #include <thread>
 
 #include "GDBRemoteCommunicationServerLLGS.h"
 #include "lldb/Host/ConnectionFileDescriptor.h"
GDBRemoteCommunicationServerLLGS::ReadTid(StringExtractorGDBRemote &packet,
   if (pid != StringExtractorGDBRemote::AllProcesses) {
     if (pid != m_debugged_process_up->GetID())
       return llvm::make_error<StringError>(
           inconvertibleErrorCode(), llvm::formatv("PID {0} not debugged", pid));
   }
 
   return tid;
 }
+
+std::vector<std::string> GDBRemoteCommunicationServerLLGS::HandleFeatures(
+    const llvm::ArrayRef<llvm::StringRef> client_features) {
+  auto ret =
+      GDBRemoteCommunicationServerCommon::HandleFeatures(client_features);
+  ret.insert(ret.end(), {
+    "qXfer:features:read+", "multiprocess+",
+#if defined(__linux__) || defined(__NetBSD__) || defined(__FreeBSD__)
+        "QPassSignals+", "qXfer:auxv:read+", "qXfer:libraries-svr4:read+",
+#endif
+  });
+  return ret;
+}