[lldb] [Process] Introduce protocol extension support API

[mgorny]

Summary

Introduce a NativeProcessProtocol API for indicating support for
protocol extensions and enabling them. LLGS calls
GetSupportedExtensions() method on the process factory to determine
which extensions are supported by the plugin. If the future is both
supported by the plugin and reported as supported by the client, LLGS
enables it and reports to the client as supported by the server.

The extension is enabled on the process instance by calling
SetEnabledExtensions() method. This is done after qSupported exchange
(if the debugger is attached to any process), as well as after launching
or attaching to a new inferior.

The patch adds 'fork' extension corresponding to 'fork-events+'
qSupported feature and 'vfork' extension for 'vfork-events+'.

[mgorny]

Note that I've included full fork-events+ and vfork-events+ as demo of the API. I can split them further and/or move to more relevant commits as I progress with the split.

[mgorny]

Add to _KNOWN_QSUPPORTED_STUB_FEATURES and clang-format.

[mgorny]

Fix uninitialized flags variable in GDBRemoteCommunicationServerLLGS::SetEnabledExtensions().

[labath]

I like that (particularly the dependency graph :P). Let's just figure out the error handling..

In D100153#2678223, @mgorny wrote:

Note that I've included full fork-events+ and vfork-events+ as demo of the API. I can split them further and/or move to more relevant commits as I progress with the split.

Maybe we could start by feature-ifying one of the existing fields currently controlled by ifdefs. Say, QPassSignals+. It does not have a client component, so it won't cover everything, but I think it will give us something.

lldb/include/lldb/Host/common/NativeProcessProtocol.h

389

Are you sure that returning success is the best "default" behavior? Maybe the base implementation should always return an error (as it does not implement any extensions)? Or return success, only if one enables an empty set of extensions?

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

3592–3595

... or actually, I am wondering if this should not be a hard error/assertion. In the current set up, I think it would be a programmer error if the factory reports an extension as supported, but then the instance fails to enable it...

[mgorny]

lldb/include/lldb/Host/common/NativeProcessProtocol.h

389

I'm not sure whether we need error handling here at all.

The current impl doesn't do anything but setting an instance var here. The original impl controlled events to PTRACE_SETOPTIONS but in the end I've decided to enable tracing fork/vfork unconditionally, and just using extensions to decide whether to handle it locally or defer to client.

I suppose it might make sense to review other patches first and get back to this one once we're sure what we need.

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

3592–3595

Technically, the original idea was that this would fail if ptrace() failed to set options. But as I said, this is no longer the case, so I'm not even sure if we need any error reporting here.

[mgorny]

Rebase + clang-format.

[labath]

lldb/include/lldb/Host/common/NativeProcessProtocol.h

389

I think I agree with you. Let's just drop error handling altogether. Maybe add something like assert(features && ~m_process_factory.GetSupportedExtensions() == {}) to GDBRemoteCommunicationServerLLGS::SetEnabledExtensions.

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

3562–3567

Maybe drop the == Extension::[v]fork part?

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

102–103

I am wondering if this should just be NativeProcessProtocol::Extension m_enabled_extensions; Can we think of an extension that would belong to NativeProcessProtocol::Extension, but we would not want to store its state in the GDBRemoteCommunicationServerLLGS class?

[mgorny]

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

3562–3567

Can't do that, enum class doesn't convert to bool. In fact, I tried a few more or less crazy ideas to make this work, and none worked ;-).

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

102–103

I can't think of any right now and I suppose even if there were one, there would probably be no big loss in using Extension here.

[mgorny]

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

3562–3567

The next best thing I was able to do is check != NativeProcessProtocol::Extension() which is a little bit shorter. Maybe I could try adding operator== and != against, say, nullptr or some special constant?

[labath]

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

3562–3567

An explicit bool cast works though, and is less fancy. Maybe if we combine this the m_enabled_extensions idea, we could do something like:

