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

[via lldb-commits]

royitaqi wrote:

Hi @JDevlieghere ,

The PR has changed quite a bit. If you have the time, maybe it's a good time to 
take another look.

Below is a TL;DR of some of the changes:
* Aadded Add/Remove
* Leave Set unchanged (but marked as deprecated)
* Added token in Add/Remove
* Handle concurrency
* Handle Add/Remove invocations from existing destroy callbacks during the 
destroy event


https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

royitaqi wrote:

Hi @jimingham and @bulbazord ,

I *think* I have addressed all your comments (thanks for those!). I have also 
eye-balled through my PR again. But it's possible that I missed something.

Will be great to have you taking a look when you have the time.

Thanks,
Roy

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

@@ -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,

royitaqi wrote:

Added.  I assume I only need to add it to SBDebugger.h, not Debugger.h.  Hope 
the message/fix I wrote is also good.

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

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 
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,
   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>
+   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 
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,
   Diagnostics::CallbackID m_diagnostics_callback_id;
 
   std::vector>
-   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 
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,
 
   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(log_callback, baton);
 }
 
-void Debugger::SetDestroyCallback(
+void Debugger::AddDestroyCallback(
 lldb_private::DebuggerDestroyCallback destroy_callbac

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

[via lldb-commits]

@@ -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,

royitaqi wrote:

Thanks for the tip! Will find an example and use it.

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

https://github.com/royitaqi edited 
https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

https://github.com/royitaqi edited 
https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

https://github.com/royitaqi edited 
https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

@@ -743,9 +743,19 @@ 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;
+  std::lock_guard guard(m_destroy_callback_mutex);
+  const lldb::user_id_t user_id = GetID();
+  // 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);
+}

royitaqi wrote:

Readability discussion (ignore if you think this particular one is too small a 
topic):

I thought about writing the inner loop as `for (begin; end; ) { callback(); 
iter = map.erase(); }`. However, I felt it's basically the same as the above. 
So I just left it as is. LMK if you think the `for` loop will look better.

FWIW, if we do use the `for` loop, I didn't like the idea of moving the `iter = 
map.erase()` part into the 3rd clause of the `for` line. LMK if you think 
differently

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Alex Langford via lldb-commits]

@@ -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,

bulbazord wrote:

There are some macros you can use in addition to the documentation: 
`LLDB_DEPRECATED_FIXME`. This will give compiler warnings to anyone using 
`SetDestroyCallback` so they can update their code.

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

@@ -321,8 +321,15 @@ class LLDB_API SBDebugger {
 
   void SetLoggingCallback(lldb::LogOutputCallback log_callback, void *baton);
 
-  void SetDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
-  void *baton);
+  lldb::SBDebuggerDestroyCallbackToken
+  AddDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
+ void *baton);
+
+  lldb::SBDebuggerDestroyCallbackToken
+  SetDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,

royitaqi wrote:

Yeah. Thanks for catching this. Reverted the API changes (both in SBDebugger 
and in Debugger).

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

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 
Date: Tue, 23 Apr 2024 18:10:21 -0700
Subject: [PATCH 01/11] 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,
   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>
+   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 
Date: Tue, 23 Apr 2024 21:05:58 -0700
Subject: [PATCH 02/11] 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,
   Diagnostics::CallbackID m_diagnostics_callback_id;
 
   std::vector>
-   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 
Date: Wed, 24 Apr 2024 11:55:16 -0700
Subject: [PATCH 03/11] 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,
 
   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(log_callback, baton);
 }
 
-void Debugger::SetDestroyCallback(
+void Debugger::AddDestroyCallback(
 lldb_private::DebuggerDestroyCallback destroy_callbac

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

[via lldb-commits]

@@ -1686,13 +1686,33 @@ void 
SBDebugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
   }
 }
 
-void SBDebugger::SetDestroyCallback(
-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(
+destroy_callback, baton);
+  }
+  return -1;

royitaqi wrote:

Added `LLDB_INVALID_DESTROY_CALLBACK_TOKEN`.

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

@@ -568,10 +569,22 @@ class Debugger : public 
std::enable_shared_from_this,
 
   static void ReportSymbolChange(const ModuleSpec &module_spec);
 
-  void
+  /// DEPRECATED. Use AddDestroyCallback and RemoveDestroyCallback instead.
+  /// Clear all previously added callbacks and only add the given one.

royitaqi wrote:

Added into comment

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

@@ -121,6 +121,7 @@ typedef struct type256 { uint64_t x[4]; } type256;
 using ValueObjectProviderTy =
 std::function;
 
+typedef int DebuggerDestroyCallbackToken;

royitaqi wrote:

Added `lldb::destroy_callback_token_t`.

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

@@ -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();

royitaqi wrote:

Added double while-loop in `HandleDestroyCallback` to handle callbacks which 
can be added/removed during destroy. Added test `test_HandleDestroyCallback` to 
validate a simple example case.

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

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 
Date: Tue, 23 Apr 2024 18:10:21 -0700
Subject: [PATCH 01/10] 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,
   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>
+   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 
Date: Tue, 23 Apr 2024 21:05:58 -0700
Subject: [PATCH 02/10] 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,
   Diagnostics::CallbackID m_diagnostics_callback_id;
 
   std::vector>
-   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 
Date: Wed, 24 Apr 2024 11:55:16 -0700
Subject: [PATCH 03/10] 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,
 
   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(log_callback, baton);
 }
 
-void Debugger::SetDestroyCallback(
+void Debugger::AddDestroyCallback(
 lldb_private::DebuggerDestroyCallback destroy_callbac

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

[via lldb-commits]

@@ -321,8 +321,15 @@ class LLDB_API SBDebugger {
 
   void SetLoggingCallback(lldb::LogOutputCallback log_callback, void *baton);
 
-  void SetDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
-  void *baton);
+  lldb::SBDebuggerDestroyCallbackToken
+  AddDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
+ void *baton);
+
+  lldb::SBDebuggerDestroyCallbackToken
+  SetDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,

jimingham wrote:

Formally, returning the token from SetDestroyCallback is a reasonable addition, 
but it changes the signature of an extant SB API, which we don't allow - we are 
strict about maintaining backwards compatibility of the SB API's.  Since we're 
deprecating this API anyway, I don't think we need to make Set play well with 
the Add/Remove model.  Just leave it returning void.

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

jimingham wrote:

> Hi @jimingham ,
> 
> I have updated the code. Now we have `Add` and `Remove` (with tokens as you 
> suggested). `Set` clears everything then add the given one (also returns a 
> token).
> 
> Thread-safety is done through a `std::lock_guard`. This 
> is consistent with some of the other functions/fields.
> 
> Didn't understand one of your comment about "one destroy callback can 
> add/remove others" and how that relates to the code. See my reply in the 
> above.
> 
> Another thing I am not sure is, in python API test, how do I make concurrent 
> calls to these APIs. Do you know if this should be tested (e.g. if other 
> existing functions has similar tests)? If the answer is "yes", do you happen 
> to know what existing test I can see as examples?
> 
> Thanks for your reviews so far. Hope this PR is getting better as we converse.
> 
> Best, Roy

I don't think it's necessary to try to test concurrency.  That tends to be very 
hard to write a stable test that you know actually tests concurrent access.  
Those end up being more trouble than they are worth.

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

@@ -321,8 +321,15 @@ class LLDB_API SBDebugger {
 
   void SetLoggingCallback(lldb::LogOutputCallback log_callback, void *baton);
 
-  void SetDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
-  void *baton);
+  lldb::SBDebuggerDestroyCallbackToken
+  AddDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
+ void *baton);
+
+  lldb::SBDebuggerDestroyCallbackToken
+  SetDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,

jimingham wrote:

This is also deprecated.  I'd add the same comment here, and use the 
LLDB_DEPRECATED_IN macro to mark it formally.

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

@@ -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();

jimingham wrote:

I meant the former.  You could have added two Destroy callbacks, but then 
during the execution of the first one, the code figures out that it has already 
done the job the second one was intended to do, so the first Destroy callback 
removed the second one on the way out to keep the work from getting done twice.

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

@@ -1686,13 +1686,33 @@ void 
SBDebugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
   }
 }
 
-void SBDebugger::SetDestroyCallback(
-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(
+destroy_callback, baton);
+  }
+  return -1;

jimingham wrote:

It's better to make an LLDB_INVALID_DESTROY_TOKEN and use that than have a 
magical -1 floating around in code.

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

@@ -568,10 +569,22 @@ class Debugger : public 
std::enable_shared_from_this,
 
   static void ReportSymbolChange(const ModuleSpec &module_spec);
 
-  void
+  /// DEPRECATED. Use AddDestroyCallback and RemoveDestroyCallback instead.
+  /// Clear all previously added callbacks and only add the given one.

jimingham wrote:

I would give a reason for the deprecation, like:

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.

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

@@ -121,6 +121,7 @@ typedef struct type256 { uint64_t x[4]; } type256;
 using ValueObjectProviderTy =
 std::function;
 
+typedef int DebuggerDestroyCallbackToken;

jimingham wrote:

There's no need to make separate lldb_private & SB versions of the 
CallbackToken.  Since this is intended to be a public type (it shows up in the 
SB API) you can put it in the lldb namespace - in lldb-types.h, and then both 
the SB API's and the lldb_private one can use that.

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

royitaqi wrote:

Hi @jimingham ,

I have updated the code. Now we have `Add` and `Remove` (with tokens as you 
suggested). `Set` clears everything then add the given one (also returns a 
token).

Thread-safety is done through a `std::lock_guard`. This 
is consistent with some of the other functions/fields.

Didn't understand one of your comment about "one destroy callback can 
add/remove others" and how that relates to the code. See my reply in the above.

Another thing I am not sure is, in python API test, how do I make concurrent 
calls to these APIs. Do you know if this should be tested (e.g. if other 
existing functions has similar tests)? If the answer is "yes", do you happen to 
know what existing test I can see as examples?


Thanks for your reviews so far. Hope this PR is getting better as we converse.

Best,
Roy

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

@@ -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();

royitaqi wrote:

I don't understand.

Were you referring to the `foreach` loop below and saying that since the 
container can be modified during the loop, the looping needs to be changed 
(e.g. to use the iterator instead of the `foreach` syntax)?

If you did meant to talk about the `user_id` then I don't think I understand 
how the `user_id` is related, because it seems it should be constant throughout 
the `HandleDestroyCallback` execution.

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

@@ -1425,10 +1426,19 @@ void 
Debugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
   std::make_shared(log_callback, baton);
 }
 
+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) {
-  m_destroy_callback = destroy_callback;
-  m_destroy_callback_baton = baton;
+  ClearDestroyCallback();
+  AddDestroyCallback(destroy_callback, baton);
+}
+
+void Debugger::ClearDestroyCallback() {
+  m_destroy_callback_and_baton.clear();

royitaqi wrote:

Addressed in the latest version.

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

@@ -1425,10 +1426,19 @@ void 
Debugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
   std::make_shared(log_callback, baton);
 }
 
+void Debugger::AddDestroyCallback(
+lldb_private::DebuggerDestroyCallback destroy_callback, void *baton) {
+  m_destroy_callback_and_baton.emplace_back(destroy_callback, baton);

royitaqi wrote:

Addressed in the latest version.

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

@@ -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();

royitaqi wrote:

Addressed in the latest version.

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

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 
Date: Tue, 23 Apr 2024 18:10:21 -0700
Subject: [PATCH 1/8] 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,
   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>
+   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 
Date: Tue, 23 Apr 2024 21:05:58 -0700
Subject: [PATCH 2/8] 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,
   Diagnostics::CallbackID m_diagnostics_callback_id;
 
   std::vector>
-   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 
Date: Wed, 24 Apr 2024 11:55:16 -0700
Subject: [PATCH 3/8] 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,
 
   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(log_callback, baton);
 }
 
-void Debugger::SetDestroyCallback(
+void Debugger::AddDestroyCallback(
 lldb_private::DebuggerDestroyCallback destroy_callback, voi

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

[via lldb-commits]

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 
Date: Tue, 23 Apr 2024 18:10:21 -0700
Subject: [PATCH 1/7] 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,
   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>
+   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 
Date: Tue, 23 Apr 2024 21:05:58 -0700
Subject: [PATCH 2/7] 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,
   Diagnostics::CallbackID m_diagnostics_callback_id;
 
   std::vector>
-   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 
Date: Wed, 24 Apr 2024 11:55:16 -0700
Subject: [PATCH 3/7] 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,
 
   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(log_callback, baton);
 }
 
-void Debugger::SetDestroyCallback(
+void Debugger::AddDestroyCallback(
 lldb_private::DebuggerDestroyCallback destroy_callback, voi

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

[via lldb-commits]

jimingham wrote:

For legacy reasons, I think we have to keep SetDestroyCallbacks doing what it 
did, but we should comment that it is deprecated and to use the Add and Remove 
to only affect your own callbacks.

Jim


> On Apr 24, 2024, at 6:19 PM, royitaqi ***@***.***> wrote:
> 
> 
> @royitaqi commented on this pull request.
> 
> In lldb/include/lldb/API/SBDebugger.h 
> :
> 
> >void SetDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
>void *baton);
>  
> +  void ClearDestroyCallback();
> It seems wrong that one client can only remove its destroy callback by 
> removing ones it didn't install.
> Makes sense to me.
> 
> I think you are suggesting:
> 
> TToken AddDestroyCallback(callback, baton)
> bool RemoveDestroyCallback(TToken token) - returns success/fail
> No/remove ClearDestroyCallbacks - because no one is supposed to clear
> Then what should SetDestroyCallback be?
> 
> It clears existing callbacks, so I think in this new world this semantics 
> isn't allowed, similar to how ClearDestroyCallbacks shouldn't exist.
> That is, unless, we introduce the concept of a client ID, where 
> SetDestroyCallback will remove all callbacks from the specified client ID. 
> But then I feel the introduction of the client ID opens up even more 
> questions, and it's not an existing pattern in the Debugger class. So it's 
> probably not a good idea.
> —
> Reply to this email directly, view it on GitHub 
> , or 
> unsubscribe 
> .
> You are receiving this because you were mentioned.
> 



https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

https://github.com/royitaqi edited 
https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

https://github.com/royitaqi edited 
https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

@@ -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();

royitaqi wrote:

> It seems wrong that one client can only remove its destroy callback by 
> removing ones it didn't install.
Makes sense to me.

I think you are suggesting:
* `TToken AddDestroyCallback(callback, baton)`
* `bool RemoveDestroyCallback(TToken token)` - returns success/fail
* No/remove `ClearDestroyCallbacks` - because no one is supposed to clear

Then what should `SetDestroyCallback` be?
* It clears existing callbacks, so I think in this new world this semantics 
isn't allowed, similar to how `ClearDestroyCallbacks` shouldn't exist.
* ~~That is, unless, we introduce the concept of a client ID, where 
`SetDestroyCallback` will remove all callbacks from the specified client ID. 
But then I feel the introduction of the client ID opens up even more questions, 
and it's not an existing pattern in the Debugger class. So it's probably not a 
good idea.~~

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

@@ -1425,10 +1426,19 @@ void 
Debugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
   std::make_shared(log_callback, baton);
 }
 
+void Debugger::AddDestroyCallback(
+lldb_private::DebuggerDestroyCallback destroy_callback, void *baton) {
+  m_destroy_callback_and_baton.emplace_back(destroy_callback, baton);

royitaqi wrote:

Good to know about the potential concurrency. Thanks!

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

royitaqi wrote:

# Tests that I did

## Manual test

```
username-mac ~/public_llvm/build % bin/lldb
(lldb) script
Python Interactive Interpreter. To exit, type 'quit()', 'exit()' or Ctrl-D.
>>> lldb.debugger.AddDestroyCallback(lambda user_id: print('foo %s' % user_id))
>>> lldb.debugger.AddDestroyCallback(lambda user_id: print('bar %s' % user_id))
>>> ^D
now exiting InteractiveConsole...
(lldb) ^D
foo 1
bar 1
username-mac ~/public_llvm/build %
```

## Run the added unit tests

username-mac ~/public_llvm/build % bin/llvm-lit -sv 
/Users/username/public_llvm/llvm-project/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
llvm-lit: 
/Users/username/public_llvm/llvm-project/lldb/test/API/lit.cfg.py:175: warning: 
Could not set a default per-test timeout. Requires the Python psutil module but 
it could not be found. Try installing it via pip or via your operating system's 
package manager.
llvm-lit: 
/Users/username/public_llvm/llvm-project/lldb/test/API/lit.cfg.py:113: note: 
Deleting module cache at 
/Users/username/public_llvm/build/lldb-test-build.noindex/module-cache-lldb/lldb-api.

Testing Time: 1.97s

Total Discovered Tests: 1
  Passed: 1 (100.00%)

1 warning(s) in tests
username-mac ~/public_llvm/build %
```


https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

royitaqi wrote:

Hi @JDevlieghere and @jimingham,

I have updated the PR to add `AddDestroyCallback()`, `ClearDestroyCallback()`, 
and tests for these. Also updated `SetDestroyCallback()` to work with the new 
field `std::vector>`.

Hope it looks better now. LMK if anything else needs to be added/changed.

--

BTW, noob as I am, is there anything I need to do to change the PR's state 
"Changes requested"?  E.g. In some other diff review tools, there is a thing to 
flip the state into "Ready for review".

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

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 
Date: Tue, 23 Apr 2024 18:10:21 -0700
Subject: [PATCH 1/6] 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,
   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>
+   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 
Date: Tue, 23 Apr 2024 21:05:58 -0700
Subject: [PATCH 2/6] 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,
   Diagnostics::CallbackID m_diagnostics_callback_id;
 
   std::vector>
-   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 
Date: Wed, 24 Apr 2024 11:55:16 -0700
Subject: [PATCH 3/6] 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,
 
   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(log_callback, baton);
 }
 
