https://github.com/chelcassanova updated https://github.com/llvm/llvm-project/pull/83069
>From f38204941062691bf3e3f6e57d8171a5c0258f77 Mon Sep 17 00:00:00 2001 From: Chelsea Cassanova <chelsea_cassan...@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 | 73 +++++++++++++++------- lldb/unittests/Core/ProgressReportTest.cpp | 57 +++++++++++++++++ 5 files changed, 156 insertions(+), 50 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..ffe21e9c15c80d 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,32 @@ 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; + m_progress_data.completed = m_progress_data.completed; ReportProgress(); + ProgressManager::Instance().Decrement(m_progress_data); } void Progress::Increment(uint64_t amount, @@ -47,13 +56,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 +72,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 +94,37 @@ 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. + if (!m_progress_category_map.contains(progress_data.title)) + 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) { + m_progress_category_map.erase(progress_data.title); + ReportProgress(progress_data); + } 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 when broadcasting to it since that + // bit doesn't need that information. + progress_data.details = ""; + Debugger::ReportProgress(progress_data.progress_id, progress_data.title, + progress_data.details, progress_data.completed, + progress_data.total, progress_data.debugger_id, + Debugger::eBroadcastBitProgressCategory); } 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"); +} _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