using Extension = NativeProcessProtocol::Extension;
Extension client_extensions{};
for (StringRef x : client_features)
  client_extensions |= llvm::StringSwitch<Extension>(x).Case("fork-events+", Extension::fork).Case("vfork-events+", Extension::vfork).Default({});
m_enabled_extensions = client_extensions & m_process_factory.GetSupportedExtensions();
if (bool(m_enabled_extensions & Extension::fork))
  ret.push_back("fork-events+");
if (bool(m_enabled_extensions & Extension::vfork))
  ret.push_back("vfork-events+");

[mgorny]

Implemented the suggested changes, including bool() use in SetEnabledExtensions(). Removed the client announcements of fork-events and vfork-events support.

I've left the server parts of fork-events and vfork-events. It won't trigger until both the process plugin and the client indicates support too.

[mgorny]

Added Extension bit for multiprocess. Made fork-events and vfork-events both depend on it — i.e. be implicitly disabled if multiprocess wasn't reported as supported.

Simplify GDBRemoteCommunicationServerLLGS::SetEnabledExtensions() — we can just take the value instead of copying it bit-by-bit ;-).

Move add_qSupported_packets() change from later patch. Add tests for qSupported responses (particularly, rejecting [v]fork-events if multiprocess is not present). The new tests are in fork category that's skipped on all platforms where fork/vfork isn't enabled yet (i.e. everywhere in this patch).