-void Debugger::SetDestroyCallback(
+void Debugger::AddDestroyCallback(
 lldb_private::DebuggerDestroyCallback destroy_callback, voi

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

[via lldb-commits]

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 
Date: Tue, 23 Apr 2024 18:10:21 -0700
Subject: [PATCH 1/5] 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,
   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>
+   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 
Date: Tue, 23 Apr 2024 21:05:58 -0700
Subject: [PATCH 2/5] 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,
   Diagnostics::CallbackID m_diagnostics_callback_id;
 
   std::vector>
-   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 
Date: Wed, 24 Apr 2024 11:55:16 -0700
Subject: [PATCH 3/5] 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,
 
   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(log_callback, baton);
 }
 
-void Debugger::SetDestroyCallback(
+void Debugger::AddDestroyCallback(
 lldb_private::DebuggerDestroyCallback destroy_callback, voi

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

[via lldb-commits]

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 
Date: Tue, 23 Apr 2024 18:10:21 -0700
Subject: [PATCH 1/4] 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,
   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>
+   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 
Date: Tue, 23 Apr 2024 21:05:58 -0700
Subject: [PATCH 2/4] 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,
   Diagnostics::CallbackID m_diagnostics_callback_id;
 
   std::vector>
-   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 
Date: Wed, 24 Apr 2024 11:55:16 -0700
Subject: [PATCH 3/4] 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,
 
   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(log_callback, baton);
 }
 
-void Debugger::SetDestroyCallback(
+void Debugger::AddDestroyCallback(
 lldb_private::DebuggerDestroyCallback destroy_callback, voi

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

[via lldb-commits]

jimingham wrote:

I don't have a strong preference for new PR vrs. reuse this one.

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

royitaqi wrote:

Thank you, @JDevlieghere and @jimingham for the input.

IIUR, you are basically advocating for the following alternative approach (that 
was mentioned in the "Alternatives considered" section in the above):

> Instead of changing SetDestroyCallback(), a new method AddDestroyCallback() 
> can be added, which use the same std::vector> 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.


I agree that adding new methods have more clear naming and don't change the 
semantics of the eixsting method. I will update this PR (or, **should I create 
a new PR?** - Noob I am.)

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

jimingham wrote:

I would also find it surprising if "SetDestroyCallback" did 
"AddDestroyCallback".  That's not what the name says it does.  I have no 
problem with supporting multiple Destroy callbacks.  But I agree with Jonas, 
that needs to be a separate API with an appropriate name.  And also tests...

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Jonas Devlieghere via lldb-commits]

https://github.com/JDevlieghere requested changes to this pull request.

Can you elaborate a bit more about why you want to change the behavior? Your 
motivation touches on the fact that it might be surprising or racy. While I 
think having the ability to register multiple callbacks makes sense, I'm not 
sure I agree with either.

 - The method is called `SetDestroyCallback` which makes it pretty clear that 
it's a setter, which is generally expected to change the value, rather than add 
to it. If the method had been called `AddDestroyCallback` on the other hand, I 
would expect it to add the callback rather than overwrite it. 
 - I don't think there's anything racy about a setter. Even if it were, nothing 
in this PR addresses the racy-ness.

The setter also makes it possible to remove a callback by overwriting it with a 
callback that does nothing. With the current approach that's no longer 
possible. I'm not sure if that's important, but something to consider. 

If the goal is to be able to specify multiple callbacks, a potential solution 
could be to store the callbacks in a list as you do in this PR, but add a new 
method `AddDestroyCallback` which appends to the list. We could keep the 
existing behavior for `SetDestroyCallback` by clearing the list and adding it 
as the first entry. That would also solve the problem of not being able to 
remove existing callbacks. 

Regardless of the direction, this definitely needs a corresponding test and 
updated documentation. 

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

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 
Date: Tue, 23 Apr 2024 18:10:21 -0700
Subject: [PATCH 1/2] 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,
   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>
+   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 
Date: Tue, 23 Apr 2024 21:05:58 -0700
Subject: [PATCH 2/2] 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,
   Diagnostics::CallbackID m_diagnostics_callback_id;
 
   std::vector>
-   m_destroy_callback_and_baton;
+  m_destroy_callback_and_baton;
 
   uint32_t m_interrupt_requested = 0; ///< Tracks interrupt requests
   std::mutex m_interrupt_mutex;

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

https://github.com/royitaqi edited 
https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

llvmbot wrote:




@llvm/pr-subscribers-lldb

Author: None (royitaqi)


Changes

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

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

## Implementation

The implementation holds a `std::vector>`. 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.

# Other considerations

Instead of changing `SetDestroyCallback()`, a new method `AddDestroyCallback()` 
can be added, which use the same `std::vector>` 
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.

---
Full diff: https://github.com/llvm/llvm-project/pull/89868.diff


2 Files Affected:

- (modified) lldb/include/lldb/Core/Debugger.h (+2-2) 
- (modified) lldb/source/Core/Debugger.cpp (+5-5) 


``diff
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,
   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>
+   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_

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

[via lldb-commits]

github-actions[bot] wrote:



Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this 
page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using `@` followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from 
other developers.

If you have further questions, they may be answered by the [LLVM GitHub User 
Guide](https://llvm.org/docs/GitHub.html).

You can also ask questions in a comment on this PR, on the [LLVM 
Discord](https://discord.com/invite/xS7Z362) or on the 
[forums](https://discourse.llvm.org/).

https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[via lldb-commits]

https://github.com/royitaqi created 
https://github.com/llvm/llvm-project/pull/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.

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

## Implementation

The implementation holds a `std::vector>`. 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.

# Other considerations

Instead of changing `SetDestroyCallback()`, a new method `AddDestroyCallback()` 
can be added, which use the same `std::vector>` 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.

>From 079a550481d4cdcb69ad01c376b5e1f0632a07d6 Mon Sep 17 00:00:00 2001
From: Roy Shi 
Date: Tue, 23 Apr 2024 18:10:21 -0700
Subject: [PATCH] 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,
   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>
+   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_dest