[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<std::pair<callback, baton>>`. When `SetDestroyCallback()` is called, callbacks and batons are appended to the `std::vector`. When destroy event happen, the `(callback, baton)` pairs are invoked FIFO. Finally, the `std::vector` is cleared.

# Other considerations

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

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

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

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

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