lldb/include/lldb/Host/common/NativeProcessProtocol.h

 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include <mutex>
 #include <unordered_map>
 #include <vector>
 
 namespace lldb_private {
+LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE();
+
 class MemoryRegionInfo;
 class ResumeActionList;
 
 struct SVR4LibraryInfo {
   std::string name;
   lldb::addr_t link_map;
   lldb::addr_t base_addr;
   lldb::addr_t ld_addr;
public:
   bool UnregisterNativeDelegate(NativeDelegate &native_delegate);
 
   virtual Status GetLoadedModuleFileSpec(const char *module_path,
                                          FileSpec &file_spec) = 0;
 
   virtual Status GetFileLoadAddress(const llvm::StringRef &file_name,
                                     lldb::addr_t &load_addr) = 0;
 
+  /// Extension flag constants, passed to SetEnabledExtension().
+  enum class Extension {
+    fork = (1u << 0),
+    vfork = (1u << 1),
+
+    LLVM_MARK_AS_BITMASK_ENUM(vfork)
+  };
+
   class Factory {
   public:
     virtual ~Factory();
     /// Launch a process for debugging.
     ///
     /// \param[in] launch_info
     ///     Information required to launch the process.
     ///
public:
     ///     instance.
     ///
     /// \return
     ///     A NativeProcessProtocol shared pointer if the operation succeeded or
     ///     an error object if it failed.
     virtual llvm::Expected<std::unique_ptr<NativeProcessProtocol>>
     Attach(lldb::pid_t pid, NativeDelegate &native_delegate,
            MainLoop &mainloop) const = 0;
+
+    /// Get the bitmask of extensions supported by this process plugin.
+    ///
+    /// \return
+    ///     A NativeProcessProtocol::Extension bitmask.
+    virtual Extension GetSupportedExtensions() const {
+      return {};
+    }
   };
 
   /// Start tracing a process or its threads.
   ///
   /// \param[in] json_params
   ///     JSON object with the information of what and how to trace.
   ///     In the case of gdb-remote, this object should conform to the
   ///     jLLDBTraceStart packet.
return llvm::createStringError(
         request.kind.c_str(), request.type.c_str());
   }
 
   /// \copydoc Process::TraceSupported()
   virtual llvm::Expected<TraceSupportedResponse> TraceSupported() {
     return llvm::make_error<UnimplementedError>();
   }
 
+  /// Method called in order to propagate the bitmap of protocol
+  /// extensions supported by the client.
+  ///
+  /// \param[in] flags
+  ///     The bitmap of enabled extensions.
+  ///
+  /// \return An error if extension-related setup failed, success
+  /// otherwise.
+  virtual llvm::Error SetEnabledExtensions(Extension flags) {
+    m_enabled_extensions = flags;
+    return llvm::Error::success();
+  }
+
 protected:
   struct SoftwareBreakpoint {
     uint32_t ref_count;
     llvm::SmallVector<uint8_t, 4> saved_opcodes;
     llvm::ArrayRef<uint8_t> breakpoint_opcodes;
   };
 
   std::unordered_map<lldb::addr_t, SoftwareBreakpoint> m_software_breakpoints;
protected:
   HardwareBreakpointMap m_hw_breakpoints_map;
   int m_terminal_fd;
   uint32_t m_stop_id = 0;
 
   // Set of signal numbers that LLDB directly injects back to inferior without
   // stopping it.
   llvm::DenseSet<int> m_signals_to_ignore;
 
+  // Extensions enabled per the last SetEnabledExtensions() call.
+  Extension m_enabled_extensions;
+
   // lldb_private::Host calls should be used to launch a process for debugging,
   // and then the process should be attached to. When attaching to a process
   // lldb_private::Host calls should be used to locate the process to attach
   // to, and then this function should be called.
   NativeProcessProtocol(lldb::pid_t pid, int terminal_fd,
                         NativeDelegate &delegate);
 
   void SetID(lldb::pid_t pid) { m_pid = pid; }

lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py

_KNOWN_QSUPPORTED_STUB_FEATURES = [
         "QListThreadsInStopReply",
         "qXfer:auxv:read",
         "qXfer:libraries:read",
         "qXfer:libraries-svr4:read",
         "qXfer:features:read",
         "qEcho",
         "QPassSignals",
         "multiprocess",
+        "fork-events",
+        "vfork-events",
     ]
 
     def parse_qSupported_response(self, context):
         self.assertIsNotNone(context)
 
         raw_response = context.get("qSupported_response")
         self.assertIsNotNone(raw_response)
 

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

void GDBRemoteCommunicationClient::GetRemoteQSupported() {
   m_supports_qEcho = eLazyBoolNo;
   m_supports_QPassSignals = eLazyBoolNo;
 
   m_max_packet_size = UINT64_MAX; // It's supposed to always be there, but if
                                   // not, we assume no limit
 
   // build the qSupported packet
   std::vector<std::string> features = {"xmlRegisters=i386,arm,mips,arc",
-                                       "multiprocess+"};
+                                       "multiprocess+",
+                                       "fork-events+",
+                                       "vfork-events+"};
   StreamString packet;
   packet.PutCString("qSupported");
   for (uint32_t i = 0; i < features.size(); ++i) {
     packet.PutCString(i == 0 ? ":" : ";");
     packet.PutCString(features[i]);
   }
 
   StringExtractorGDBRemote response;

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:
 
   lldb::StateType m_inferior_prev_state = lldb::StateType::eStateInvalid;
   llvm::StringMap<std::unique_ptr<llvm::MemoryBuffer>> m_xfer_buffer_map;
   std::mutex m_saved_registers_mutex;
   std::unordered_map<uint32_t, lldb::DataBufferSP> m_saved_registers_map;
   uint32_t m_next_saved_registers_id = 1;
   bool m_handshake_completed = false;
 
+  bool m_fork_events_supported = false;
+  bool m_vfork_events_supported = false;
+
   PacketResult SendONotification(const char *buffer, uint32_t len);
 
   PacketResult SendWResponse(NativeProcessProtocol *process);
 
   PacketResult SendStopReplyPacketForThread(lldb::tid_t tid);
 
   PacketResult SendStopReasonForState(lldb::StateType process_state);
 
private:
   // Otherwise, returns the error.
   //
   // If allow_all is true, then the pid/tid value of -1 ('all') will be allowed.
   // In any case, the function assumes that exactly one inferior is being
   // debugged and rejects pid values that do no match that inferior.
   llvm::Expected<lldb::tid_t> ReadTid(StringExtractorGDBRemote &packet,
                                       bool allow_all = false);
 
+  // Call SetEnabledExtensions() with appropriate flags on the process.
+  void SetEnabledExtensions(NativeProcessProtocol& process);
+
   // For GDBRemoteCommunicationServerLLGS only
   GDBRemoteCommunicationServerLLGS(const GDBRemoteCommunicationServerLLGS &) =
       delete;
   const GDBRemoteCommunicationServerLLGS &
   operator=(const GDBRemoteCommunicationServerLLGS &) = delete;
 };
 
 } // namespace process_gdb_remote
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_PROCESS_GDB_REMOTE_GDBREMOTECOMMUNICATIONSERVERLLGS_H

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"
assert(!m_debugged_process_up && "lldb-server creating debugged "
                                      "process but one already exists");
     auto process_or =
         m_process_factory.Launch(m_process_launch_info, *this, m_mainloop);
     if (!process_or)
       return Status(process_or.takeError());
     m_debugged_process_up = std::move(*process_or);
   }
 
