[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

359

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

3612–3615

... 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

359

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

3612–3615

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

359

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

3583–3588

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

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

106–107

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

3583–3588

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

106–107

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

3583–3588

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

3583–3588

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:
   };
 
   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, returned by Factory::GetSupportedExtensions()
+  /// and passed to SetEnabledExtension()
+  enum class Extension {
+    multiprocess = (1u << 0),
+    fork = (1u << 1),
+    vfork = (1u << 2),
+
+    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.
+  virtual void SetEnabledExtensions(Extension flags) {
+    m_enabled_extensions = flags;
+  }
+
 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;
   lldb::pid_t m_pid;
 
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/dotest.py

if lldbplatformutil.platformIsDarwin():
                 print(skip_msg%"debugserver");
     else:
         configuration.skip_categories.append("debugserver")
         if lldb.remote_platform and lldbplatformutil.getPlatform() == "windows":
             configuration.skip_categories.append("llgs")
             if configuration.verbose:
                 print(skip_msg%"lldb-server");
 
+
+def checkForkVForkSupport():
+    from lldbsuite.test import lldbplatformutil
+
+    platform = lldbplatformutil.getPlatform()
+    if platform not in []:
+        configuration.skip_categories.append("fork")
+
+
 def run_suite():
     # On MacOS X, check to make sure that domain for com.apple.DebugSymbols defaults
     # does not exist before proceeding to running the test suite.
     if sys.platform.startswith("darwin"):
         checkDsymForUUIDIsNotOn()
 
     # Start the actions by first parsing the options while setting up the test
     # directories, followed by setting up the search paths for lldb utilities;
def run_suite():
     target_platform = lldbplatformutil.getPlatform()
 
     checkLibcxxSupport()
     checkLibstdcxxSupport()
     checkWatchpointSupport()
     checkDebugInfoSupport()
     checkDebugServerSupport()
     checkObjcSupport()
+    checkForkVForkSupport()
 
     print("Skipping the following test categories: {}".format(configuration.skip_categories))
 
     for testdir in configuration.testdirs:
         for (dirpath, dirnames, filenames) in os.walk(testdir):
             visit('Test', dirpath, filenames)
 
     #

lldb/packages/Python/lldbsuite/test/test_categories.py

all_categories = {
     'dataformatters': 'Tests related to the type command and the data formatters subsystem',
     'debugserver': 'Debugserver tests',
     'dsym': 'Tests that can be run with DSYM debug information',
     'dwarf': 'Tests that can be run with DWARF debug information',
     'dwo': 'Tests that can be run with DWO debug information',
     'dyntype': 'Tests related to dynamic type support',
     'expression': 'Tests related to the expression parser',
     'flakey': 'Flakey test cases, i.e. tests that do not reliably pass at each execution',
+    'fork': 'Tests requiring the process plugin fork/vfork event support',
     'gmodules': 'Tests that can be run with -gmodules debug information',
     'instrumentation-runtime': 'Tests for the instrumentation runtime plugins',
     'libc++': 'Test for libc++ data formatters',
     'libstdcxx': 'Test for libstdcxx data formatters',
     'lldb-server': 'Tests related to lldb-server',
     'lldb-vscode': 'Visual Studio Code debug adaptor tests',
     'llgs': 'Tests for the gdb-server functionality of lldb-server',
     'objc': 'Tests related to the Objective-C programming language support',

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

def add_remove_breakpoint_packets(
         self.test_sequence.add_log_lines(
             [  # Remove the breakpoint.
                 "read packet: $z{2},{0:x},{1}#00".format(
                     address, breakpoint_kind, z_packet_type),
                 # Verify the stub could unset it.
                 "send packet: $OK#00",
             ], True)
 
-    def add_qSupported_packets(self):
+    def add_qSupported_packets(self, client_features=[]):
+        features = ''.join(';' + x for x in client_features)
         self.test_sequence.add_log_lines(
-            ["read packet: $qSupported#00",
+            ["read packet: $qSupported{}#00".format(features),
              {"direction": "send", "regex": r"^\$(.*)#[0-9a-fA-F]{2}", "capture": {1: "qSupported_response"}},
              ], True)
 
     _KNOWN_QSUPPORTED_STUB_FEATURES = [
         "augmented-libraries-svr4-read",
         "PacketSize",
         "QStartNoAckMode",
         "QThreadSuffixSupported",
         "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/GDBRemoteCommunicationServerLLGS.h

protected:
   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_thread_suffix_supported = false;
   bool m_list_threads_in_stop_reply = false;
 
+  NativeProcessProtocol::Extension m_extensions_supported = {};
+
   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, lldb::pid_t default_pid);
 
+  // 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

#endif
     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);
     m_continue_process = m_current_process = m_debugged_process_up.get();
   }
 
+  SetEnabledExtensions(*m_current_process);
+
   // 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) {
   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);
   m_continue_process = m_current_process = m_debugged_process_up.get();
+  SetEnabledExtensions(*m_current_process);
 
   // 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);
if (pid != m_current_process->GetID())
           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 =
+  std::vector<std::string> ret =
       GDBRemoteCommunicationServerCommon::HandleFeatures(client_features);
   ret.insert(ret.end(), {
     "QThreadSuffixSupported+", "QListThreadsInStopReply+",
-        "qXfer:features:read+", "multiprocess+",
+        "qXfer:features:read+",
 #if defined(__linux__) || defined(__NetBSD__) || defined(__FreeBSD__)
         "QPassSignals+", "qXfer:auxv:read+", "qXfer:libraries-svr4:read+",
 #endif
   });
+
+  // check for client features
+  using Extension = NativeProcessProtocol::Extension;
+  m_extensions_supported = {};
+  for (llvm::StringRef x : client_features)
+    m_extensions_supported |=
+        llvm::StringSwitch<Extension>(x)
+            .Case("multiprocess+", Extension::multiprocess)
+            .Case("fork-events+", Extension::fork)
+            .Case("vfork-events+", Extension::vfork)
+            .Default({});
+  m_extensions_supported &= m_process_factory.GetSupportedExtensions();
+
+  // fork & vfork require multiprocess
+  if (!bool(m_extensions_supported & Extension::multiprocess))
+    m_extensions_supported &= ~(Extension::fork | Extension::vfork);
+
+  // report only if actually supported
+  if (bool(m_extensions_supported & Extension::multiprocess))
+    ret.push_back("multiprocess+");
+  if (bool(m_extensions_supported & Extension::fork))
+    ret.push_back("fork-events+");
+  if (bool(m_extensions_supported & Extension::vfork))
+    ret.push_back("vfork-events+");
+
+  if (m_debugged_process_up)
+    SetEnabledExtensions(*m_debugged_process_up);
   return ret;
 }
