SBDebugger: Add new APIs AddDestroyCallback and RemoveDestroyCallback (#89868)

# Motivation

Individual callers of `SBDebugger::SetDestroyCallback()` might think
that they have registered their callback and expect it to be called when
the debugger is destroyed. In reality, only the last caller survives,
and all previous callers are forgotten, which might be a surprise to
them. Worse, if this is called in a race condition, which callback
survives is less predictable, which may case confusing behavior
elsewhere.

# This PR

Allows multiple destroy callbacks to be registered and all called when
the debugger is destroyed.

**EDIT**: Adds two new APIs: `AddDestroyCallback()` and
`ClearDestroyCallback()`. `SetDestroyCallback()` will first clear then
add the given callback. Tests are added for the new APIs.

## Tests

```
bin/llvm-lit -sv ../external/llvm-project/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
```

## (out-dated, see comments below) Semantic change to
`SetDestroyCallback()`

~~Currently, the method overwrites the old callback with the new one.
With this PR, it will NOT overwrite. Instead, it will hold on to both.
Both callbacks get called during destroy.~~

~~**Risk**: Although the documentation of `SetDestroyCallback()` (see
[C++](https://lldb.llvm.org/cpp_reference/classlldb_1_1SBDebugger.html#afa1649d9453a376b5c95888b5a0cb4ec)
and
[python](https://lldb.llvm.org/python_api/lldb.SBDebugger.html#lldb.SBDebugger.SetDestroyCallback))
doesn't really specify the behavior, there is a risk: if existing call
sites rely on the "overwrite" behavior, they will be surprised because
now the old callback will get called. But as the above said, the current
behavior of "overwrite" itself might be unintended, so I don't
anticipate users to rely on this behavior. In short, this risk might be
less of a problem if we correct it sooner rather than later (which is
what this PR is trying to do).~~

## (out-dated, see comments below) Implementation

~~The implementation holds a `std::vector<std::pair<callback, baton>>`.
When `SetDestroyCallback()` is called, callbacks and batons are appended
to the `std::vector`. When destroy event happen, the `(callback, baton)`
pairs are invoked FIFO. Finally, the `std::vector` is cleared.~~

# (out-dated, see comments below) Alternatives considered

~~Instead of changing `SetDestroyCallback()`, a new method
`AddDestroyCallback()` can be added, which use the same
`std::vector<std::pair<>>` implementation. Together with
`ClearDestroyCallback()` (see below), they will replace and deprecate
`SetDestroyCallback()`. Meanwhile, in order to be backward compatible,
`SetDestroyCallback()` need to be updated to clear the `std::vector` and
then add the new callback. Pros: The end state is semantically more
correct. Cons: More steps to take; potentially maintaining an
"incorrect" behavior (of "overwrite").~~

~~A new method `ClearDestroyCallback()` can be added. Might be
unnecessary at this point, because workflows which need to set then
clear callbacks may exist but shouldn't be too common at least for now.
Such method can be added later when needed.~~

~~The `std::vector` may bring slight performance drawback if its
implementation doesn't handle small size efficiently. However, even if
that's the case, this path should be very cold (only used during init
and destroy). Such performance drawback should be negligible.~~

~~A different implementation was also considered. Instead of using
`std::vector`, the current `m_destroy_callback` field can be kept
unchanged. When `SetDestroyCallback()` is called, a lambda function can
be stored into `m_destroy_callback`. This lambda function will first
call the old callback, then the new one. This way, `std::vector` is
avoided. However, this implementation is more complex, thus less
readable, with not much perf to gain.~~

---------

Co-authored-by: Roy Shi <[email protected]>

Author: royitaqi

lldb/include/lldb/API/SBDebugger.h

@@ -328,9 +328,22 @@ class LLDB_API SBDebugger {
 
   void SetLoggingCallback(lldb::LogOutputCallback log_callback, void *baton);
 
+  /// Clear all previously added callbacks and only add the given one.
+  LLDB_DEPRECATED_FIXME("Use AddDestroyCallback and RemoveDestroyCallback",
+                        "AddDestroyCallback")
   void SetDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
                           void *baton);
 
+  /// Add a callback for when the debugger is destroyed. Return a token, which
+  /// can be used to remove said callback. Multiple callbacks can be added by
+  /// calling this function multiple times, and will be invoked in FIFO order.
+  lldb::callback_token_t
+  AddDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
+                     void *baton);
+
+  /// Remove the specified callback. Return true if successful.
+  bool RemoveDestroyCallback(lldb::callback_token_t token);
+
 #ifndef SWIG
   LLDB_DEPRECATED_FIXME("Use DispatchInput(const void *, size_t)",
                         "DispatchInput(const void *, size_t)")

lldb/include/lldb/Core/Debugger.h

@@ -40,6 +40,7 @@
 #include "lldb/lldb-types.h"
 
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/DynamicLibrary.h"
@@ -559,10 +560,25 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
 
   static void ReportSymbolChange(const ModuleSpec &module_spec);
 
+  /// DEPRECATED: We used to only support one Destroy callback. Now that we
+  /// support Add and Remove, you should only remove callbacks that you added.
+  /// Use Add and Remove instead.
+  ///
+  /// Clear all previously added callbacks and only add the given one.
   void
   SetDestroyCallback(lldb_private::DebuggerDestroyCallback destroy_callback,
                      void *baton);
 
+  /// Add a callback for when the debugger is destroyed. Return a token, which
+  /// can be used to remove said callback. Multiple callbacks can be added by
+  /// calling this function multiple times, and will be invoked in FIFO order.
+  lldb::callback_token_t
+  AddDestroyCallback(lldb_private::DebuggerDestroyCallback destroy_callback,
+                     void *baton);
+
+  /// Remove the specified callback. Return true if successful.
+  bool RemoveDestroyCallback(lldb::callback_token_t token);
+
   /// Manually start the global event handler thread. It is useful to plugins
   /// that directly use the \a lldb_private namespace and want to use the
   /// debugger's default event handler thread instead of defining their own.
@@ -721,8 +737,19 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
   lldb::TargetSP m_dummy_target_sp;
   Diagnostics::CallbackID m_diagnostics_callback_id;
 
-  lldb_private::DebuggerDestroyCallback m_destroy_callback = nullptr;
-  void *m_destroy_callback_baton = nullptr;
+  std::mutex m_destroy_callback_mutex;
+  lldb::callback_token_t m_destroy_callback_next_token = 0;
+  struct DestroyCallbackInfo {
+    DestroyCallbackInfo() {}
+    DestroyCallbackInfo(lldb::callback_token_t token,
+                        lldb_private::DebuggerDestroyCallback callback,
+                        void *baton)
+        : token(token), callback(callback), baton(baton) {}
+    lldb::callback_token_t token;
+    lldb_private::DebuggerDestroyCallback callback;
+    void *baton;
+  };
+  llvm::SmallVector<DestroyCallbackInfo, 2> m_destroy_callbacks;
 
   uint32_t m_interrupt_requested = 0; ///< Tracks interrupt requests
   std::mutex m_interrupt_mutex;

lldb/include/lldb/lldb-types.h

@@ -62,12 +62,14 @@ typedef void *thread_arg_t;             // Host thread argument type
 typedef void *thread_result_t;          // Host thread result type
 typedef void *(*thread_func_t)(void *); // Host thread function type
 typedef int pipe_t;                     // Host pipe type
+typedef int callback_token_t;
 
 #endif // _WIN32
 
 #define LLDB_INVALID_PROCESS ((lldb::process_t)-1)
 #define LLDB_INVALID_HOST_THREAD ((lldb::thread_t)NULL)
 #define LLDB_INVALID_PIPE ((lldb::pipe_t)-1)
+#define LLDB_INVALID_CALLBACK_TOKEN ((lldb::callback_token_t) - 1)
 
 typedef void (*LogOutputCallback)(const char *, void *baton);
 typedef bool (*CommandOverrideCallback)(void *baton, const char **argv);

lldb/source/API/SBDebugger.cpp

@@ -1695,6 +1695,26 @@ void SBDebugger::SetDestroyCallback(
   }
 }
 
+lldb::callback_token_t
+SBDebugger::AddDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
+                               void *baton) {
+  LLDB_INSTRUMENT_VA(this, destroy_callback, baton);
+
+  if (m_opaque_sp)
+    return m_opaque_sp->AddDestroyCallback(destroy_callback, baton);
+
+  return LLDB_INVALID_CALLBACK_TOKEN;
+}
+
+bool SBDebugger::RemoveDestroyCallback(lldb::callback_token_t token) {
+  LLDB_INSTRUMENT_VA(this, token);
+
+  if (m_opaque_sp)
+    return m_opaque_sp->RemoveDestroyCallback(token);
+
+  return false;
+}
+
 SBTrace
 SBDebugger::LoadTraceFromFile(SBError &error,
                               const SBFileSpec &trace_description_file) {

lldb/source/Core/Debugger.cpp

@@ -743,9 +743,22 @@ DebuggerSP Debugger::CreateInstance(lldb::LogOutputCallback log_callback,
 }
 
 void Debugger::HandleDestroyCallback() {
-  if (m_destroy_callback) {
-    m_destroy_callback(GetID(), m_destroy_callback_baton);
-    m_destroy_callback = nullptr;
+  const lldb::user_id_t user_id = GetID();
+  // Invoke and remove all the callbacks in an FIFO order. Callbacks which are
+  // added during this loop will be appended, invoked and then removed last.
+  // Callbacks which are removed during this loop will not be invoked.
+  while (true) {
+    DestroyCallbackInfo callback_info;
+    {
+      std::lock_guard<std::mutex> guard(m_destroy_callback_mutex);
+      if (m_destroy_callbacks.empty())
+        break;
+      // Pop the first item in the list
+      callback_info = m_destroy_callbacks.front();
+      m_destroy_callbacks.erase(m_destroy_callbacks.begin());
+    }
+    // Call the destroy callback with user id and baton
+    callback_info.callback(user_id, callback_info.baton);
   }
 }
 
@@ -1427,8 +1440,30 @@ void Debugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
 
 void Debugger::SetDestroyCallback(
     lldb_private::DebuggerDestroyCallback destroy_callback, void *baton) {
-  m_destroy_callback = destroy_callback;
-  m_destroy_callback_baton = baton;
+  std::lock_guard<std::mutex> guard(m_destroy_callback_mutex);
+  m_destroy_callbacks.clear();
+  const lldb::callback_token_t token = m_destroy_callback_next_token++;
+  m_destroy_callbacks.emplace_back(token, destroy_callback, baton);
+}
+
+lldb::callback_token_t Debugger::AddDestroyCallback(
+    lldb_private::DebuggerDestroyCallback destroy_callback, void *baton) {
+  std::lock_guard<std::mutex> guard(m_destroy_callback_mutex);
+  const lldb::callback_token_t token = m_destroy_callback_next_token++;
+  m_destroy_callbacks.emplace_back(token, destroy_callback, baton);
+  return token;
+}
+
+bool Debugger::RemoveDestroyCallback(lldb::callback_token_t token) {
+  std::lock_guard<std::mutex> guard(m_destroy_callback_mutex);
+  for (auto it = m_destroy_callbacks.begin(); it != m_destroy_callbacks.end();
+       ++it) {
+    if (it->token == token) {
+      m_destroy_callbacks.erase(it);
+      return true;
+    }
+  }
+  return false;
 }
 
 static void PrivateReportProgress(Debugger &debugger, uint64_t progress_id,

lldb/test/API/python_api/debugger/TestDebuggerAPI.py

@@ -161,3 +161,124 @@ def foo(dbg_id):
         original_dbg_id = self.dbg.GetID()
         self.dbg.Destroy(self.dbg)
         self.assertEqual(destroy_dbg_id, original_dbg_id)
+
+    def test_AddDestroyCallback(self):
+        original_dbg_id = self.dbg.GetID()
+        called = []
+
+        def foo(dbg_id):
+            # Need nonlocal to modify closure variable.
+            nonlocal called
+            called += [('foo', dbg_id)]
+
+        def bar(dbg_id):
+            # Need nonlocal to modify closure variable.
+            nonlocal called
+            called += [('bar', dbg_id)]
+
+        token_foo = self.dbg.AddDestroyCallback(foo)
+        token_bar = self.dbg.AddDestroyCallback(bar)
+        self.dbg.Destroy(self.dbg)
+
+        # Should call both `foo()` and `bar()`.
+        self.assertEqual(called, [
+            ('foo', original_dbg_id),
+            ('bar', original_dbg_id),
+        ])
+
+    def test_RemoveDestroyCallback(self):
+        original_dbg_id = self.dbg.GetID()
+        called = []
+
+        def foo(dbg_id):
+            # Need nonlocal to modify closure variable.
+            nonlocal called
+            called += [('foo', dbg_id)]
+
+        def bar(dbg_id):
+            # Need nonlocal to modify closure variable.
+            nonlocal called
+            called += [('bar', dbg_id)]
+
+        token_foo = self.dbg.AddDestroyCallback(foo)
+        token_bar = self.dbg.AddDestroyCallback(bar)
+        ret = self.dbg.RemoveDestroyCallback(token_foo)
+        self.dbg.Destroy(self.dbg)
+
+        # `Remove` should be successful
+        self.assertTrue(ret)
+        # Should only call `bar()`
+        self.assertEqual(called, [('bar', original_dbg_id)])
+
+    def test_RemoveDestroyCallback_invalid_token(self):
+        original_dbg_id = self.dbg.GetID()
+        magic_token_that_should_not_exist = 32413
+        called = []
+
+        def foo(dbg_id):
+            # Need nonlocal to modify closure variable.
+            nonlocal called
+            called += [('foo', dbg_id)]
+
+        token_foo = self.dbg.AddDestroyCallback(foo)
+        ret = self.dbg.RemoveDestroyCallback(magic_token_that_should_not_exist)
+        self.dbg.Destroy(self.dbg)
+
+        # `Remove` should be unsuccessful
+        self.assertFalse(ret)
+        # Should call `foo()`
+        self.assertEqual(called, [('foo', original_dbg_id)])
+
+    def test_HandleDestroyCallback(self):
+        """
+        Validates:
+        1. AddDestroyCallback and RemoveDestroyCallback work during debugger destroy.
+        2. HandleDestroyCallback invokes all callbacks in FIFO order.
+        """
+        original_dbg_id = self.dbg.GetID()
+        events = []
+        bar_token = None
+
+        def foo(dbg_id):
+            # Need nonlocal to modify closure variable.
+            nonlocal events
+            events.append(('foo called', dbg_id))
+
+        def bar(dbg_id):
+            # Need nonlocal to modify closure variable.
+            nonlocal events
+            events.append(('bar called', dbg_id))
+
+        def add_foo(dbg_id):
+            # Need nonlocal to modify closure variable.
+            nonlocal events
+            events.append(('add_foo called', dbg_id))
+            events.append(('foo token', self.dbg.AddDestroyCallback(foo)))
+
+        def remove_bar(dbg_id):
+            # Need nonlocal to modify closure variable.
+            nonlocal events
+            events.append(('remove_bar called', dbg_id))
+            events.append(('remove bar ret', self.dbg.RemoveDestroyCallback(bar_token)))
+
+        # Setup
+        events.append(('add_foo token', self.dbg.AddDestroyCallback(add_foo)))
+        bar_token = self.dbg.AddDestroyCallback(bar)
+        events.append(('bar token', bar_token))
+        events.append(('remove_bar token', self.dbg.AddDestroyCallback(remove_bar)))
+        # Destroy
+        self.dbg.Destroy(self.dbg)
+
+        self.assertEqual(events, [
+            # Setup
+            ('add_foo token', 0), # add_foo should be added
+            ('bar token', 1), # bar should be added
+            ('remove_bar token', 2), # remove_bar should be added
+            # Destroy
+            ('add_foo called', original_dbg_id), # add_foo should be called
+            ('foo token', 3), # foo should be added
+            ('bar called', original_dbg_id), # bar should be called
+            ('remove_bar called', original_dbg_id), # remove_bar should be called
+            ('remove bar ret', False), # remove_bar should fail, because it's already invoked and removed
+            ('foo called', original_dbg_id), # foo should be called
+        ])