[lldb] Remove the Diagnostics callback mechanism

[JDevlieghere]

The Diagnostics framework had a callback registry (AddCallback / RemoveCallback) so subsystems could contribute files to a diagnostics directory, intended to also run during crash handling. That crash-time path never materialized, and the sole registered callback was the Debugger copying its file-backed logs. If you had no logging enabled, the directory would be empty, confusing the users.

Remove the registry and the callback loop in Diagnostics::Create (which now just writes the in-memory log), and expose the log copying as Debugger::CopyLogFilesToDirectory, which "diagnostics dump" calls directly. The dump command now copies the invoking debugger's logs rather than every debugger's, which is the more useful behavior I want to double down on.

[llvmorg-github-actions]

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

The Diagnostics framework had a callback registry (AddCallback / RemoveCallback) so subsystems could contribute files to a diagnostics directory, intended to also run during crash handling. That crash-time path never materialized, and the sole registered callback was the Debugger copying its file-backed logs. If you had no logging enabled, the directory would be empty, confusing the users.

Remove the registry and the callback loop in Diagnostics::Create (which now just writes the in-memory log), and expose the log copying as Debugger::CopyLogFilesToDirectory, which "diagnostics dump" calls directly. The dump command now copies the invoking debugger's logs rather than every debugger's, which is the more useful behavior I want to double down on.

---

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

5 Files Affected:

• (modified) lldb/include/lldb/Core/Debugger.h (+4-1)
• (modified) lldb/include/lldb/Utility/Diagnostics.h (+6-33)
• (modified) lldb/source/Commands/CommandObjectDiagnostics.cpp (+5)
• (modified) lldb/source/Core/Debugger.cpp (+10-19)
• (modified) lldb/source/Utility/Diagnostics.cpp (-18)

diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index 179ef0f38d940..b7d66f5795d43 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -266,6 +266,10 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
 
   void SetLoggingCallback(lldb::LogOutputCallback log_callback, void *baton);
 
+  /// Copy this debugger's file-backed log files into the given directory, for
+  /// inclusion in a diagnostics bundle. Best-effort; failures are skipped.
+  void CopyLogFilesToDirectory(const FileSpec &dir);
+
   Status SetPropertyValue(const ExecutionContext *exe_ctx,
                           VarSetOperationType op, llvm::StringRef property_path,
                           llvm::StringRef value) override;
@@ -805,7 +809,6 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
   lldb::ListenerSP m_forward_listener_sp;
   llvm::once_flag m_clear_once;
   lldb::TargetSP m_dummy_target_sp;
-  Diagnostics::CallbackID m_diagnostics_callback_id;
 
   /// Bookkeeping for command line progress events.
   /// @{
diff --git a/lldb/include/lldb/Utility/Diagnostics.h b/lldb/include/lldb/Utility/Diagnostics.h
index c2e4b44350a37..e32ffa5233f11 100644
--- a/lldb/include/lldb/Utility/Diagnostics.h
+++ b/lldb/include/lldb/Utility/Diagnostics.h
@@ -11,29 +11,25 @@
 
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
-#include "llvm/ADT/SmallVector.h"
-#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/Error.h"
 
-#include <functional>
-#include <mutex>
 #include <optional>
