[lldb] [MainLoop] Support multiple callbacks per signal

[mgorny]

Summary

Support registering multiple callbacks for a single signal. This is
necessary to support multiple co-existing native process instances, with
separate SIGCHLD handlers.

The system signal handler is registered on first request, additional
callback are added on subsequent requests. The system signal handler
is removed when last callback is unregistered.

[labath]

I've been thinking about whether this should be done here, or via a separate multiplexer entity. It's not clear to me which one is cleaner so I suppose we can go with what you have done here.

That said, I'm not too happy about this callback_id business. IIUC, it's only there to enable removing a specific callback (because you can't have a set of std::functions). But there are other ways to achieve that, and they don't leak this detail to the users. One option would be to hold the callbacks in a std::list, and use the (stable) iterator as the ID....

It could also use a test... something like, register a callback, fire a signal and check it's called, registers a second callback and check that both are called, unregister the first one and check that only the second one is called, ...

[mgorny]

In D100418#2689348, @labath wrote:

I've been thinking about whether this should be done here, or via a separate multiplexer entity. It's not clear to me which one is cleaner so I suppose we can go with what you have done here.

That said, I'm not too happy about this callback_id business. IIUC, it's only there to enable removing a specific callback (because you can't have a set of std::functions). But there are other ways to achieve that, and they don't leak this detail to the users. One option would be to hold the callbacks in a std::list, and use the (stable) iterator as the ID....

It could also use a test... something like, register a callback, fire a signal and check it's called, registers a second callback and check that both are called, unregister the first one and check that only the second one is called, ...

Hmm, my first though was to use a static int that increases with every call. But the std::list idea is interesting, so I'll explore that.

[labath]

In D100418#2689425, @mgorny wrote:

In D100418#2689348, @labath wrote:

I've been thinking about whether this should be done here, or via a separate multiplexer entity. It's not clear to me which one is cleaner so I suppose we can go with what you have done here.

That said, I'm not too happy about this callback_id business. IIUC, it's only there to enable removing a specific callback (because you can't have a set of std::functions). But there are other ways to achieve that, and they don't leak this detail to the users. One option would be to hold the callbacks in a std::list, and use the (stable) iterator as the ID....

It could also use a test... something like, register a callback, fire a signal and check it's called, registers a second callback and check that both are called, unregister the first one and check that only the second one is called, ...

Hmm, my first though was to use a static int that increases with every call. But the std::list idea is interesting, so I'll explore that.

That would work too, though it should definitely be a member variable instead of a global (thread safety). The only difference should be in the memory footprint. I suspect the list solution will be slightly smaller, unless you use DenseMap. OTOH, a DenseMap with only a few items tends to waste space...

[mgorny]

Store callbacks in std::list, and use the list iterator to identify the callback to remove.

[mgorny]

Fixed storing incorrect iterator (to temporary info).

[labath]

Looks good, just add the test case.

lldb/source/Host/common/MainLoop.cpp

417–419

llvm::SmallVector<Callback, 4> callbacks_to_run(it->second.callbacks.begin(), it->second.callbacks.end());

[mgorny]

Fixed the iterator, formatting and added a test for adding, running and removing multiple handlers.

lldb/include/lldb/Host/MainLoop.h

 //===-- MainLoop.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_HOST_MAINLOOP_H
 #define LLDB_HOST_MAINLOOP_H
 
 #include "lldb/Host/Config.h"
 #include "lldb/Host/MainLoopBase.h"
 #include "llvm/ADT/DenseMap.h"
 #include <csignal>
 
 #if !HAVE_PPOLL && !HAVE_SYS_EVENT_H && !defined(__ANDROID__)
 #define SIGNAL_POLLING_UNSUPPORTED 1
 #endif
 
public:
   // Listening for signals from multiple MainLoop instances is perfectly safe
   // as long as they don't try to listen for the same signal. The callback
   // function is invoked when the control returns to the Run() function, not
   // when the hander is executed. This mean that you can treat the callback as
   // a normal function and perform things which would not be safe in a signal
   // handler. However, since the callback is not invoked synchronously, you
   // cannot use this mechanism to handle SIGSEGV and the like.
   SignalHandleUP RegisterSignal(int signo, const Callback &callback,
-                                Status &error);
+                                Status &error, void *callback_id = nullptr);
 
   Status Run() override;
 
   // This should only be performed from a callback. Do not attempt to terminate
   // the processing from another thread.
   // TODO: Add synchronization if we want to be terminated from another thread.
   void RequestTermination() override { m_terminate_request = true; }
 
 protected:
   void UnregisterReadObject(IOObject::WaitableHandle handle) override;
 
-  void UnregisterSignal(int signo);
+  void UnregisterSignal(int signo, void *callback_id);
 
 private:
   void ProcessReadObject(IOObject::WaitableHandle handle);
   void ProcessSignal(int signo);
 
   class SignalHandle {
   public:
-    ~SignalHandle() { m_mainloop.UnregisterSignal(m_signo); }
+    ~SignalHandle() { m_mainloop.UnregisterSignal(m_signo, m_callback_id); }
 
   private:
-    SignalHandle(MainLoop &mainloop, int signo)
-        : m_mainloop(mainloop), m_signo(signo) {}
+    SignalHandle(MainLoop &mainloop, int signo, void *callback_id)
+        : m_mainloop(mainloop), m_signo(signo), m_callback_id(callback_id) {}
 
     MainLoop &m_mainloop;
     int m_signo;
+    void *m_callback_id;
 
     friend class MainLoop;
     SignalHandle(const SignalHandle &) = delete;
     const SignalHandle &operator=(const SignalHandle &) = delete;
   };
 
   struct SignalInfo {
-    Callback callback;
+    llvm::DenseMap<void*, Callback> callbacks;
 #if HAVE_SIGACTION
     struct sigaction old_action;
 #endif
     bool was_blocked : 1;
   };
   class RunImpl;
 
   llvm::DenseMap<IOObject::WaitableHandle, Callback> m_read_fds;

lldb/source/Host/common/MainLoop.cpp

 //===-- MainLoop.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 "llvm/Config/llvm-config.h"
 #include "lldb/Host/Config.h"
 
 #include "lldb/Host/MainLoop.h"
 #include "lldb/Host/PosixApi.h"
 #include "lldb/Utility/Status.h"
 #include <algorithm>
 #include <cassert>
 #include <cerrno>
 #include <csignal>
if (!inserted) {
     return nullptr;
   }
 
   return CreateReadHandle(object_sp);
 }
 
 // We shall block the signal, then install the signal handler. The signal will
 // be unblocked in the Run() function to check for signal delivery.
-MainLoop::SignalHandleUP
-MainLoop::RegisterSignal(int signo, const Callback &callback, Status &error) {
+MainLoop::SignalHandleUP MainLoop::RegisterSignal(int signo,
+                                                  const Callback &callback,
+                                                  Status &error,
+                                                  void *callback_id) {
 #ifdef SIGNAL_POLLING_UNSUPPORTED
   error.SetErrorString("Signal polling is not supported on this platform.");
   return nullptr;
 #else
-  if (m_signals.find(signo) != m_signals.end()) {
-    error.SetErrorStringWithFormat("Signal %d already monitored.", signo);
-    return nullptr;
+  auto signal_it = m_signals.find(signo);
+  if (signal_it != m_signals.end()) {
+    assert(signal_it->second.callbacks.find(callback_id) ==
+           signal_it->second.callbacks.end());
+    signal_it->second.callbacks[callback_id] = callback;
+    return SignalHandleUP(new SignalHandle(*this, signo, callback_id));
   }
 
   SignalInfo info;
-  info.callback = callback;
+  info.callbacks[callback_id] = callback;
   struct sigaction new_action;
   new_action.sa_sigaction = &SignalHandler;
   new_action.sa_flags = SA_SIGINFO;
   sigemptyset(&new_action.sa_mask);
   sigaddset(&new_action.sa_mask, signo);
   sigset_t old_set;
 
   g_signal_flags[signo] = 0;
#endif
   // receive it. If using pselect/ppoll, we need to block it, and later unblock
   // it as a part of the system call.
   ret = pthread_sigmask(HAVE_SYS_EVENT_H ? SIG_UNBLOCK : SIG_BLOCK,
                         &new_action.sa_mask, &old_set);
   assert(ret == 0 && "pthread_sigmask failed");
   info.was_blocked = sigismember(&old_set, signo);
   m_signals.insert({signo, info});
 
-  return SignalHandleUP(new SignalHandle(*this, signo));
+  return SignalHandleUP(new SignalHandle(*this, signo, callback_id));
 #endif
 }
 
 void MainLoop::UnregisterReadObject(IOObject::WaitableHandle handle) {
   bool erased = m_read_fds.erase(handle);
   UNUSED_IF_ASSERT_DISABLED(erased);
   assert(erased);
 }
 
-void MainLoop::UnregisterSignal(int signo) {
+void MainLoop::UnregisterSignal(int signo, void *callback_id) {
 #if SIGNAL_POLLING_UNSUPPORTED
   Status("Signal polling is not supported on this platform.");
 #else
   auto it = m_signals.find(signo);
   assert(it != m_signals.end());
 
+  auto cb_it = it->second.callbacks.find(callback_id);
+  assert(cb_it != it->second.callbacks.end());
+  it->second.callbacks.erase(cb_it);
+
+  // Do not remove the signal handler unless all callbacks have been erased.
+  if (!it->second.callbacks.empty())
+    return;
+
   sigaction(signo, &it->second.old_action, nullptr);
 
   sigset_t set;
   sigemptyset(&set);
   sigaddset(&set, signo);
   int ret = pthread_sigmask(it->second.was_blocked ? SIG_BLOCK : SIG_UNBLOCK,
                             &set, nullptr);
   assert(ret == 0);
while (!m_terminate_request && (!m_read_fds.empty() || !m_signals.empty())) {
 
     impl.ProcessEvents();
   }
   return Status();
 }
 
 void MainLoop::ProcessSignal(int signo) {
   auto it = m_signals.find(signo);
-  if (it != m_signals.end())
-    it->second.callback(*this); // Do the work
+  if (it != m_signals.end()) {
+    // The callback may actually register/unregister signal handlers,
+    // so we need to create a copy first.
+    llvm::SmallVector<Callback, 4> callbacks_to_run;
+    for (auto &x : it->second.callbacks)
+      callbacks_to_run.push_back(x.second);
+    for (auto &x : callbacks_to_run)
+      x(*this); // Do the work
+  }
 }
 
 void MainLoop::ProcessReadObject(IOObject::WaitableHandle handle) {
   auto it = m_read_fds.find(handle);
   if (it != m_read_fds.end())
     it->second(*this); // Do the work
 }

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

: NativeProcessELF(pid, terminal_fd, delegate), m_arch(arch),
       m_main_loop(mainloop), m_intel_pt_manager(pid) {
   if (m_terminal_fd != -1) {
     Status status = EnsureFDFlags(m_terminal_fd, O_NONBLOCK);
     assert(status.Success());
   }
 
   Status status;
   m_sigchld_handle = mainloop.RegisterSignal(
-      SIGCHLD, [this](MainLoopBase &) { SigchldHandler(); }, status);
+      SIGCHLD, [this](MainLoopBase &) { SigchldHandler(); }, status, this);
   assert(m_sigchld_handle && status.Success());
 
   for (const auto &tid : tids) {
     NativeThreadLinux &thread = AddThread(tid, /*resume*/ false);
     ThreadWasCreated(thread);
   }
 
   // Let our process instance know the thread has stopped.