[LLDB] Add external progress bit category

[Jlalond]

As feedback on #119052, it was recommended I add a new bit to delineate internal and external progress events. This patch adds this new category, and sets up Progress.h to support external events via SBProgress.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jacob Lalonde (Jlalond)

Changes

As feedback on #119052, it was recommended I add a new bit to delineate internal and external progress events. This patch adds this new category, and sets up Progress.h to support external events via SBProgress.

---

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

3 Files Affected:

• (modified) lldb/include/lldb/Core/Progress.h (+11-1)
• (modified) lldb/include/lldb/lldb-enumerations.h (+7-6)
• (modified) lldb/source/Core/Progress.cpp (+10-3)

diff --git a/lldb/include/lldb/Core/Progress.h b/lldb/include/lldb/Core/Progress.h
index f6cea282842e1c..1d56703a9f586a 100644
--- a/lldb/include/lldb/Core/Progress.h
+++ b/lldb/include/lldb/Core/Progress.h
@@ -21,6 +21,12 @@
 
 namespace lldb_private {
 
+/// Enum to indicate the origin of a progress event, internal or external.
+enum class ProgressOrigin : uint8_t {
+  eLLDBInternal = 0,
+  eExternal = 1,
+};
+
 /// A Progress indicator helper class.
 ///
 /// Any potentially long running sections of code in LLDB should report
@@ -83,7 +89,8 @@ class Progress {
   Progress(std::string title, std::string details = {},
            std::optional<uint64_t> total = std::nullopt,
            lldb_private::Debugger *debugger = nullptr,
-           Timeout<std::nano> minimum_report_time = std::nullopt);
+           Timeout<std::nano> minimum_report_time = std::nullopt,
+           ProgressOrigin origin = ProgressOrigin::eLLDBInternal);
 
   /// Destroy the progress object.
   ///
@@ -149,6 +156,9 @@ class Progress {
 
   /// The "completed" value of the last reported event.
   std::optional<uint64_t> m_prev_completed;
+
+  /// The origin of this progress event.
+  ProgressOrigin m_origin;
 };
 
 /// A class used to group progress reports by category. This is done by using a
diff --git a/lldb/include/lldb/lldb-enumerations.h b/lldb/include/lldb/lldb-enumerations.h
index 0094fcd596fdf7..aa9673feb1a586 100644
--- a/lldb/include/lldb/lldb-enumerations.h
+++ b/lldb/include/lldb/lldb-enumerations.h
@@ -195,10 +195,10 @@ enum Format {
                          ///< character arrays that can contain non printable
                          ///< characters
   eFormatAddressInfo,    ///< Describe what an address points to (func + offset
-                      ///< with file/line, symbol + offset, data, etc)
-  eFormatHexFloat,    ///< ISO C99 hex float string
-  eFormatInstruction, ///< Disassemble an opcode
-  eFormatVoid,        ///< Do not print this
+                         ///< with file/line, symbol + offset, data, etc)
+  eFormatHexFloat,       ///< ISO C99 hex float string
+  eFormatInstruction,    ///< Disassemble an opcode
+  eFormatVoid,           ///< Do not print this
   eFormatUnicode8,
   kNumFormats
 };
@@ -302,7 +302,7 @@ enum ConnectionStatus {
   eConnectionStatusNoConnection,   ///< No connection
   eConnectionStatusLostConnection, ///< Lost connection while connected to a
                                    ///< valid connection
-  eConnectionStatusInterrupted ///< Interrupted read
+  eConnectionStatusInterrupted     ///< Interrupted read
 };
 
 enum ErrorType {
@@ -1094,7 +1094,7 @@ enum PathType {
   ePathTypeGlobalLLDBTempSystemDir, ///< The LLDB temp directory for this
                                     ///< system, NOT cleaned up on a process
                                     ///< exit.
-  ePathTypeClangDir ///< Find path to Clang builtin headers
+  ePathTypeClangDir                 ///< Find path to Clang builtin headers
 };
 
 /// Kind of member function.
@@ -1357,6 +1357,7 @@ enum DebuggerBroadcastBit {
   eBroadcastBitError = (1 << 2),
   eBroadcastSymbolChange = (1 << 3),
   eBroadcastBitProgressCategory = (1 << 4),
+  eBroadcastBitExternalProgressCategory = (1 << 5),
 };
 
 /// Used for expressing severity in logs and diagnostics.
diff --git a/lldb/source/Core/Progress.cpp b/lldb/source/Core/Progress.cpp
index ed8dfb85639b71..e3161d79275693 100644
--- a/lldb/source/Core/Progress.cpp
+++ b/lldb/source/Core/Progress.cpp
@@ -28,7 +28,8 @@ static llvm::ManagedStatic<llvm::SignpostEmitter> g_progress_signposts;
 Progress::Progress(std::string title, std::string details,
                    std::optional<uint64_t> total,
                    lldb_private::Debugger *debugger,
-                   Timeout<std::nano> minimum_report_time)
+                   Timeout<std::nano> minimum_report_time,
+                   ProgressOrigin origin)
     : m_total(total.value_or(Progress::kNonDeterministicTotal)),
       m_minimum_report_time(minimum_report_time),
       m_progress_data{title, ++g_id,
@@ -38,7 +39,7 @@ Progress::Progress(std::string title, std::string details,
           std::chrono::nanoseconds(
               std::chrono::steady_clock::now().time_since_epoch())
               .count()),
-      m_details(std::move(details)) {
+      m_details(std::move(details)), m_origin(origin) {
   std::lock_guard<std::mutex> guard(m_mutex);
   ReportProgress();
 
@@ -106,9 +107,15 @@ void Progress::ReportProgress() {
   if (completed < m_prev_completed)
     return; // An overflow in the m_completed counter. Just ignore these events.
 
+  // Change the category bit if we're an internal or external progress.
+  uint32_t progress_category_bit =
+      m_origin == ProgressOrigin::eExternal
+          ? lldb::eBroadcastBitExternalProgressCategory
+          : lldb::eBroadcastBitProgressCategory;
+
   Debugger::ReportProgress(m_progress_data.progress_id, m_progress_data.title,
                            m_details, completed, m_total,
-                           m_progress_data.debugger_id);
+                           m_progress_data.debugger_id, progress_category_bit);
   m_prev_completed = completed;
 }
 

[Jlalond]

@JDevlieghere I'm hoping I did this right, additionally any preference on where the enum should live? I debated on enumerations and just make a generic 'eventorigin' enumeration that we could reuse for events in the future. But I figured that was reaching.

[JDevlieghere]

This is wrong, it will report all progress events as aggregated by category. You'll need to check both eBroadcastBitProgress and eBroadcastBitProgressCategory and pick the right external variant.

[Jlalond]

I understand we have two categories and two defined bits here, but I'm confused what you mean by

You'll need to check both eBroadcastBitProgress and eBroadcastBitProgressCategory

Wouldn't I just want to set both bits (and unset internal), and pass that to the debugger to see if there is a listener for either?

EDIT:

I read around the class and a comment in ProgressManager mentioned the category is only used for beginning or ending of the progress, so I made ReportProgress set the external bit if it's external, and the manager sets the category if it's completed (or started).

[Jlalond]

Hey @JDevlieghere, I was out for the holidays, but did you want to take a second look at this? Or can I land it to rebase the SBProgress changes on top of?

[JDevlieghere]

LGTM with the redundant field removed.