[lldb] Add the ability to remove diagnostic callbacks

[JDevlieghere]

Summary

Add the ability to remove diagnostic callbacks.

[mib]

LGTM! Is there a simple way to test this ?

lldb/source/Utility/Diagnostics.cpp

55–58

nit: may be use llvm::remove_if instead ?

[JDevlieghere]

In D143548#4113687, @mib wrote:

LGTM! Is there a simple way to test this ?

I considered a unit test, but that would require making everything protected and would basically boil down to testing the implementation of the underlying container, but I can add it if you think it's worthwhile. I think a better way to test this is through an integration test that exercises this code path. D143548 would indirectly test it (it doesn't crash but I assume ASAN would have caught it).

[bulbazord]

LGTM

lldb/include/lldb/Utility/Diagnostics.h

public:
   /// @{
   bool Dump(llvm::raw_ostream &stream);
   bool Dump(llvm::raw_ostream &stream, const FileSpec &dir);
   /// @}
 
   void Report(llvm::StringRef message);
 
   using Callback = std::function<llvm::Error(const FileSpec &)>;
+  using CallbackID = uint64_t;
 
-  void AddCallback(Callback callback);
+  CallbackID AddCallback(Callback callback);
+  void RemoveCallback(CallbackID id);
 
   static Diagnostics &Instance();
 
   static bool Enabled();
   static void Initialize();
   static void Terminate();
 
   /// Create a unique diagnostic directory.
   static llvm::Expected<FileSpec> CreateUniqueDirectory();
 
 private:
   static std::optional<Diagnostics> &InstanceImpl();
 
   llvm::Error DumpDiangosticsLog(const FileSpec &dir) const;
 
   RotatingLogHandler m_log_handler;
 
-  llvm::SmallVector<Callback, 4> m_callbacks;
+  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
 
 #endif

lldb/source/Utility/Diagnostics.cpp

 }
 
 Diagnostics &Diagnostics::Instance() { return *InstanceImpl(); }
 
 Diagnostics::Diagnostics() : m_log_handler(g_num_log_messages) {}
 
 Diagnostics::~Diagnostics() {}
 
-void Diagnostics::AddCallback(Callback callback) {
+Diagnostics::CallbackID Diagnostics::AddCallback(Callback callback) {
   std::lock_guard<std::mutex> guard(m_callbacks_mutex);
-  m_callbacks.push_back(callback);
+  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);
+  m_callbacks.erase(
+      std::remove_if(m_callbacks.begin(), m_callbacks.end(),
+                     [id](const CallbackEntry &e) { return e.id == id; }),
+      m_callbacks.end());
 }
 
 bool Diagnostics::Dump(raw_ostream &stream) {
   Expected<FileSpec> diagnostics_dir = CreateUniqueDirectory();
   if (!diagnostics_dir) {
     stream << "unable to create diagnostic dir: "
            << toString(diagnostics_dir.takeError()) << '\n';
     return false;
if (ec)
     return errorCodeToError(ec);
   return FileSpec(diagnostics_dir.str());
 }
 
 Error Diagnostics::Create(const FileSpec &dir) {
   if (Error err = DumpDiangosticsLog(dir))
     return err;
 
-  for (Callback c : m_callbacks) {
-    if (Error err = c(dir))
+  for (CallbackEntry e : m_callbacks) {
+    if (Error err = e.callback(dir))
       return err;
   }
 
   return Error::success();
 }
 
 llvm::Error Diagnostics::DumpDiangosticsLog(const FileSpec &dir) const {
   FileSpec log_file = dir.CopyByAppendingPathComponent("diagnostics.log");