https://github.com/chelcassanova updated https://github.com/llvm/llvm-project/pull/83069
>From 2cef4a29f0105847fa11b7f6a6ee063184fb838a 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 | 43 +++++++++++----- lldb/source/Core/Debugger.cpp | 25 +++++++--- lldb/source/Core/Progress.cpp | 40 ++++++++++++--- lldb/unittests/Core/ProgressReportTest.cpp | 57 ++++++++++++++++++++++ 5 files changed, 145 insertions(+), 30 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..7554d4fe2b77e9 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,44 @@ 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 + /// the progress manager. + struct ProgressData { + /// The title of the progress activity, also used as a category to group + /// reports. + 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: + friend class Debugger; 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 +148,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(Progress::ProgressData); + void Decrement(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..de0981a5da7204 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,17 @@ 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; + 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 +1453,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 +1463,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 +1474,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 +1883,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 +1938,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..cc1803bc3a6cd0 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> @@ -29,8 +30,12 @@ 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}; std::lock_guard<std::mutex> guard(m_mutex); ReportProgress(); + ProgressManager::Instance().Increment(m_progress_data); } Progress::~Progress() { @@ -39,7 +44,9 @@ Progress::~Progress() { std::lock_guard<std::mutex> guard(m_mutex); if (!m_completed) m_completed = m_total; + m_progress_data.completed = m_completed; ReportProgress(); + ProgressManager::Instance().Decrement(m_progress_data); } void Progress::Increment(uint64_t amount, @@ -82,20 +89,37 @@ 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]++; + // 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(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