-#include <vector>
 
 namespace lldb_private {
 
-/// Diagnostics are a collection of files to help investigate bugs and
-/// troubleshoot issues. Any part of the debugger can register itself with the
-/// help of a callback to emit one or more files into the diagnostic directory.
+/// Diagnostics maintain an always-on, in-memory log of recent diagnostic
+/// messages that can be written out to help investigate bugs and troubleshoot
+/// issues.
 class Diagnostics {
 public:
   Diagnostics();
   ~Diagnostics();
 
-  /// Gather diagnostics in the given directory.
+  /// Write the in-memory diagnostic log into the given directory.
   llvm::Error Create(const FileSpec &dir);
 
-  /// Gather diagnostics and print a message to the given output stream.
+  /// Write the diagnostic log into a directory and print a message to the given
+  /// output stream.
   /// @{
   bool Dump(llvm::raw_ostream &stream);
   bool Dump(llvm::raw_ostream &stream, const FileSpec &dir);
@@ -41,12 +37,6 @@ class Diagnostics {
 
   void Report(llvm::StringRef message);
 
-  using Callback = std::function<llvm::Error(const FileSpec &)>;
-  using CallbackID = uint64_t;
-
-  CallbackID AddCallback(Callback callback);
-  void RemoveCallback(CallbackID id);
-
   static Diagnostics &Instance();
 
   static bool Enabled();
@@ -62,23 +52,6 @@ class Diagnostics {
   llvm::Error DumpDiangosticsLog(const FileSpec &dir) const;
 
   RotatingLogHandler m_log_handler;
-
-  struct CallbackEntry {
-    CallbackEntry(CallbackID id, Callback callback)
-        : id(id), callback(std::move(callback)) {}
-    CallbackID id;
-    Callback callback;
-  };
-
-  /// Monotonically increasing callback identifier. Unique per Diagnostic
-  /// instance.
-  CallbackID m_callback_id;
-
-  /// List of callback entries.
-  llvm::SmallVector<CallbackEntry, 4> m_callbacks;
-
-  /// Mutex to protect callback list and callback identifier.
-  std::mutex m_callbacks_mutex;
 };
 
 } // namespace lldb_private
diff --git a/lldb/source/Commands/CommandObjectDiagnostics.cpp b/lldb/source/Commands/CommandObjectDiagnostics.cpp
index b565e16e76b53..3e942c98c8180 100644
--- a/lldb/source/Commands/CommandObjectDiagnostics.cpp
+++ b/lldb/source/Commands/CommandObjectDiagnostics.cpp
@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "CommandObjectDiagnostics.h"
+#include "lldb/Core/Debugger.h"
 #include "lldb/Host/OptionParser.h"
 #include "lldb/Interpreter/CommandOptionArgumentTable.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
@@ -93,6 +94,10 @@ class CommandObjectDiagnosticsDump : public CommandObjectParsed {
       return;
     }
 
+    // Copy this debugger's file-backed logs into the directory. This used to be
+    // done by a Diagnostics callback registered by the Debugger.
+    GetDebugger().CopyLogFilesToDirectory(*directory);
+
     result.GetOutputStream() << "diagnostics written to " << *directory << '\n';
 
     result.SetStatus(eReturnStatusSuccessFinishResult);
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index 9a75c5eb407c5..b2e6fe66dc29e 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -1103,22 +1103,6 @@ Debugger::Debugger(lldb::LogOutputCallback log_callback, void *baton)
   if (!GetOutputFileSP()->GetIsTerminalWithColors())
     disable_color();
 
-  if (Diagnostics::Enabled()) {
-    m_diagnostics_callback_id = Diagnostics::Instance().AddCallback(
-        [this](const FileSpec &dir) -> llvm::Error {
-          for (auto &entry : m_stream_handlers) {
-            llvm::StringRef log_path = entry.first();
-            llvm::StringRef file_name = llvm::sys::path::filename(log_path);
-            FileSpec destination = dir.CopyByAppendingPathComponent(file_name);
-            std::error_code ec =
-                llvm::sys::fs::copy_file(log_path, destination.GetPath());
-            if (ec)
-              return llvm::errorCodeToError(ec);
-          }
-          return llvm::Error::success();
-        });
-  }
-
 #if defined(_WIN32) && defined(ENABLE_VIRTUAL_TERMINAL_PROCESSING)
   // Enabling use of ANSI color codes because LLDB is using them to highlight
   // text.
@@ -1164,9 +1148,6 @@ void Debugger::Clear() {
     GetInputFile().Close();
 
     m_command_interpreter_up->Clear();
-
-    if (Diagnostics::Enabled())
-      Diagnostics::Instance().RemoveCallback(m_diagnostics_callback_id);
   });
 }
 
@@ -1692,6 +1673,16 @@ void Debugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
       std::make_shared<CallbackLogHandler>(log_callback, baton);
 }
 
+void Debugger::CopyLogFilesToDirectory(const FileSpec &dir) {
+  for (auto &entry : m_stream_handlers) {
+    llvm::StringRef log_path = entry.first();
+    llvm::StringRef file_name = llvm::sys::path::filename(log_path);
+    FileSpec destination = dir.CopyByAppendingPathComponent(file_name);
+    // Best-effort: skip logs that can't be copied rather than aborting.
+    llvm::sys::fs::copy_file(log_path, destination.GetPath());
+  }
+}
+
 void Debugger::SetDestroyCallback(
     lldb_private::DebuggerDestroyCallback destroy_callback, void *baton) {
   std::lock_guard<std::mutex> guard(m_destroy_callback_mutex);
diff --git a/lldb/source/Utility/Diagnostics.cpp b/lldb/source/Utility/Diagnostics.cpp
index b2a08165dd6ca..175011783ead2 100644
--- a/lldb/source/Utility/Diagnostics.cpp
+++ b/lldb/source/Utility/Diagnostics.cpp
@@ -43,19 +43,6 @@ Diagnostics::Diagnostics() : m_log_handler(g_num_log_messages) {}
 
 Diagnostics::~Diagnostics() {}
 
-Diagnostics::CallbackID Diagnostics::AddCallback(Callback callback) {
-  std::lock_guard<std::mutex> guard(m_callbacks_mutex);
-  CallbackID id = m_callback_id++;
-  m_callbacks.emplace_back(id, callback);
-  return id;
-}
-
-void Diagnostics::RemoveCallback(CallbackID id) {
-  std::lock_guard<std::mutex> guard(m_callbacks_mutex);
-  llvm::erase_if(m_callbacks,
-                 [id](const CallbackEntry &e) { return e.id == id; });
-}
-
 bool Diagnostics::Dump(raw_ostream &stream) {
   Expected<FileSpec> diagnostics_dir = CreateUniqueDirectory();
   if (!diagnostics_dir) {
@@ -92,11 +79,6 @@ Error Diagnostics::Create(const FileSpec &dir) {
   if (Error err = DumpDiangosticsLog(dir))
     return err;
 
-  for (CallbackEntry e : m_callbacks) {
-    if (Error err = e.callback(dir))
-      return err;
-  }
-
   return Error::success();
 }
 

[bulbazord]

Looks ok to me. We can revive this system or add another one if needed.