[Lldb-commits] [lldb] [lldb][progress] Hook up new broadcast bit and progress manager (PR #83069)
Chelsea Cassanova via lldb-commits
lldb-commits at lists.llvm.org
Thu Feb 29 09:06:56 PST 2024
https://github.com/chelcassanova updated https://github.com/llvm/llvm-project/pull/83069
>From 59e3b1da1335261badeace24d6136d924cda6949 Mon Sep 17 00:00:00 2001
From: Chelsea Cassanova <chelsea_cassanova at apple.com>
Date: Tue, 20 Feb 2024 13:56:53 -0800
Subject: [PATCH] [lldb][progress] Hook up new broadcast bit and progress
manager
This commit adds the functionality to broadcast events using the
`Debugger::eBroadcastProgressCategory`
bit (https://github.com/llvm/llvm-project/pull/81169) by keeping track of these
reports with the `ProgressManager`
class (https://github.com/llvm/llvm-project/pull/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.
---
lldb/include/lldb/Core/Debugger.h | 10 ++-
lldb/include/lldb/Core/Progress.h | 47 +++++++----
lldb/source/Core/Debugger.cpp | 19 +++--
lldb/source/Core/Progress.cpp | 77 +++++++++++++-----
lldb/unittests/Core/ProgressReportTest.cpp | 93 +++++++++++++++++++++-
5 files changed, 195 insertions(+), 51 deletions(-)
diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index 6ba90eb6ed8fdf..b65ec1029ab24b 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.
///
@@ -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,
diff --git a/lldb/include/lldb/Core/Progress.h b/lldb/include/lldb/Core/Progress.h
index eb4d9f9d7af08e..450c0b439910f2 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,27 +98,36 @@ class Progress {
/// Used to indicate a non-deterministic progress report
static constexpr uint64_t kNonDeterministicTotal = UINT64_MAX;
+ /// Data belonging to this Progress event. This data is used by the Debugger
+ /// to broadcast the event and by the ProgressManager for bookkeeping.
+ struct ProgressData {
+ /// The title of the progress activity, also used as a category.
+ std::string title;
+ /// More specific information about the current file being displayed in the
+ /// report.
+ std::string details;
+ /// A unique integer identifier for progress reporting.
+ uint64_t progress_id;
+ /// How much work ([0...m_total]) that has been completed.
+ uint64_t completed;
+ /// Total amount of work, use a std::nullopt in the constructor for non
+ /// deterministic progress.
+ uint64_t 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> debugger_id;
+ };
+
private:
void ReportProgress();
static std::atomic<uint64_t> g_id;
- /// The title of the progress activity.
- std::string m_title;
- 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;
/// 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
@@ -130,13 +140,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();
+ 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..356758de1af9ff 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,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);
}
}
diff --git a/lldb/source/Core/Progress.cpp b/lldb/source/Core/Progress.cpp
index 9e8deb1ad4e731..ab30ea7f56d5d6 100644
--- a/lldb/source/Core/Progress.cpp
+++ b/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,24 +23,31 @@ 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_progress_data{title,
+ details,
+ ++g_id,
+ /*m_progress_data.completed=*/0,
+ Progress::kNonDeterministicTotal,
+ /*m_progress_data.debugger_id=*/std::nullopt} {
if (total)
- m_total = *total;
+ m_progress_data.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() {
// Make sure to always report progress completed when this object is
// destructed so it indicates the progress dialog/activity should go away.
std::lock_guard<std::mutex> guard(m_mutex);
- if (!m_completed)
- m_completed = m_total;
+ if (!m_progress_data.completed)
+ m_progress_data.completed = m_progress_data.total;
ReportProgress();
+ ProgressManager::Instance().Decrement(m_progress_data);
}
void Progress::Increment(uint64_t amount,
@@ -47,13 +55,14 @@ void Progress::Increment(uint64_t amount,
if (amount > 0) {
std::lock_guard<std::mutex> guard(m_mutex);
if (updated_detail)
- m_details = std::move(updated_detail.value());
+ m_progress_data.details = std::move(updated_detail.value());
// Watch out for unsigned overflow and make sure we don't increment too
- // much and exceed m_total.
- if (m_total && (amount > (m_total - m_completed)))
- m_completed = m_total;
+ // much and exceed the total.
+ if (m_progress_data.total &&
+ (amount > (m_progress_data.total - m_progress_data.completed)))
+ m_progress_data.completed = m_progress_data.total;
else
- m_completed += amount;
+ m_progress_data.completed += amount;
ReportProgress();
}
}
@@ -62,9 +71,11 @@ void Progress::ReportProgress() {
if (!m_complete) {
// 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);
+ m_complete = m_progress_data.completed == m_progress_data.total;
+ Debugger::ReportProgress(m_progress_data.progress_id, m_progress_data.title,
+ m_progress_data.details, m_progress_data.completed,
+ m_progress_data.total,
+ m_progress_data.debugger_id);
}
}
@@ -82,20 +93,42 @@ 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(progress_data);
+ m_progress_category_map.erase(progress_data.title);
+ } else {
+ --pos->second.first;
+ }
+}
+
+void ProgressManager::ReportProgress(Progress::ProgressData progress_data) {
+ // The category bit only keeps track of when progress report categories have
+ // started and ended, so clear the details and other fields when broadcasting
+ // to it since that bit doesn't need that information.
+
+ Debugger::ReportProgress(
+ m_progress_category_map[progress_data.title].second.progress_id,
+ progress_data.title, "", Progress::kNonDeterministicTotal,
+ Progress::kNonDeterministicTotal, progress_data.debugger_id,
+ Debugger::eBroadcastBitProgressCategory);
}
diff --git a/lldb/unittests/Core/ProgressReportTest.cpp b/lldb/unittests/Core/ProgressReportTest.cpp
index 559f3ef1ae46bf..98cbc475ce2835 100644
--- a/lldb/unittests/Core/ProgressReportTest.cpp
+++ b/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);
+}
More information about the lldb-commits
mailing list