+  SetEnabledExtensions(*m_debugged_process_up);
+
   // Handle mirroring of inferior stdout/stderr over the gdb-remote protocol as
   // needed. llgs local-process debugging may specify PTY paths, which will
   // make these file actions non-null process launch -i/e/o will also make
   // these file actions non-null nullptr means that the traffic is expected to
   // flow over gdb-remote protocol
   if (should_forward_stdio) {
     // nullptr means it's not redirected to file or pty (in case of LLGS local)
     // at least one of stdio will be transferred pty<->gdb-remote we need to
Status GDBRemoteCommunicationServerLLGS::AttachToProcess(lldb::pid_t pid) {
   auto process_or = m_process_factory.Attach(pid, *this, m_mainloop);
   if (!process_or) {
     Status status(process_or.takeError());
     llvm::errs() << llvm::formatv("failed to attach to process {0}: {1}", pid,
                                   status);
     return status;
   }
   m_debugged_process_up = std::move(*process_or);
+  SetEnabledExtensions(*m_debugged_process_up);
 
   // Setup stdout/stderr mapping from inferior.
   auto terminal_fd = m_debugged_process_up->GetTerminalFileDescriptor();
   if (terminal_fd >= 0) {
     LLDB_LOGF(log,
               "ProcessGDBRemoteCommunicationServerLLGS::%s setting "
               "inferior STDIO fd to %d",
               __FUNCTION__, terminal_fd);
std::vector<std::string> GDBRemoteCommunicationServerLLGS::HandleFeatures(
   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
   });
+
+  // check for platform features
+  auto process_features = m_process_factory.GetSupportedExtensions();
+
+  // reset to defaults
+  m_fork_events_supported = false;
+  m_vfork_events_supported = false;
+
+  // check for client features
+  for (auto x : client_features) {
+    if (x == "fork-events+" &&
+        (process_features & NativeProcessProtocol::Extension::fork) ==
+            NativeProcessProtocol::Extension::fork)
+      m_fork_events_supported = true;
+    else if (x == "vfork-events+" &&
+             (process_features & NativeProcessProtocol::Extension::vfork) ==
+                 NativeProcessProtocol::Extension::vfork)
+      m_vfork_events_supported = true;
+  }
+
+  // report only if actually supported
+  if (m_fork_events_supported)
+    ret.push_back("fork-events+");
+  if (m_vfork_events_supported)
+    ret.push_back("vfork-events+");
+
+  if (m_debugged_process_up)
+    SetEnabledExtensions(*m_debugged_process_up);
   return ret;
 }
+
+void GDBRemoteCommunicationServerLLGS::SetEnabledExtensions(
+    NativeProcessProtocol &process) {
+  Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS));
+
+  NativeProcessProtocol::Extension flags = {};
+  if (m_fork_events_supported)
+    flags |= NativeProcessProtocol::Extension::fork;
+  if (m_vfork_events_supported)
+    flags |= NativeProcessProtocol::Extension::vfork;
+
+  llvm::Error error = process.SetEnabledExtensions(flags);
+  if (error)
+    LLDB_LOG_ERROR(log, std::move(error),
+                   "Enabling protocol extensions failed: {0}");
+}