https://github.com/chelcassanova created https://github.com/llvm/llvm-project/pull/83069
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. >From c27cab4a6fe067aeac6864e7262b2c57a53f2d0c 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 | 4 +- lldb/include/lldb/Core/Progress.h | 41 ++++++++++++++-- lldb/source/Core/Debugger.cpp | 38 ++++++++++++--- lldb/source/Core/Progress.cpp | 47 +++++++++++++----- lldb/unittests/Core/ProgressReportTest.cpp | 57 ++++++++++++++++++++++ 5 files changed, 164 insertions(+), 23 deletions(-) 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"); +} _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
