[Lldb-commits] [lldb] Allow multiple destroy callbacks in `SBDebugger::SetDestroyCallback()` (PR #89868)

via lldb-commits lldb-commits at lists.llvm.org
Fri Apr 26 14:15:52 PDT 2024


https://github.com/royitaqi updated https://github.com/llvm/llvm-project/pull/89868

>From 079a550481d4cdcb69ad01c376b5e1f0632a07d6 Mon Sep 17 00:00:00 2001
From: Roy Shi <royshi at meta.com>
Date: Tue, 23 Apr 2024 18:10:21 -0700
Subject: [PATCH 01/12] Allow multiple destroy callbacks in
 `SBDebugger::SetDestroyCallback()`

---
 lldb/include/lldb/Core/Debugger.h |  4 ++--
 lldb/source/Core/Debugger.cpp     | 10 +++++-----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index 418c2403d020f4..20884f954ec7db 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -731,8 +731,8 @@ 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::vector<std::pair<lldb_private::DebuggerDestroyCallback, void *>>
+ 	    m_destroy_callback_and_baton;
 
   uint32_t m_interrupt_requested = 0; ///< Tracks interrupt requests
   std::mutex m_interrupt_mutex;
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index 19b3cf3bbf46b1..0ebdf2b0a0f970 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -743,10 +743,11 @@ 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();
+  for (const auto &callback_and_baton : m_destroy_callback_and_baton) {
+    callback_and_baton.first(user_id, callback_and_baton.second);
   }
+  m_destroy_callback_and_baton.clear();
 }
 
 void Debugger::Destroy(DebuggerSP &debugger_sp) {
@@ -1427,8 +1428,7 @@ 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;
+  m_destroy_callback_and_baton.emplace_back(destroy_callback, baton);
 }
 
 static void PrivateReportProgress(Debugger &debugger, uint64_t progress_id,

>From 771b52723be8d0ffecaf8f0818105cfdb82ba332 Mon Sep 17 00:00:00 2001
From: Roy Shi <royshi at meta.com>
Date: Tue, 23 Apr 2024 21:05:58 -0700
Subject: [PATCH 02/12] Fix code format

---
 lldb/include/lldb/Core/Debugger.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index 20884f954ec7db..af025219b0bc12 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -732,7 +732,7 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
   Diagnostics::CallbackID m_diagnostics_callback_id;
 
   std::vector<std::pair<lldb_private::DebuggerDestroyCallback, void *>>
- 	    m_destroy_callback_and_baton;
+      m_destroy_callback_and_baton;
 
   uint32_t m_interrupt_requested = 0; ///< Tracks interrupt requests
   std::mutex m_interrupt_mutex;

>From d1f13cad8a3789a994572459893b32a225ba3e1b Mon Sep 17 00:00:00 2001
From: Roy Shi <royshi at meta.com>
Date: Wed, 24 Apr 2024 11:55:16 -0700
Subject: [PATCH 03/12] Add `AddDestroyCallback()` and `ClearDestroyCallback()`

---
 lldb/include/lldb/Core/Debugger.h | 11 +++++++++++
 lldb/source/Core/Debugger.cpp     | 12 +++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index af025219b0bc12..5b3e433f09c68e 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -568,10 +568,21 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
 
   static void ReportSymbolChange(const ModuleSpec &module_spec);
 
+  /// Add a callback for when the debugger is destroyed. A list is maintained
+  /// internally.
+  void
+  AddDestroyCallback(lldb_private::DebuggerDestroyCallback destroy_callback,
+                     void *baton);
+
+  /// Clear the list of callbacks, then add the callback.
   void
   SetDestroyCallback(lldb_private::DebuggerDestroyCallback destroy_callback,
                      void *baton);
 
+  /// Clear the list of callbacks.
+  void
+  ClearDestroyCallback();
+
   /// 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.
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index 0ebdf2b0a0f970..159913642f253e 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -1426,11 +1426,21 @@ void Debugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
       std::make_shared<CallbackLogHandler>(log_callback, baton);
 }
 
-void Debugger::SetDestroyCallback(
+void Debugger::AddDestroyCallback(
     lldb_private::DebuggerDestroyCallback destroy_callback, void *baton) {
   m_destroy_callback_and_baton.emplace_back(destroy_callback, baton);
 }
 
+void Debugger::SetDestroyCallback(
+    lldb_private::DebuggerDestroyCallback destroy_callback, void *baton) {
+  ClearDestroyCallback();
+  AddDestroyCallback(destroy_callback, baton);
+}
+
+void Debugger::ClearDestroyCallback() {
+  m_destroy_callback_and_baton.clear();
+}
+
 static void PrivateReportProgress(Debugger &debugger, uint64_t progress_id,
                                   std::string title, std::string details,
                                   uint64_t completed, uint64_t total,

>From f536124f6f31ac72baf601fd438f21a04ac20691 Mon Sep 17 00:00:00 2001
From: Roy Shi <royshi at meta.com>
Date: Wed, 24 Apr 2024 14:18:25 -0700
Subject: [PATCH 04/12] Reflect API changes in `SBDebugger` class and add test

---
 lldb/include/lldb/API/SBDebugger.h            |  7 +++-
 lldb/source/API/SBDebugger.cpp                | 26 +++++++++---
 .../python_api/debugger/TestDebuggerAPI.py    | 42 +++++++++++++++++++
 3 files changed, 69 insertions(+), 6 deletions(-)

diff --git a/lldb/include/lldb/API/SBDebugger.h b/lldb/include/lldb/API/SBDebugger.h
index cf5409a12a056a..2501f8efb45ad5 100644
--- a/lldb/include/lldb/API/SBDebugger.h
+++ b/lldb/include/lldb/API/SBDebugger.h
@@ -194,7 +194,7 @@ class LLDB_API SBDebugger {
   lldb::SBCommandInterpreter GetCommandInterpreter();
 
   void HandleCommand(const char *command);
-  
+
   void RequestInterrupt();
   void CancelInterruptRequest();
   bool InterruptRequested();
@@ -321,9 +321,14 @@ class LLDB_API SBDebugger {
 
   void SetLoggingCallback(lldb::LogOutputCallback log_callback, void *baton);
 
+  void AddDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
+                          void *baton);
+
   void SetDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
                           void *baton);
 
+  void ClearDestroyCallback();
+
 #ifndef SWIG
   LLDB_DEPRECATED_FIXME("Use DispatchInput(const void *, size_t)",
                         "DispatchInput(const void *, size_t)")
diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp
index fbcf30e67fc1cd..754e3d27024a9f 100644
--- a/lldb/source/API/SBDebugger.cpp
+++ b/lldb/source/API/SBDebugger.cpp
@@ -1686,6 +1686,15 @@ void SBDebugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
   }
 }
 
