[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

413–415

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>
+#include <list>
 
 #if !HAVE_PPOLL && !HAVE_SYS_EVENT_H && !defined(__ANDROID__)
 #define SIGNAL_POLLING_UNSUPPORTED 1
 #endif
 
 namespace lldb_private {
 
 // Implementation of the MainLoopBase class. It can monitor file descriptors
public:
   // 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, std::list<Callback>::iterator callback_it);
 
 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_it); }
 
   private:
-    SignalHandle(MainLoop &mainloop, int signo)
-        : m_mainloop(mainloop), m_signo(signo) {}
+    SignalHandle(MainLoop &mainloop, int signo,
+                 std::list<Callback>::iterator callback_it)
+        : m_mainloop(mainloop), m_signo(signo), m_callback_it(callback_it) {}
 
     MainLoop &m_mainloop;
     int m_signo;
+    std::list<Callback>::iterator m_callback_it;
 
     friend class MainLoop;
     SignalHandle(const SignalHandle &) = delete;
     const SignalHandle &operator=(const SignalHandle &) = delete;
   };
 
   struct SignalInfo {
-    Callback callback;
+    std::list<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>

 // 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) {
 #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()) {
+    auto callback_it = signal_it->second.callbacks.insert(
+        signal_it->second.callbacks.end(), callback);
+    return SignalHandleUP(new SignalHandle(*this, signo, callback_it));
   }
 
   SignalInfo info;
-  info.callback = callback;
+  info.callbacks.push_back(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
 
   // If we're using kqueue, the signal needs to be unblocked in order to
   // 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});
+  auto insert_ret = m_signals.insert({signo, info});
 
-  return SignalHandleUP(new SignalHandle(*this, signo));
+  return SignalHandleUP(
+      new SignalHandle(*this, signo, insert_ret.first->second.callbacks.begin()));
 #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,
+                                std::list<Callback>::iterator callback_it) {
 #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());
 
+  it->second.callbacks.erase(callback_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);
+    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
 }