[lldb][progress] Hook up new broadcast bit and progress manager (#83069)

This commit adds the functionality to broadcast events using the
`Debugger::eBroadcastProgressCategory`
bit (#81169) by keeping track
of these reports with the `ProgressManager`
class (#81319). The new bit is
used in such a way that it will only broadcast the initial and final
progress reports for specific progress categories that are managed by
the progress manager.

This commit also adds a new test to the progress report unit test that
checks that only the initial/final reports are broadcasted when using
the new bit.

Author: chelcassanova

lldb/include/lldb/Core/Debugger.h

@@ -593,6 +593,7 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
   friend class CommandInterpreter;
   friend class REPL;
   friend class Progress;
+  friend class ProgressManager;
 
   /// Report progress events.
   ///
@@ -623,10 +624,11 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
   ///   debugger identifier that this progress should be delivered to. If this
   ///   optional parameter does not have a value, the progress will be
   ///   delivered to all debuggers.
-  static void ReportProgress(uint64_t progress_id, std::string title,
-                             std::string details, uint64_t completed,
-                             uint64_t total,
-                             std::optional<lldb::user_id_t> debugger_id);
+  static void
+  ReportProgress(uint64_t progress_id, std::string title, std::string details,
+                 uint64_t completed, uint64_t total,
+                 std::optional<lldb::user_id_t> debugger_id,
+                 uint32_t progress_category_bit = eBroadcastBitProgress);
 
   static void ReportDiagnosticImpl(DiagnosticEventData::Type type,
                                    std::string message,

lldb/include/lldb/Core/Progress.h

@@ -9,10 +9,11 @@
 #ifndef LLDB_CORE_PROGRESS_H
 #define LLDB_CORE_PROGRESS_H
 
-#include "lldb/Utility/ConstString.h"
+#include "lldb/lldb-forward.h"
 #include "lldb/lldb-types.h"
 #include "llvm/ADT/StringMap.h"
 #include <atomic>
+#include <cstdint>
 #include <mutex>
 #include <optional>
 
@@ -100,27 +101,36 @@ class Progress {
   /// Used to indicate a non-deterministic progress report
   static constexpr uint64_t kNonDeterministicTotal = UINT64_MAX;
 
+  /// Data belonging to this Progress event that is used for bookkeeping by
+  /// ProgressManager.
+  struct ProgressData {
+    /// The title of the progress activity, also used as a category.
+    std::string title;
+    /// A unique integer identifier for progress reporting.
+    uint64_t progress_id;
+    /// The optional debugger ID to report progress to. If this has no value
+    /// then all debuggers will receive this event.
+    std::optional<lldb::user_id_t> debugger_id;
+  };
+
 private:
   void ReportProgress();
   static std::atomic<uint64_t> g_id;
-  /// The title of the progress activity.
-  std::string m_title;
+  /// More specific information about the current file being displayed in the
+  /// report.
   std::string m_details;
-  std::mutex m_mutex;
-  /// A unique integer identifier for progress reporting.
-  const uint64_t m_id;
   /// How much work ([0...m_total]) that has been completed.
   uint64_t m_completed;
   /// Total amount of work, use a std::nullopt in the constructor for non
   /// deterministic progress.
   uint64_t m_total;
-  /// The optional debugger ID to report progress to. If this has no value then
-  /// all debuggers will receive this event.
-  std::optional<lldb::user_id_t> m_debugger_id;
+  std::mutex m_mutex;
   /// Set to true when progress has been reported where m_completed == m_total
   /// to ensure that we don't send progress updates after progress has
   /// completed.
   bool m_complete = false;
+  /// Data needed by the debugger to broadcast a progress event.
+  ProgressData m_progress_data;
 };
 
 /// A class used to group progress reports by category. This is done by using a
@@ -133,13 +143,16 @@ class ProgressManager {
   ~ProgressManager();
 
   /// Control the refcount of the progress report category as needed.
-  void Increment(std::string category);
-  void Decrement(std::string category);
+  void Increment(const Progress::ProgressData &);
+  void Decrement(const Progress::ProgressData &);
 
   static ProgressManager &Instance();
 
+  static void ReportProgress(const Progress::ProgressData &);
+
 private:
-  llvm::StringMap<uint64_t> m_progress_category_map;
+  llvm::StringMap<std::pair<uint64_t, Progress::ProgressData>>
+      m_progress_category_map;
   std::mutex m_progress_map_mutex;
 };
 

lldb/source/Core/Debugger.cpp

@@ -15,6 +15,7 @@
 #include "lldb/Core/ModuleList.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Progress.h"
 #include "lldb/Core/StreamAsynchronousIO.h"
 #include "lldb/DataFormatters/DataVisualization.h"
 #include "lldb/Expression/REPL.h"
@@ -1433,13 +1434,14 @@ void Debugger::SetDestroyCallback(
 static void PrivateReportProgress(Debugger &debugger, uint64_t progress_id,
                                   std::string title, std::string details,
                                   uint64_t completed, uint64_t total,
-                                  bool is_debugger_specific) {
+                                  bool is_debugger_specific,
+                                  uint32_t progress_broadcast_bit) {
   // Only deliver progress events if we have any progress listeners.
-  const uint32_t event_type = Debugger::eBroadcastBitProgress;
-  if (!debugger.GetBroadcaster().EventTypeHasListeners(event_type))
+  if (!debugger.GetBroadcaster().EventTypeHasListeners(progress_broadcast_bit))
     return;
+
   EventSP event_sp(new Event(
-      event_type,
+      progress_broadcast_bit,
       new ProgressEventData(progress_id, std::move(title), std::move(details),
                             completed, total, is_debugger_specific)));
   debugger.GetBroadcaster().BroadcastEvent(event_sp);
@@ -1448,7 +1450,8 @@ static void PrivateReportProgress(Debugger &debugger, uint64_t progress_id,
 void Debugger::ReportProgress(uint64_t progress_id, std::string title,
                               std::string details, uint64_t completed,
                               uint64_t total,
-                              std::optional<lldb::user_id_t> debugger_id) {
+                              std::optional<lldb::user_id_t> debugger_id,
+                              uint32_t progress_category_bit) {
   // Check if this progress is for a specific debugger.
   if (debugger_id) {
     // It is debugger specific, grab it and deliver the event if the debugger
@@ -1457,7 +1460,8 @@ void Debugger::ReportProgress(uint64_t progress_id, std::string title,
     if (debugger_sp)
       PrivateReportProgress(*debugger_sp, progress_id, std::move(title),
                             std::move(details), completed, total,
-                            /*is_debugger_specific*/ true);
+                            /*is_debugger_specific*/ true,
+                            progress_category_bit);
     return;
   }
   // The progress event is not debugger specific, iterate over all debuggers
@@ -1467,7 +1471,8 @@ void Debugger::ReportProgress(uint64_t progress_id, std::string title,
     DebuggerList::iterator pos, end = g_debugger_list_ptr->end();
     for (pos = g_debugger_list_ptr->begin(); pos != end; ++pos)
       PrivateReportProgress(*(*pos), progress_id, title, details, completed,
-                            total, /*is_debugger_specific*/ false);
+                            total, /*is_debugger_specific*/ false,
+                            progress_category_bit);
   }
 }
 

lldb/source/Core/Progress.cpp

@@ -11,6 +11,7 @@
 #include "lldb/Core/Debugger.h"
 #include "lldb/Utility/StreamString.h"
 
+#include <cstdint>
 #include <mutex>
 #include <optional>
 
@@ -22,15 +23,19 @@ std::atomic<uint64_t> Progress::g_id(0);
 Progress::Progress(std::string title, std::string details,
                    std::optional<uint64_t> total,
                    lldb_private::Debugger *debugger)
-    : m_title(title), m_details(details), m_id(++g_id), m_completed(0),
-      m_total(Progress::kNonDeterministicTotal) {
+    : m_details(details), m_completed(0),
+      m_total(Progress::kNonDeterministicTotal),
+      m_progress_data{title, ++g_id,
+                      /*m_progress_data.debugger_id=*/std::nullopt} {
   if (total)
     m_total = *total;
 
   if (debugger)
-    m_debugger_id = debugger->GetID();
+    m_progress_data.debugger_id = debugger->GetID();
+
   std::lock_guard<std::mutex> guard(m_mutex);
   ReportProgress();
+  ProgressManager::Instance().Increment(m_progress_data);
 }
 
 Progress::~Progress() {
@@ -40,6 +45,7 @@ Progress::~Progress() {
   if (!m_completed)
     m_completed = m_total;
   ReportProgress();
+  ProgressManager::Instance().Decrement(m_progress_data);
 }
 
 void Progress::Increment(uint64_t amount,
@@ -49,7 +55,7 @@ void Progress::Increment(uint64_t amount,
     if (updated_detail)
       m_details = std::move(updated_detail.value());
     // Watch out for unsigned overflow and make sure we don't increment too
-    // much and exceed m_total.
+    // much and exceed the total.
     if (m_total && (amount > (m_total - m_completed)))
       m_completed = m_total;
     else
@@ -63,8 +69,9 @@ void Progress::ReportProgress() {
     // Make sure we only send one notification that indicates the progress is
     // complete
     m_complete = m_completed == m_total;
-    Debugger::ReportProgress(m_id, m_title, m_details, m_completed, m_total,
-                             m_debugger_id);
+    Debugger::ReportProgress(m_progress_data.progress_id, m_progress_data.title,
+                             m_details, m_completed, m_total,
+                             m_progress_data.debugger_id);
   }
 }
 
@@ -82,20 +89,41 @@ ProgressManager &ProgressManager::Instance() {
   return *g_progress_manager;
 }
 
-void ProgressManager::Increment(std::string title) {
+void ProgressManager::Increment(const Progress::ProgressData &progress_data) {
   std::lock_guard<std::mutex> lock(m_progress_map_mutex);
-  m_progress_category_map[title]++;
+  // If the current category exists in the map then it is not an initial report,
+  // therefore don't broadcast to the category bit. Also, store the current
+  // progress data in the map so that we have a note of the ID used for the
+  // initial progress report.
+  if (!m_progress_category_map.contains(progress_data.title)) {
+    m_progress_category_map[progress_data.title].second = progress_data;
+    ReportProgress(progress_data);
+  }
+  m_progress_category_map[progress_data.title].first++;
 }
 
-void ProgressManager::Decrement(std::string title) {
+void ProgressManager::Decrement(const Progress::ProgressData &progress_data) {
   std::lock_guard<std::mutex> lock(m_progress_map_mutex);
-  auto pos = m_progress_category_map.find(title);
+  auto pos = m_progress_category_map.find(progress_data.title);
 
   if (pos == m_progress_category_map.end())
     return;
 
-  if (pos->second <= 1)
-    m_progress_category_map.erase(title);
-  else
-    --pos->second;
+  if (pos->second.first <= 1) {
+    ReportProgress(pos->second.second);
+    m_progress_category_map.erase(progress_data.title);
+  } else {
+    --pos->second.first;
+  }
+}
+
+void ProgressManager::ReportProgress(
+    const Progress::ProgressData &progress_data) {
+  // The category bit only keeps track of when progress report categories have
+  // started and ended, so clear the details and reset other fields when
+  // broadcasting to it since that bit doesn't need that information.
+  Debugger::ReportProgress(
+      progress_data.progress_id, progress_data.title, "",
+      Progress::kNonDeterministicTotal, Progress::kNonDeterministicTotal,
+      progress_data.debugger_id, Debugger::eBroadcastBitProgressCategory);
 }

lldb/unittests/Core/ProgressReportTest.cpp

@@ -16,8 +16,8 @@
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Utility/Listener.h"
 #include "gtest/gtest.h"
+#include <memory>
 #include <mutex>
-#include <thread>
 
 using namespace lldb;
 using namespace lldb_private;
@@ -126,3 +126,94 @@ TEST_F(ProgressReportTest, TestReportCreation) {
   ASSERT_FALSE(data->IsFinite());
   ASSERT_EQ(data->GetMessage(), "Progress report 1: Starting report 1");
 }
+
+TEST_F(ProgressReportTest, TestProgressManager) {
+  std::chrono::milliseconds timeout(100);
+
+  // Set up the debugger, make sure that was done properly.
+  ArchSpec arch("x86_64-apple-macosx-");
+  Platform::SetHostPlatform(PlatformRemoteMacOSX::CreateInstance(true, &arch));
+
+  DebuggerSP debugger_sp = Debugger::CreateInstance();
+  ASSERT_TRUE(debugger_sp);
+
+  // Get the debugger's broadcaster.
+  Broadcaster &broadcaster = debugger_sp->GetBroadcaster();
+
+  // Create a listener, make sure it can receive events and that it's
+  // listening to the correct broadcast bit.
+  ListenerSP listener_sp = Listener::MakeListener("progress-category-listener");
+
+  listener_sp->StartListeningForEvents(&broadcaster,
+                                       Debugger::eBroadcastBitProgressCategory);
+  EXPECT_TRUE(broadcaster.EventTypeHasListeners(
+      Debugger::eBroadcastBitProgressCategory));
+
+  EventSP event_sp;
+  const ProgressEventData *data;
+
+  // Create three progress events with the same category then try to pop 2
+  // events from the queue in a row before the progress reports are destroyed.
+  // Since only 1 event should've been broadcast for this category, the second
+  // GetEvent() call should return false.
+  {
+    Progress progress1("Progress report 1", "Starting report 1");
+    Progress progress2("Progress report 1", "Starting report 2");
+    Progress progress3("Progress report 1", "Starting report 3");
+    EXPECT_TRUE(listener_sp->GetEvent(event_sp, timeout));
+    EXPECT_FALSE(listener_sp->GetEvent(event_sp, timeout));
+  }
+
+  data = ProgressEventData::GetEventDataFromEvent(event_sp.get());
+
+  ASSERT_EQ(data->GetDetails(), "");
+  ASSERT_FALSE(data->IsFinite());
+  ASSERT_TRUE(data->GetCompleted());
+  ASSERT_EQ(data->GetTotal(), Progress::kNonDeterministicTotal);
+  ASSERT_EQ(data->GetMessage(), "Progress report 1");
+
+  // Pop another event from the queue, this should be the event for the final
+  // report for this category.
+  EXPECT_TRUE(listener_sp->GetEvent(event_sp, timeout));
+
+  data = ProgressEventData::GetEventDataFromEvent(event_sp.get());
+  ASSERT_EQ(data->GetDetails(), "");
+  ASSERT_FALSE(data->IsFinite());
+  ASSERT_TRUE(data->GetCompleted());
+  ASSERT_EQ(data->GetTotal(), Progress::kNonDeterministicTotal);
+  ASSERT_EQ(data->GetMessage(), "Progress report 1");
+
+  // Create two progress reports of the same category that overlap with each
+  // other. Here we want to ensure that the ID broadcasted for the initial and
+  // final reports for this category are the same.
+  std::unique_ptr<Progress> overlap_progress1 =
+      std::make_unique<Progress>("Overlapping report 1", "Starting report 1");
+  std::unique_ptr<Progress> overlap_progress2 =
+      std::make_unique<Progress>("Overlapping report 1", "Starting report 2");
+  overlap_progress1.reset();
+
+  EXPECT_TRUE(listener_sp->GetEvent(event_sp, timeout));
+  data = ProgressEventData::GetEventDataFromEvent(event_sp.get());
+  // Get the ID used in the first report for this category.
+  uint64_t expected_progress_id = data->GetID();
+
+  ASSERT_EQ(data->GetDetails(), "");
+  ASSERT_FALSE(data->IsFinite());
+  ASSERT_TRUE(data->GetCompleted());
+  ASSERT_EQ(data->GetTotal(), Progress::kNonDeterministicTotal);
+  ASSERT_EQ(data->GetMessage(), "Overlapping report 1");
+
+  overlap_progress2.reset();
+
+  EXPECT_TRUE(listener_sp->GetEvent(event_sp, timeout));
+  data = ProgressEventData::GetEventDataFromEvent(event_sp.get());
+
+  ASSERT_EQ(data->GetDetails(), "");
+  ASSERT_FALSE(data->IsFinite());
+  ASSERT_TRUE(data->GetCompleted());
+  ASSERT_EQ(data->GetTotal(), Progress::kNonDeterministicTotal);
+  ASSERT_EQ(data->GetMessage(), "Overlapping report 1");
+  // The progress ID for the final report should be the same as that for the
+  // initial report.
+  ASSERT_EQ(data->GetID(), expected_progress_id);
+}