+void 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);
+  }
+}
+
 void SBDebugger::SetDestroyCallback(
     lldb::SBDebuggerDestroyCallback destroy_callback, void *baton) {
   LLDB_INSTRUMENT_VA(this, destroy_callback, baton);
@@ -1695,6 +1704,13 @@ void SBDebugger::SetDestroyCallback(
   }
 }
 
+void SBDebugger::ClearDestroyCallback() {
+  LLDB_INSTRUMENT_VA(this);
+  if (m_opaque_sp) {
+    return m_opaque_sp->ClearDestroyCallback();
+  }
+}
+
 SBTrace
 SBDebugger::LoadTraceFromFile(SBError &error,
                               const SBFileSpec &trace_description_file) {
@@ -1704,20 +1720,20 @@ SBDebugger::LoadTraceFromFile(SBError &error,
 
 void SBDebugger::RequestInterrupt() {
   LLDB_INSTRUMENT_VA(this);
-  
+
   if (m_opaque_sp)
-    m_opaque_sp->RequestInterrupt();  
+    m_opaque_sp->RequestInterrupt();
 }
 void SBDebugger::CancelInterruptRequest()  {
   LLDB_INSTRUMENT_VA(this);
-  
+
   if (m_opaque_sp)
-    m_opaque_sp->CancelInterruptRequest();  
+    m_opaque_sp->CancelInterruptRequest();
 }
 
 bool SBDebugger::InterruptRequested()   {
   LLDB_INSTRUMENT_VA(this);
-  
+
   if (m_opaque_sp)
     return m_opaque_sp->InterruptRequested();
   return false;
diff --git a/lldb/test/API/python_api/debugger/TestDebuggerAPI.py b/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
index 522de2466012ed..46c62ea222a2c8 100644
--- a/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
+++ b/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
@@ -161,3 +161,45 @@ 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):
+        destroy_dbg_id = None
+        called = []
+
+        def foo(dbg_id):
+            # Need nonlocal to modify closure variable.
+            nonlocal destroy_dbg_id
+            destroy_dbg_id = dbg_id
+
+            nonlocal called
+            called += ['foo']
+
+        def bar(dbg_id):
+            # Need nonlocal to modify closure variable.
+            nonlocal destroy_dbg_id
+            destroy_dbg_id = dbg_id
+
+            nonlocal called
+            called += ['bar']
+
+        self.dbg.AddDestroyCallback(foo)
+        self.dbg.AddDestroyCallback(bar)
+
+        original_dbg_id = self.dbg.GetID()
+        self.dbg.Destroy(self.dbg)
+        self.assertEqual(destroy_dbg_id, original_dbg_id)
+        self.assertEqual(called, ['bar', 'foo'])
+
+    def test_ClearDestroyCallback(self):
+        destroy_dbg_id = None
+
+        def foo(dbg_id):
+            # Need nonlocal to modify closure variable.
+            nonlocal destroy_dbg_id
+            destroy_dbg_id = dbg_id
+
+        self.dbg.AddDestroyCallback(foo)
+        self.dbg.ClearDestroyCallback()
+
+        self.dbg.Destroy(self.dbg)
+        self.assertEqual(destroy_dbg_id, None)

>From 661dc1c27279d35ea9bf30a76fd1cbce045b37c7 Mon Sep 17 00:00:00 2001
From: Roy Shi <royshi at meta.com>
Date: Wed, 24 Apr 2024 14:47:52 -0700
Subject: [PATCH 05/12] Update test

---
 .../python_api/debugger/TestDebuggerAPI.py    | 22 +++++++------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/lldb/test/API/python_api/debugger/TestDebuggerAPI.py b/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
index 46c62ea222a2c8..cbcf60587a89aa 100644
--- a/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
+++ b/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
@@ -163,32 +163,25 @@ def foo(dbg_id):
         self.assertEqual(destroy_dbg_id, original_dbg_id)
 
     def test_AddDestroyCallback(self):
-        destroy_dbg_id = None
+        original_dbg_id = self.dbg.GetID()
         called = []
 
         def foo(dbg_id):
             # Need nonlocal to modify closure variable.
-            nonlocal destroy_dbg_id
-            destroy_dbg_id = dbg_id
-
             nonlocal called
-            called += ['foo']
+            called += [('foo', dbg_id)]
 
         def bar(dbg_id):
             # Need nonlocal to modify closure variable.
-            nonlocal destroy_dbg_id
-            destroy_dbg_id = dbg_id
-
             nonlocal called
-            called += ['bar']
+            called += [('bar', dbg_id)]
 
         self.dbg.AddDestroyCallback(foo)
         self.dbg.AddDestroyCallback(bar)
-
-        original_dbg_id = self.dbg.GetID()
         self.dbg.Destroy(self.dbg)
-        self.assertEqual(destroy_dbg_id, original_dbg_id)
-        self.assertEqual(called, ['bar', 'foo'])
+
+        # Should first call `foo()`, then `bar()`
+        self.assertEqual(called, [('foo', original_dbg_id), ('bar', original_dbg_id)])
 
     def test_ClearDestroyCallback(self):
         destroy_dbg_id = None
@@ -200,6 +193,7 @@ def foo(dbg_id):
 
         self.dbg.AddDestroyCallback(foo)
         self.dbg.ClearDestroyCallback()
-
         self.dbg.Destroy(self.dbg)
+
+        # `foo()` should never be called
         self.assertEqual(destroy_dbg_id, None)

>From b295e0d961938a218ca7f743067390f269c0603b Mon Sep 17 00:00:00 2001
From: Roy Shi <royshi at meta.com>
Date: Wed, 24 Apr 2024 14:54:20 -0700
Subject: [PATCH 06/12] Improve comment and Fix code format

---
 lldb/include/lldb/Core/Debugger.h | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index 5b3e433f09c68e..0669f031164558 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -568,20 +568,19 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
 
   static void ReportSymbolChange(const ModuleSpec &module_spec);
 
-  /// Add a callback for when the debugger is destroyed. A list is maintained
-  /// internally.
+  /// Add a callback for when the debugger is destroyed. Multiple callbacks
+  /// can be added by calling this function multiple times.
   void
   AddDestroyCallback(lldb_private::DebuggerDestroyCallback destroy_callback,
                      void *baton);
 
-  /// Clear the list of callbacks, then add the callback.
+  /// Clear all previously added callbacks and only add the given one.
   void
   SetDestroyCallback(lldb_private::DebuggerDestroyCallback destroy_callback,
                      void *baton);
 
-  /// Clear the list of callbacks.
-  void
-  ClearDestroyCallback();
+  /// Clear all previously added callbacks.
+  void ClearDestroyCallback();
 
   /// 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

>From 4ac92deaa1baa2e5ccc9359d12a760c32720f5e4 Mon Sep 17 00:00:00 2001
From: Roy Shi <royshi at meta.com>
Date: Thu, 25 Apr 2024 13:30:21 -0700
Subject: [PATCH 07/12] Thread-safe Add/Remove with tokens

---
 lldb/include/lldb/API/SBDebugger.h            |  6 +--
 lldb/include/lldb/API/SBDefines.h             |  2 +
 lldb/include/lldb/Core/Debugger.h             | 25 +++++----
 lldb/include/lldb/lldb-private-types.h        |  1 +
 lldb/source/API/SBDebugger.cpp                | 11 ++--
 lldb/source/Core/Debugger.cpp                 | 33 ++++++++----
 .../python_api/debugger/TestDebuggerAPI.py    | 54 ++++++++++++++-----
 7 files changed, 92 insertions(+), 40 deletions(-)

diff --git a/lldb/include/lldb/API/SBDebugger.h b/lldb/include/lldb/API/SBDebugger.h
index 2501f8efb45ad5..5d1e4deb6424d0 100644
--- a/lldb/include/lldb/API/SBDebugger.h
+++ b/lldb/include/lldb/API/SBDebugger.h
@@ -321,13 +321,13 @@ class LLDB_API SBDebugger {
 
   void SetLoggingCallback(lldb::LogOutputCallback log_callback, void *baton);
 
-  void AddDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
+  lldb::SBDebuggerDestroyCallbackToken AddDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
                           void *baton);
 
-  void SetDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
+  lldb::SBDebuggerDestroyCallbackToken SetDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
                           void *baton);
 
-  void ClearDestroyCallback();
+  bool RemoveDestroyCallback(lldb::SBDebuggerDestroyCallbackToken);
 
 #ifndef SWIG
   LLDB_DEPRECATED_FIXME("Use DispatchInput(const void *, size_t)",
diff --git a/lldb/include/lldb/API/SBDefines.h b/lldb/include/lldb/API/SBDefines.h
index 1181920677b46f..f1f1e0bfd9a84f 100644
--- a/lldb/include/lldb/API/SBDefines.h
+++ b/lldb/include/lldb/API/SBDefines.h
@@ -139,6 +139,8 @@ typedef bool (*SBBreakpointHitCallback)(void *baton, SBProcess &process,
 typedef void (*SBDebuggerDestroyCallback)(lldb::user_id_t debugger_id,
                                           void *baton);
 
+typedef int SBDebuggerDestroyCallbackToken;
+
 typedef SBError (*SBPlatformLocateModuleCallback)(
     void *baton, const SBModuleSpec &module_spec, SBFileSpec &module_file_spec,
     SBFileSpec &symbol_file_spec);
diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index 0669f031164558..93ad5abde8c02d 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -14,6 +14,7 @@
 #include <memory>
 #include <optional>
 #include <vector>
+#include <unordered_map>
 
 #include "lldb/Core/DebuggerEvents.h"
 #include "lldb/Core/FormatEntity.h"
@@ -568,19 +569,21 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
 
   static void ReportSymbolChange(const ModuleSpec &module_spec);
 
-  /// Add a callback for when the debugger is destroyed. Multiple callbacks
-  /// can be added by calling this function multiple times.
-  void
-  AddDestroyCallback(lldb_private::DebuggerDestroyCallback destroy_callback,
-                     void *baton);
-
+  /// DEPRECATED. Use AddDestroyCallback and RemoveDestroyCallback instead.
   /// Clear all previously added callbacks and only add the given one.
-  void
+  lldb_private::DebuggerDestroyCallbackToken
   SetDestroyCallback(lldb_private::DebuggerDestroyCallback destroy_callback,
                      void *baton);
 
-  /// Clear all previously added callbacks.
-  void ClearDestroyCallback();
+  /// 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.
+  lldb_private::DebuggerDestroyCallbackToken
+  AddDestroyCallback(lldb_private::DebuggerDestroyCallback destroy_callback,
+                     void *baton);
+
+  /// Remove the specified callback. Return true if successful.
+  bool RemoveDestroyCallback(lldb_private::DebuggerDestroyCallbackToken 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
@@ -741,7 +744,9 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
   lldb::TargetSP m_dummy_target_sp;
   Diagnostics::CallbackID m_diagnostics_callback_id;
 
-  std::vector<std::pair<lldb_private::DebuggerDestroyCallback, void *>>
+  std::recursive_mutex m_destroy_callback_mutex;
+  lldb_private::DebuggerDestroyCallbackToken m_destroy_callback_next_token = 0;
+  std::unordered_map<lldb_private::DebuggerDestroyCallbackToken, std::pair<lldb_private::DebuggerDestroyCallback, void *>>
       m_destroy_callback_and_baton;
 
   uint32_t m_interrupt_requested = 0; ///< Tracks interrupt requests
diff --git a/lldb/include/lldb/lldb-private-types.h b/lldb/include/lldb/lldb-private-types.h
index 7d301666df1a17..0d89dd34003d6c 100644
--- a/lldb/include/lldb/lldb-private-types.h
+++ b/lldb/include/lldb/lldb-private-types.h
@@ -121,6 +121,7 @@ typedef struct type256 { uint64_t x[4]; } type256;
 using ValueObjectProviderTy =
     std::function<lldb::ValueObjectSP(ConstString, StackFrame *)>;
 
+typedef int DebuggerDestroyCallbackToken;
 typedef void (*DebuggerDestroyCallback)(lldb::user_id_t debugger_id,
                                         void *baton);
 typedef bool (*CommandOverrideCallbackWithResult)(
diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp
index 754e3d27024a9f..2d7e496315cf51 100644
--- a/lldb/source/API/SBDebugger.cpp
+++ b/lldb/source/API/SBDebugger.cpp
@@ -1686,29 +1686,32 @@ void SBDebugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
   }
 }
 
-void SBDebugger::AddDestroyCallback(
+lldb::SBDebuggerDestroyCallbackToken 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 -1;
 }
 
-void SBDebugger::SetDestroyCallback(
+lldb::SBDebuggerDestroyCallbackToken SBDebugger::SetDestroyCallback(
     lldb::SBDebuggerDestroyCallback destroy_callback, void *baton) {
   LLDB_INSTRUMENT_VA(this, destroy_callback, baton);
   if (m_opaque_sp) {
     return m_opaque_sp->SetDestroyCallback(
         destroy_callback, baton);
   }
+  return -1;
 }
 
-void SBDebugger::ClearDestroyCallback() {
+bool SBDebugger::RemoveDestroyCallback(lldb::SBDebuggerDestroyCallbackToken token) {
   LLDB_INSTRUMENT_VA(this);
   if (m_opaque_sp) {
-    return m_opaque_sp->ClearDestroyCallback();
+    return m_opaque_sp->RemoveDestroyCallback(token);
   }
+  return false;
 }
 
 SBTrace
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index 159913642f253e..ea66db2c360d8c 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -743,9 +743,12 @@ DebuggerSP Debugger::CreateInstance(lldb::LogOutputCallback log_callback,
 }
 
 void Debugger::HandleDestroyCallback() {
+  std::lock_guard<std::recursive_mutex> guard(m_destroy_callback_mutex);
   const lldb::user_id_t user_id = GetID();
-  for (const auto &callback_and_baton : m_destroy_callback_and_baton) {
-    callback_and_baton.first(user_id, callback_and_baton.second);
+  for (const auto &element : m_destroy_callback_and_baton) {
+    const auto &callback = element.second.first;
+    const auto &baton = element.second.second;
+    callback(user_id, baton);
   }
   m_destroy_callback_and_baton.clear();
 }
@@ -1426,19 +1429,27 @@ void Debugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
       std::make_shared<CallbackLogHandler>(log_callback, baton);
 }
 
-void Debugger::AddDestroyCallback(
+lldb_private::DebuggerDestroyCallbackToken Debugger::SetDestroyCallback(
     lldb_private::DebuggerDestroyCallback destroy_callback, void *baton) {
-  m_destroy_callback_and_baton.emplace_back(destroy_callback, baton);
+  std::lock_guard<std::recursive_mutex> guard(m_destroy_callback_mutex);
+  m_destroy_callback_and_baton.clear();
+  return AddDestroyCallback(destroy_callback, baton);
 }
 
-void Debugger::SetDestroyCallback(
+lldb_private::DebuggerDestroyCallbackToken Debugger::AddDestroyCallback(
     lldb_private::DebuggerDestroyCallback destroy_callback, void *baton) {
-  ClearDestroyCallback();
-  AddDestroyCallback(destroy_callback, baton);
-}
-
-void Debugger::ClearDestroyCallback() {
-  m_destroy_callback_and_baton.clear();
+  std::lock_guard<std::recursive_mutex> guard(m_destroy_callback_mutex);
+  const auto token = m_destroy_callback_next_token++;
+  m_destroy_callback_and_baton.emplace(
+    std::piecewise_construct,
+              std::forward_as_tuple(token),
+              std::forward_as_tuple(destroy_callback, baton));
+  return token;
+}
+
+bool Debugger::RemoveDestroyCallback(lldb_private::DebuggerDestroyCallbackToken token) {
+  std::lock_guard<std::recursive_mutex> guard(m_destroy_callback_mutex);
+  return m_destroy_callback_and_baton.erase(token);
 }
 
 static void PrivateReportProgress(Debugger &debugger, uint64_t progress_id,
diff --git a/lldb/test/API/python_api/debugger/TestDebuggerAPI.py b/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
index cbcf60587a89aa..a8f3be2fe71362 100644
--- a/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
+++ b/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
@@ -176,24 +176,54 @@ def bar(dbg_id):
             nonlocal called
             called += [('bar', dbg_id)]
 
-        self.dbg.AddDestroyCallback(foo)
-        self.dbg.AddDestroyCallback(bar)
+        token_foo = self.dbg.AddDestroyCallback(foo)
+        token_bar = self.dbg.AddDestroyCallback(bar)
         self.dbg.Destroy(self.dbg)
 
-        # Should first call `foo()`, then `bar()`
-        self.assertEqual(called, [('foo', original_dbg_id), ('bar', original_dbg_id)])
+        # Should call both `foo()` and `bar()`. Order is undermined because
+        # of the `unordered_map` in the implementation.
+        self.assertTrue(('foo', original_dbg_id) in called)
+        self.assertTrue(('bar', original_dbg_id) in called)
 
-    def test_ClearDestroyCallback(self):
-        destroy_dbg_id = None
+    def test_RemoveDestroyCallback(self):
+        original_dbg_id = self.dbg.GetID()
+        called = []
 
         def foo(dbg_id):
             # Need nonlocal to modify closure variable.
-            nonlocal destroy_dbg_id
-            destroy_dbg_id = dbg_id
+            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)]
 
-        self.dbg.AddDestroyCallback(foo)
-        self.dbg.ClearDestroyCallback()
+        token_foo = self.dbg.AddDestroyCallback(foo)
+        ret = self.dbg.RemoveDestroyCallback(magic_token_that_should_not_exist)
         self.dbg.Destroy(self.dbg)
 
-        # `foo()` should never be called
-        self.assertEqual(destroy_dbg_id, None)
+        # `Remove` should be unsuccessful
+        self.assertFalse(ret)
+        # Should call `foo()`
+        self.assertEqual(called, [('foo', original_dbg_id)])

>From e5bf62059e4052806e1d4851d75c8c708090811c Mon Sep 17 00:00:00 2001
From: Roy Shi <royshi at meta.com>
Date: Thu, 25 Apr 2024 13:34:43 -0700
Subject: [PATCH 08/12] Fix format

---
 lldb/include/lldb/API/SBDebugger.h | 10 ++++++----
 lldb/include/lldb/Core/Debugger.h  |  5 +++--
 lldb/source/API/SBDebugger.cpp     | 11 ++++++-----
 lldb/source/Core/Debugger.cpp      |  8 ++++----
 4 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/lldb/include/lldb/API/SBDebugger.h b/lldb/include/lldb/API/SBDebugger.h
index 5d1e4deb6424d0..a85016754f6256 100644
--- a/lldb/include/lldb/API/SBDebugger.h
+++ b/lldb/include/lldb/API/SBDebugger.h
@@ -321,11 +321,13 @@ class LLDB_API SBDebugger {
 
   void SetLoggingCallback(lldb::LogOutputCallback log_callback, void *baton);
 
-  lldb::SBDebuggerDestroyCallbackToken AddDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
-                          void *baton);
+  lldb::SBDebuggerDestroyCallbackToken
+  AddDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
+                     void *baton);
 
-  lldb::SBDebuggerDestroyCallbackToken SetDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
-                          void *baton);
+  lldb::SBDebuggerDestroyCallbackToken
+  SetDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
+                     void *baton);
 
   bool RemoveDestroyCallback(lldb::SBDebuggerDestroyCallbackToken);
 
diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index 93ad5abde8c02d..31d9556fb9ae7e 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -13,8 +13,8 @@
 
 #include <memory>
 #include <optional>
-#include <vector>
 #include <unordered_map>
+#include <vector>
 
 #include "lldb/Core/DebuggerEvents.h"
 #include "lldb/Core/FormatEntity.h"
@@ -746,7 +746,8 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
 
   std::recursive_mutex m_destroy_callback_mutex;
   lldb_private::DebuggerDestroyCallbackToken m_destroy_callback_next_token = 0;
-  std::unordered_map<lldb_private::DebuggerDestroyCallbackToken, std::pair<lldb_private::DebuggerDestroyCallback, void *>>
+  std::unordered_map<lldb_private::DebuggerDestroyCallbackToken,
+                     std::pair<lldb_private::DebuggerDestroyCallback, void *>>
       m_destroy_callback_and_baton;
 
   uint32_t m_interrupt_requested = 0; ///< Tracks interrupt requests
diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp
index 2d7e496315cf51..b9e579b443c3c3 100644
--- a/lldb/source/API/SBDebugger.cpp
+++ b/lldb/source/API/SBDebugger.cpp
@@ -1686,8 +1686,8 @@ void SBDebugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
   }
 }
 
-lldb::SBDebuggerDestroyCallbackToken SBDebugger::AddDestroyCallback(
-    lldb::SBDebuggerDestroyCallback destroy_callback, void *baton) {
+lldb::SBDebuggerDestroyCallbackToken
+SBDebugger::AddDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback, void *baton) {
   LLDB_INSTRUMENT_VA(this, destroy_callback, baton);
   if (m_opaque_sp) {
     return m_opaque_sp->AddDestroyCallback(
@@ -1696,8 +1696,8 @@ lldb::SBDebuggerDestroyCallbackToken SBDebugger::AddDestroyCallback(
   return -1;
 }
 
-lldb::SBDebuggerDestroyCallbackToken SBDebugger::SetDestroyCallback(
-    lldb::SBDebuggerDestroyCallback destroy_callback, void *baton) {
+lldb::SBDebuggerDestroyCallbackToken
+SBDebugger::SetDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback, void *baton) {
   LLDB_INSTRUMENT_VA(this, destroy_callback, baton);
   if (m_opaque_sp) {
     return m_opaque_sp->SetDestroyCallback(
@@ -1706,7 +1706,8 @@ lldb::SBDebuggerDestroyCallbackToken SBDebugger::SetDestroyCallback(
   return -1;
 }
 
-bool SBDebugger::RemoveDestroyCallback(lldb::SBDebuggerDestroyCallbackToken token) {
+bool SBDebugger::RemoveDestroyCallback(
+    lldb::SBDebuggerDestroyCallbackToken token) {
   LLDB_INSTRUMENT_VA(this);
   if (m_opaque_sp) {
     return m_opaque_sp->RemoveDestroyCallback(token);
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index ea66db2c360d8c..0724e1aed961d5 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -1441,13 +1441,13 @@ lldb_private::DebuggerDestroyCallbackToken Debugger::AddDestroyCallback(
   std::lock_guard<std::recursive_mutex> guard(m_destroy_callback_mutex);
   const auto token = m_destroy_callback_next_token++;
   m_destroy_callback_and_baton.emplace(
-    std::piecewise_construct,
-              std::forward_as_tuple(token),
-              std::forward_as_tuple(destroy_callback, baton));
+      std::piecewise_construct, std::forward_as_tuple(token),
+      std::forward_as_tuple(destroy_callback, baton));
   return token;
 }
 
-bool Debugger::RemoveDestroyCallback(lldb_private::DebuggerDestroyCallbackToken token) {
+bool Debugger::RemoveDestroyCallback(
+    lldb_private::DebuggerDestroyCallbackToken token) {
   std::lock_guard<std::recursive_mutex> guard(m_destroy_callback_mutex);
   return m_destroy_callback_and_baton.erase(token);
 }

>From 1206762edc4129a2b9a4571b2995ac737678a7d3 Mon Sep 17 00:00:00 2001
From: Roy Shi <royshi at meta.com>
Date: Fri, 26 Apr 2024 12:39:03 -0700
Subject: [PATCH 09/12] Public token type and void return for Set

---
 lldb/include/lldb/API/SBDebugger.h     | 11 +++++------
 lldb/include/lldb/API/SBDefines.h      |  2 --
 lldb/include/lldb/Core/Debugger.h      | 15 +++++++++------
 lldb/include/lldb/lldb-private-types.h |  1 -
 lldb/include/lldb/lldb-types.h         |  3 +++
 lldb/source/API/SBDebugger.cpp         | 21 +++++++++------------
 lldb/source/Core/Debugger.cpp          |  9 ++++-----
 7 files changed, 30 insertions(+), 32 deletions(-)

diff --git a/lldb/include/lldb/API/SBDebugger.h b/lldb/include/lldb/API/SBDebugger.h
index a85016754f6256..5b50dd9a95e07a 100644
--- a/lldb/include/lldb/API/SBDebugger.h
+++ b/lldb/include/lldb/API/SBDebugger.h
@@ -321,15 +321,14 @@ class LLDB_API SBDebugger {
 
   void SetLoggingCallback(lldb::LogOutputCallback log_callback, void *baton);
 
-  lldb::SBDebuggerDestroyCallbackToken
-  AddDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
-                     void *baton);
+  void SetDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
+                          void *baton);
 
-  lldb::SBDebuggerDestroyCallbackToken
-  SetDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
+  lldb::destroy_callback_token_t
+  AddDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
                      void *baton);
 
-  bool RemoveDestroyCallback(lldb::SBDebuggerDestroyCallbackToken);
+  bool RemoveDestroyCallback(lldb::destroy_callback_token_t token);
 
 #ifndef SWIG
   LLDB_DEPRECATED_FIXME("Use DispatchInput(const void *, size_t)",
diff --git a/lldb/include/lldb/API/SBDefines.h b/lldb/include/lldb/API/SBDefines.h
index f1f1e0bfd9a84f..1181920677b46f 100644
--- a/lldb/include/lldb/API/SBDefines.h
+++ b/lldb/include/lldb/API/SBDefines.h
@@ -139,8 +139,6 @@ typedef bool (*SBBreakpointHitCallback)(void *baton, SBProcess &process,
 typedef void (*SBDebuggerDestroyCallback)(lldb::user_id_t debugger_id,
                                           void *baton);
 
-typedef int SBDebuggerDestroyCallbackToken;
-
 typedef SBError (*SBPlatformLocateModuleCallback)(
     void *baton, const SBModuleSpec &module_spec, SBFileSpec &module_file_spec,
     SBFileSpec &symbol_file_spec);
diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index 31d9556fb9ae7e..58177b463b2e00 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -569,21 +569,24 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
 
   static void ReportSymbolChange(const ModuleSpec &module_spec);
 
-  /// DEPRECATED. Use AddDestroyCallback and RemoveDestroyCallback instead.
+  /// DEPRECATED: We used to only support one Destroy callback. Now that we
+  /// support Add and Remove, you should only remove Destroy callbacks that
+  /// you Add-ed. Use Add and Remove instead.
+  ///
   /// Clear all previously added callbacks and only add the given one.
-  lldb_private::DebuggerDestroyCallbackToken
+  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.
-  lldb_private::DebuggerDestroyCallbackToken
+  lldb::destroy_callback_token_t
   AddDestroyCallback(lldb_private::DebuggerDestroyCallback destroy_callback,
                      void *baton);
 
   /// Remove the specified callback. Return true if successful.
-  bool RemoveDestroyCallback(lldb_private::DebuggerDestroyCallbackToken token);
+  bool RemoveDestroyCallback(lldb::destroy_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
@@ -745,8 +748,8 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
   Diagnostics::CallbackID m_diagnostics_callback_id;
 
   std::recursive_mutex m_destroy_callback_mutex;
-  lldb_private::DebuggerDestroyCallbackToken m_destroy_callback_next_token = 0;
-  std::unordered_map<lldb_private::DebuggerDestroyCallbackToken,
+  lldb::destroy_callback_token_t m_destroy_callback_next_token = 0;
+  std::unordered_map<lldb::destroy_callback_token_t,
                      std::pair<lldb_private::DebuggerDestroyCallback, void *>>
       m_destroy_callback_and_baton;
 
diff --git a/lldb/include/lldb/lldb-private-types.h b/lldb/include/lldb/lldb-private-types.h
index 0d89dd34003d6c..7d301666df1a17 100644
--- a/lldb/include/lldb/lldb-private-types.h
+++ b/lldb/include/lldb/lldb-private-types.h
@@ -121,7 +121,6 @@ typedef struct type256 { uint64_t x[4]; } type256;
 using ValueObjectProviderTy =
     std::function<lldb::ValueObjectSP(ConstString, StackFrame *)>;
 
-typedef int DebuggerDestroyCallbackToken;
 typedef void (*DebuggerDestroyCallback)(lldb::user_id_t debugger_id,
                                         void *baton);
 typedef bool (*CommandOverrideCallbackWithResult)(
diff --git a/lldb/include/lldb/lldb-types.h b/lldb/include/lldb/lldb-types.h
index d60686e33142ac..41afbc312ac6e1 100644
--- a/lldb/include/lldb/lldb-types.h
+++ b/lldb/include/lldb/lldb-types.h
@@ -62,12 +62,15 @@ 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 destroy_callback_token_t;   // Debugger destroy callback token type
 
 #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_DESTROY_CALLBACK_TOKEN                                    \
+  ((lldb::destroy_callback_token_t) - 1)
 
 typedef void (*LogOutputCallback)(const char *, void *baton);
 typedef bool (*CommandOverrideCallback)(void *baton, const char **argv);
diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp
index b9e579b443c3c3..c1eb2aa5d45a66 100644
--- a/lldb/source/API/SBDebugger.cpp
+++ b/lldb/source/API/SBDebugger.cpp
@@ -1686,28 +1686,25 @@ void SBDebugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
   }
 }
 
-lldb::SBDebuggerDestroyCallbackToken
-SBDebugger::AddDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback, void *baton) {
+void SBDebugger::SetDestroyCallback(
+    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);
+    m_opaque_sp->SetDestroyCallback(destroy_callback, baton);
   }
-  return -1;
 }
 
-lldb::SBDebuggerDestroyCallbackToken
-SBDebugger::SetDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback, void *baton) {
+lldb::destroy_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->SetDestroyCallback(
-        destroy_callback, baton);
+    return m_opaque_sp->AddDestroyCallback(destroy_callback, baton);
   }
-  return -1;
+  return LLDB_INVALID_DESTROY_CALLBACK_TOKEN;
 }
 
-bool SBDebugger::RemoveDestroyCallback(
-    lldb::SBDebuggerDestroyCallbackToken token) {
+bool SBDebugger::RemoveDestroyCallback(lldb::destroy_callback_token_t token) {
   LLDB_INSTRUMENT_VA(this);
   if (m_opaque_sp) {
     return m_opaque_sp->RemoveDestroyCallback(token);
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index 0724e1aed961d5..6dc1119d1bedb4 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -1429,14 +1429,14 @@ void Debugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
       std::make_shared<CallbackLogHandler>(log_callback, baton);
 }
 
-lldb_private::DebuggerDestroyCallbackToken Debugger::SetDestroyCallback(
+void Debugger::SetDestroyCallback(
     lldb_private::DebuggerDestroyCallback destroy_callback, void *baton) {
   std::lock_guard<std::recursive_mutex> guard(m_destroy_callback_mutex);
   m_destroy_callback_and_baton.clear();
-  return AddDestroyCallback(destroy_callback, baton);
+  AddDestroyCallback(destroy_callback, baton);
 }
 
-lldb_private::DebuggerDestroyCallbackToken Debugger::AddDestroyCallback(
+lldb::destroy_callback_token_t Debugger::AddDestroyCallback(
     lldb_private::DebuggerDestroyCallback destroy_callback, void *baton) {
   std::lock_guard<std::recursive_mutex> guard(m_destroy_callback_mutex);
   const auto token = m_destroy_callback_next_token++;
@@ -1446,8 +1446,7 @@ lldb_private::DebuggerDestroyCallbackToken Debugger::AddDestroyCallback(
   return token;
 }
 
-bool Debugger::RemoveDestroyCallback(
-    lldb_private::DebuggerDestroyCallbackToken token) {
+bool Debugger::RemoveDestroyCallback(lldb::destroy_callback_token_t token) {
   std::lock_guard<std::recursive_mutex> guard(m_destroy_callback_mutex);
   return m_destroy_callback_and_baton.erase(token);
 }

>From d7e8b8da44db0a9aa95895b6e74e01a57cda49f1 Mon Sep 17 00:00:00 2001
From: Roy Shi <royshi at meta.com>
Date: Fri, 26 Apr 2024 13:44:25 -0700
Subject: [PATCH 10/12] Handle Add/Remove during Destroy

---
 lldb/source/Core/Debugger.cpp                 | 16 ++++--
 .../python_api/debugger/TestDebuggerAPI.py    | 52 +++++++++++++++++++
 2 files changed, 63 insertions(+), 5 deletions(-)

diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index 6dc1119d1bedb4..6a1e8e3782b615 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -745,12 +745,18 @@ DebuggerSP Debugger::CreateInstance(lldb::LogOutputCallback log_callback,
 void Debugger::HandleDestroyCallback() {
   std::lock_guard<std::recursive_mutex> guard(m_destroy_callback_mutex);
   const lldb::user_id_t user_id = GetID();
-  for (const auto &element : m_destroy_callback_and_baton) {
-    const auto &callback = element.second.first;
-    const auto &baton = element.second.second;
-    callback(user_id, baton);
+  // In case one destroy callback adds or removes other destroy callbacks
+  // which aren't taken care of in the same inner loop.
+  while (m_destroy_callback_and_baton.size()) {
+    auto iter = m_destroy_callback_and_baton.begin();
+    while (iter != m_destroy_callback_and_baton.end()) {
+      // Invoke the callback and remove the entry from the map
+      const auto &callback = iter->second.first;
+      const auto &baton = iter->second.second;
+      callback(user_id, baton);
+      iter = m_destroy_callback_and_baton.erase(iter);
+    }
   }
-  m_destroy_callback_and_baton.clear();
 }
 
 void Debugger::Destroy(DebuggerSP &debugger_sp) {
diff --git a/lldb/test/API/python_api/debugger/TestDebuggerAPI.py b/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
index a8f3be2fe71362..6fdc9837588567 100644
--- a/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
+++ b/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
@@ -227,3 +227,55 @@ def foo(dbg_id):
         self.assertFalse(ret)
         # Should call `foo()`
         self.assertEqual(called, [('foo', original_dbg_id)])
+
+    def test_HandleDestroyCallback(self):
+        """
+        Validates:
+        1. Add and Remove can function during debugger destroy.
+        2. HandleDestroyCallback can invoke all callbacks.
+        """
+        original_dbg_id = self.dbg.GetID()
+        events = {}
+
+        def foo(dbg_id):
+            # Need nonlocal to modify closure variable.
+            nonlocal events
+            events['foo called'] = dbg_id
+
+        def bar(dbg_id):
+            # Don't log bar()'s invokation because it may or may not be called
+            # based on its position in the container relative to remove_bar
+            pass
+
+        def add_foo(dbg_id):
+            # Need nonlocal to modify closure variable.
+            nonlocal events
+            events['add_foo called'] = dbg_id
+            events['foo token'] = self.dbg.AddDestroyCallback(foo)
+
+        def remove_bar(dbg_id):
+            # Need nonlocal to modify closure variable.
+            nonlocal events
+            events['remove_bar called'] = dbg_id
+            events['remove bar ret'] = self.dbg.RemoveDestroyCallback(events['bar token'])
+
+        # Setup
+        events['add_foo token'] = self.dbg.AddDestroyCallback(add_foo)
+        events['bar token'] = self.dbg.AddDestroyCallback(bar)
+        events['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 may or may not be called
+            'remove_bar called': original_dbg_id, # remove_bar should be called
+            'remove bar ret': True, # remove_bar should succeed
+            'foo called': original_dbg_id, # foo should be called
+        })

>From 192b00e03d6f856c74358c0b441549a34e0638ac Mon Sep 17 00:00:00 2001
From: Roy Shi <royshi at meta.com>
Date: Fri, 26 Apr 2024 13:50:31 -0700
Subject: [PATCH 11/12] Add comments

---
 lldb/include/lldb/API/SBDebugger.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lldb/include/lldb/API/SBDebugger.h b/lldb/include/lldb/API/SBDebugger.h
index 5b50dd9a95e07a..49e7382e9c61fd 100644
--- a/lldb/include/lldb/API/SBDebugger.h
+++ b/lldb/include/lldb/API/SBDebugger.h
@@ -321,13 +321,22 @@ class LLDB_API SBDebugger {
 
   void SetLoggingCallback(lldb::LogOutputCallback log_callback, void *baton);
 
+  /// DEPRECATED: We used to only support one Destroy callback. Now that we
+  /// support Add and Remove, you should only remove Destroy callbacks that
+  /// you Add-ed. Use Add and Remove instead.
+  ///
+  /// Clear all previously added callbacks and only add the given one.
   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.
   lldb::destroy_callback_token_t
   AddDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
                      void *baton);
 
+  /// Remove the specified callback. Return true if successful.
   bool RemoveDestroyCallback(lldb::destroy_callback_token_t token);
 
 #ifndef SWIG

>From 1e02efc0df33906904e7545190fc78e6146278bd Mon Sep 17 00:00:00 2001
From: Roy Shi <royshi at meta.com>
Date: Fri, 26 Apr 2024 14:15:21 -0700
Subject: [PATCH 12/12] Add LLDB_DEPRECATED_FIXME

---
 lldb/include/lldb/API/SBDebugger.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lldb/include/lldb/API/SBDebugger.h b/lldb/include/lldb/API/SBDebugger.h
index 49e7382e9c61fd..58cf8deea4bec3 100644
--- a/lldb/include/lldb/API/SBDebugger.h
+++ b/lldb/include/lldb/API/SBDebugger.h
@@ -326,6 +326,8 @@ class LLDB_API SBDebugger {
   /// you Add-ed. Use Add and Remove instead.
   ///
   /// 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);
 



More information about the lldb-commits mailing list