[lldb][progress] Hook up new broadcast bit and progress manager

[chelcassanova]

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.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Chelsea Cassanova (chelcassanova)

Changes

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.

---

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

5 Files Affected:

• (modified) lldb/include/lldb/Core/Debugger.h (+3-1)
• (modified) lldb/include/lldb/Core/Progress.h (+36-5)
• (modified) lldb/source/Core/Debugger.cpp (+32-6)
• (modified) lldb/source/Core/Progress.cpp (+36-11)
• (modified) lldb/unittests/Core/ProgressReportTest.cpp (+57)

diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index 6ba90eb6ed8fdf..714ca83b890d87 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/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.
   ///
@@ -626,7 +627,8 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
   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);
+                             std::optional<lldb::user_id_t> debugger_id,
+                             uint32_t progress_category_bit);
 
   static void ReportDiagnosticImpl(DiagnosticEventData::Type type,
                                    std::string message,
diff --git a/lldb/include/lldb/Core/Progress.h b/lldb/include/lldb/Core/Progress.h
index eb4d9f9d7af08e..434a155ca7590e 100644
--- a/lldb/include/lldb/Core/Progress.h
+++ b/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>
 
@@ -97,12 +98,32 @@ class Progress {
   /// Used to indicate a non-deterministic progress report
   static constexpr uint64_t kNonDeterministicTotal = UINT64_MAX;
 
+  /// Use a struct to send data from a Progress object to
+  /// ProgressManager::ReportProgress. In addition to the Progress member fields
+  /// this also passes the debugger progress broadcast bit. Using the progress
+  /// category bit indicates that the given progress report is the initial or
+  /// final report for its category.
+  struct ProgressData {
+    std::string title;
+    std::string details;
+    uint64_t progress_id;
+    uint64_t completed;
+    uint64_t total;
+    std::optional<lldb::user_id_t> debugger_id;
+    uint32_t progress_broadcast_bit;
+  };
+
 private:
+  friend class Debugger;
   void ReportProgress();
   static std::atomic<uint64_t> g_id;
-  /// The title of the progress activity.
+  /// The title of the progress activity, also used as a category to group
+  /// reports.
   std::string m_title;
+  /// More specific information about the current file being displayed in the
+  /// report.
   std::string m_details;
+  /// Mutex for synchronization.
   std::mutex m_mutex;
   /// A unique integer identifier for progress reporting.
   const uint64_t m_id;
@@ -118,6 +139,8 @@ class Progress {
   /// 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
@@ -130,13 +153,21 @@ 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(Progress::ProgressData);
+  void Decrement(Progress::ProgressData);
 
   static ProgressManager &Instance();
 
+  llvm::StringMap<std::pair<uint64_t, Progress::ProgressData>>
+  GetProgressCategoryMap() {
+    return m_progress_category_map;
+  }
+
+  void ReportProgress(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;
 };
 
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index 97311b4716ac2f..cd0ce3a9558ce6 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/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,11 +1434,30 @@ 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))
+  const uint32_t category_event_type = Debugger::eBroadcastBitProgressCategory;
+  if (!debugger.GetBroadcaster().EventTypeHasListeners(event_type |
+                                                       category_event_type))
     return;
+
+  if (debugger.GetBroadcaster().EventTypeHasListeners(category_event_type)) {
+    ProgressManager &progress_manager = ProgressManager::Instance();
+    auto map_refcount = progress_manager.GetProgressCategoryMap().lookup(title);
+
+    // Only broadcast the event to the progress category bit if it's an initial
+    // or final report for that category. Since we're broadcasting for the
+    // category specifically, clear the details.
+    if (progress_broadcast_bit == Debugger::eBroadcastBitProgressCategory) {
+      EventSP event_sp(new Event(
+          category_event_type,
+          new ProgressEventData(progress_id, std::move(title), "", completed,
+                                total, is_debugger_specific)));
+      debugger.GetBroadcaster().BroadcastEvent(event_sp);
+    }
+  }
   EventSP event_sp(new Event(
       event_type,
       new ProgressEventData(progress_id, std::move(title), std::move(details),
@@ -1448,7 +1468,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 +1478,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 +1489,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);
   }
 }
 
@@ -1875,7 +1898,8 @@ lldb::thread_result_t Debugger::DefaultEventHandler() {
 
   listener_sp->StartListeningForEvents(
       &m_broadcaster, eBroadcastBitProgress | eBroadcastBitWarning |
-                          eBroadcastBitError | eBroadcastSymbolChange);
+                          eBroadcastBitError | eBroadcastSymbolChange |
+                          eBroadcastBitProgressCategory);
 
   // Let the thread that spawned us know that we have started up and that we
   // are now listening to all required events so no events get missed
@@ -1929,6 +1953,8 @@ lldb::thread_result_t Debugger::DefaultEventHandler() {
           } else if (broadcaster == &m_broadcaster) {
             if (event_type & Debugger::eBroadcastBitProgress)
               HandleProgressEvent(event_sp);
+            else if (event_type & Debugger::eBroadcastBitProgressCategory)
+              HandleProgressEvent(event_sp);
             else if (event_type & Debugger::eBroadcastBitWarning)
               HandleDiagnosticEvent(event_sp);
             else if (event_type & Debugger::eBroadcastBitError)
diff --git a/lldb/source/Core/Progress.cpp b/lldb/source/Core/Progress.cpp
index 9e8deb1ad4e731..fb698050799230 100644
--- a/lldb/source/Core/Progress.cpp
+++ b/lldb/source/Core/Progress.cpp
@@ -29,8 +29,16 @@ Progress::Progress(std::string title, std::string details,
 
   if (debugger)
     m_debugger_id = debugger->GetID();
+
+  m_progress_data = {m_title,
+                     m_details,
+                     m_id,
+                     m_completed,
+                     m_total,
+                     m_debugger_id,
+                     Debugger::eBroadcastBitProgress};
   std::lock_guard<std::mutex> guard(m_mutex);
-  ReportProgress();
+  ProgressManager::Instance().Increment(m_progress_data);
 }
 
 Progress::~Progress() {
@@ -39,7 +47,8 @@ Progress::~Progress() {
   std::lock_guard<std::mutex> guard(m_mutex);
   if (!m_completed)
     m_completed = m_total;
-  ReportProgress();
+  m_progress_data.completed = m_completed;
+  ProgressManager::Instance().Decrement(m_progress_data);
 }
 
 void Progress::Increment(uint64_t amount,
@@ -64,7 +73,7 @@ void Progress::ReportProgress() {
     // complete
     m_complete = m_completed == m_total;
     Debugger::ReportProgress(m_id, m_title, m_details, m_completed, m_total,
-                             m_debugger_id);
+                             m_debugger_id, Debugger::eBroadcastBitProgress);
   }
 }
 
@@ -82,20 +91,36 @@ ProgressManager &ProgressManager::Instance() {
   return *g_progress_manager;
 }
 
-void ProgressManager::Increment(std::string title) {
+void ProgressManager::Increment(Progress::ProgressData progress_data) {
   std::lock_guard<std::mutex> lock(m_progress_map_mutex);
-  m_progress_category_map[title]++;
+  progress_data.progress_broadcast_bit =
+      m_progress_category_map.contains(progress_data.title)
+          ? Debugger::eBroadcastBitProgress
+          : Debugger::eBroadcastBitProgressCategory;
+  ReportProgress(progress_data);
+  m_progress_category_map[progress_data.title].first++;
 }
 
-void ProgressManager::Decrement(std::string title) {
+void ProgressManager::Decrement(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) {
+    progress_data.progress_broadcast_bit =
+        Debugger::eBroadcastBitProgressCategory;
+    m_progress_category_map.erase(progress_data.title);
+  } else
+    --pos->second.first;
+
+  ReportProgress(progress_data);
+}
+
+void ProgressManager::ReportProgress(Progress::ProgressData progress_data) {
+  Debugger::ReportProgress(progress_data.progress_id, progress_data.title,
+                           progress_data.details, progress_data.completed,
+                           progress_data.total, progress_data.debugger_id,
+                           progress_data.progress_broadcast_bit);
 }
diff --git a/lldb/unittests/Core/ProgressReportTest.cpp b/lldb/unittests/Core/ProgressReportTest.cpp
index 559f3ef1ae46bf..d0ad731aab8f21 100644
--- a/lldb/unittests/Core/ProgressReportTest.cpp
+++ b/lldb/unittests/Core/ProgressReportTest.cpp
@@ -126,3 +126,60 @@ 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_FALSE(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");
+}

[JDevlieghere]

I think this is going in the right direction but I think the interaction between the ProgressManager and the Debugger needs to change:

At a high level I'm expecting:

1. Progress object is created
2. Progress is added to the ProgressManager
3. Debugger::ReportProgress is called with (eBroadcastBitProgress | eBroadcastBitProgressCategory)
4. Incremental updates call directly into Debugger::ReportProgress without going through the manager, with just Debugger::ReportProgress.
5. When progress is complete, we go through the progress manager again and report the final progress to both broadcast bits: eBroadcastBitProgress | eBroadcastBitProgressCategory

[JDevlieghere]

should this default to eBroadcastBitProgress?

[JDevlieghere]

I think the first sentence isn't entirely accurate. The struct is used to store information about the progress event in the progress manager?

[chelcassanova]

Yes, this can be generalized more since we use the struct for more than just the call to ReportProgress.

[JDevlieghere]

Given that you store the ProgressData, did you mean to document the fields in the new Struct and remove these?

[JDevlieghere]

I think it would be better from a layering perspective to keep PrivateReportProgress "dumb" and pass it the right arguments from the ProgressManager. The map held by the progress manager should be a private implementation detail. By letting the debugger mess with it you lose the ability to synchronize access to it. When we do the timeouts, this isn't going to work.

I expect this function to simply broadcast to whichever broadcast bit is set.

[chelcassanova]

So IIUC, instead of passing the broadcast bit as an argument we just set it in the progress manager and access it from there in the Debugger?

[JDevlieghere]

Not really. I'm saying that I think we don't need to store it at all, because its implied in when we call DebuggerReportProgress.

Can ProgressManager call ReportProgress(bit = eBroadcastBitProgress | eBroadcastBitProgressCategory) for the first and last update, and every other time call `ReportProgress(bit = eBroadcastBitProgress) every other time?

[chelcassanova]

So that instead of having it be a member of the struct or the class we just call Debugger::ReportProgress(<other information>, eBroadcastBitProgress | eBroadcastBitProgressCategory) and set the bit as needed from the progress manager itself?

[chelcassanova]

Such that we set progress_category_bit from the progress manager and broadcast to that as such:

EventSP event_sp(new Event(
          progress_broadcast_bit,
          new ProgressEventData(progress_id, std::move(title), "", completed,
                                total, is_debugger_specific)));
      debugger.GetBroadcaster().BroadcastEvent(event_sp);

[chelcassanova]

Also looking back at PrivateReportProgress, the variables for the instance and refcount are leftover from a previous implementation that I had, they shouldn't be here anymore.

[JDevlieghere]

I don't think the progress_broadcast_bit should be stored in the struct. The bit is a property of the progress manager, which will set the bit for the first and last progress event.

[JDevlieghere]

You only have to check the progress_broadcast_bit.

[JDevlieghere]

Now we're going to handle the same event twice, once when broadcast as eBroadcastBitProgress and once when broadcast as eBroadcastBitProgressCategory. In the default event handler we want to see all the individual updates and as that's a superset, we should not handle eBroadcastBitProgressCategory.

[JDevlieghere]

Did clang-format format this like this? Interesting...

Anyway, you should initialize this in the constructors initializer list (and get rid of m_title etc) as per my previous comment.

[chelcassanova]

I'm removing this anyways but yeah not sure what clang-format was thinking here.

[JDevlieghere]

See my comment below why we shouldn't listen for eBroadcastBitProgressCategory in the default event handler.

[JDevlieghere]

You're still storing the same fields as in ProgressData. You should only have the latter and initialize the progress data in the constructor.

[JDevlieghere]

LGTM with the nit addressed.

[JDevlieghere]

Suggestion:

/// Data data belonging to this Progress event. This data is used by the Debugger to broadcast the event and by the ProgressManager for bookkeeping.

[JDevlieghere]

This should take a const ref to avoid the copy const Progress::ProgressData& progress_data. If we'd unconditionally store it in the map I'd say the copy is fine as you'd be able to move it into the map, but I'm assuming most of the time we're only using the title to do the lookup.

[chelcassanova]

Yeah, the title is used for map queries but the data itself isn't stored in the map, it's passed on to ProgressManager::ReportProgress. If we change it to a const ref (const Progress::ProgressData& progress_data) here then we probably also want to do this for Decrement since it uses the map in a similar way.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[JDevlieghere]

LGTM!