+
+void GDBRemoteCommunicationServerLLGS::SetEnabledExtensions(
+    NativeProcessProtocol &process) {
+  NativeProcessProtocol::Extension flags = m_extensions_supported;
+  assert(!bool(flags & ~m_process_factory.GetSupportedExtensions()));
+  process.SetEnabledExtensions(flags);
+}

lldb/test/API/tools/lldb-server/TestLldbGdbServer.py

def test_hardware_breakpoint_set_and_remove_work(self):
         if self.getArchitecture() == "arm":
             # TODO: Handle case when setting breakpoint in thumb code
             self.build(dictionary={'CFLAGS_EXTRAS': '-marm'})
         else:
             self.build()
         self.set_inferior_startup_launch()
         self.breakpoint_set_and_remove_work(want_hardware=True)
 
-    def test_qSupported_returns_known_stub_features(self):
+    def get_qSupported_dict(self, features=[]):
         self.build()
         self.set_inferior_startup_launch()
 
         # Start up the stub and start/prep the inferior.
         procs = self.prep_debug_monitor_and_inferior()
-        self.add_qSupported_packets()
+        self.add_qSupported_packets(features)
 
         # Run the packet stream.
         context = self.expect_gdbremote_sequence()
         self.assertIsNotNone(context)
 
         # Retrieve the qSupported features.
-        supported_dict = self.parse_qSupported_response(context)
+        return self.parse_qSupported_response(context)
+
+    def test_qSupported_returns_known_stub_features(self):
+        supported_dict = self.get_qSupported_dict()
         self.assertIsNotNone(supported_dict)
         self.assertTrue(len(supported_dict) > 0)
 
+    @add_test_categories(["fork"])
+    def test_qSupported_fork_events(self):
+        supported_dict = (
+            self.get_qSupported_dict(['multiprocess+', 'fork-events+']))
+        self.assertEqual(supported_dict.get('multiprocess', '-'), '+')
+        self.assertEqual(supported_dict.get('fork-events', '-'), '+')
+        self.assertEqual(supported_dict.get('vfork-events', '-'), '-')
+
+    @add_test_categories(["fork"])
+    def test_qSupported_fork_events_without_multiprocess(self):
+        supported_dict = (
+            self.get_qSupported_dict(['fork-events+']))
+        self.assertEqual(supported_dict.get('multiprocess', '-'), '-')
+        self.assertEqual(supported_dict.get('fork-events', '-'), '-')
+        self.assertEqual(supported_dict.get('vfork-events', '-'), '-')
+
+    @add_test_categories(["fork"])
+    def test_qSupported_vfork_events(self):
+        supported_dict = (
+            self.get_qSupported_dict(['multiprocess+', 'vfork-events+']))
+        self.assertEqual(supported_dict.get('multiprocess', '-'), '+')
+        self.assertEqual(supported_dict.get('fork-events', '-'), '-')
+        self.assertEqual(supported_dict.get('vfork-events', '-'), '+')
+
+    @add_test_categories(["fork"])
+    def test_qSupported_vfork_events_without_multiprocess(self):
+        supported_dict = (
+            self.get_qSupported_dict(['vfork-events+']))
+        self.assertEqual(supported_dict.get('multiprocess', '-'), '-')
+        self.assertEqual(supported_dict.get('fork-events', '-'), '-')
+        self.assertEqual(supported_dict.get('vfork-events', '-'), '-')
+
     @skipIfWindows # No pty support to test any inferior output
     def test_written_M_content_reads_back_correctly(self):
         self.build()
         self.set_inferior_startup_launch()
 
         TEST_MESSAGE = "Hello, memory"
 
         # Start up the stub and start/prep the inferior.