[lldb] Add the ability to provide a message to a progress event update

[JDevlieghere]

Summary

This patch adds the ability to add a message to a progress event update.

Consider the following example as motivation. Say you have to load symbols for 3 dynamic libraries: libFoo, libBar, libBaz. Currently, there are two ways to report this:

• As 3 separate progress instances. In this case you create a progress instance with the message "Loading symbols: libFoo", ""Loading symbols: libBar", and "Loading symbols: libBaz" respectively. If the operations are completely separate, i.e. they belong to different modules, this approach makes sense. This is what we do for parsing the symbol table, loading the dwarf index, etc.
• As 1 progress instance with 3 units of work. This is the preferred approach is you know the number of units of work in advance, because you can report determinate progress. The title would be "Loading symbols" and you call Progress::Increment for each of the libraries. Note that with the current design, there's no way to include the name of the libraries, which is what this patch addresses.

[mib]

Makes sense! LGTM!

[bulbazord]

Thanks for including the motivation in your summary, that made following this change a lot easier.

LGTM

[kastiglione]

nice

lldb/include/lldb/Core/DebuggerEvents.h

53–54

Are these going to be needed in a follow up?

59

minor quibble, but maybe m_current_subtitle (or some other m_current_<noun>) – I know that's wordy but "update" is a bit vague. Do as you please tho.

[JDevlieghere]

lldb/include/lldb/Core/DebuggerEvents.h

53–54

Yes, they're going in the dict from D143690. I saw Ismail's comments while working on this patch so I knew I was going to change it.

59

How about m_details. I don't think we need to include current here, as this is all part of a single event. I'll add comments to set expectations about the title being fixed and the details potentially changing.

[JDevlieghere]

• update -> details
• expose title and details separately in structured data

[kastiglione]

details is good, thanks

lldb/include/lldb/Core/Debugger.h

protected:
   ///   is UINT64_MAX, then an indeterminate progress indicator should be
   ///   displayed.
   ///
   /// \param [in] debugger_id
   ///   If this optional parameter has a value, it indicates the unique
   ///   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, const std::string &message,
-                             uint64_t completed, uint64_t total,
+  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 ReportDiagnosticImpl(DiagnosticEventData::Type type,
                                    std::string message,
                                    std::optional<lldb::user_id_t> debugger_id,
                                    std::once_flag *once);
 
   void PrintProgress(const ProgressEventData &data);

lldb/include/lldb/Core/DebuggerEvents.h

 #ifndef LLDB_CORE_DEBUGGER_EVENTS_H
 #define LLDB_CORE_DEBUGGER_EVENTS_H
 
 namespace lldb_private {
 class Stream;
 
 class ProgressEventData : public EventData {
 public:
-  ProgressEventData(uint64_t progress_id, const std::string &message,
+  ProgressEventData(uint64_t progress_id, std::string title, std::string update,
                     uint64_t completed, uint64_t total, bool debugger_specific)
-      : m_message(message), m_id(progress_id), m_completed(completed),
-        m_total(total), m_debugger_specific(debugger_specific) {}
+      : m_title(std::move(title)), m_details(std::move(update)),
+        m_id(progress_id), m_completed(completed), m_total(total),
+        m_debugger_specific(debugger_specific) {}
 
   static ConstString GetFlavorString();
 
   ConstString GetFlavor() const override;
 
   void Dump(Stream *s) const override;
 
   static const ProgressEventData *GetEventDataFromEvent(const Event *event_ptr);
 
   static StructuredData::DictionarySP
   GetAsStructuredData(const Event *event_ptr);
 
   uint64_t GetID() const { return m_id; }
   bool IsFinite() const { return m_total != UINT64_MAX; }
   uint64_t GetCompleted() const { return m_completed; }
   uint64_t GetTotal() const { return m_total; }
-  const std::string &GetMessage() const { return m_message; }
+  std::string GetMessage() const {
+    std::string message = m_title;
+    if (!m_details.empty()) {
+      message.append(": ");
+      message.append(m_details);
+    }
+    return message;
+  }
+  const std::string &GetTitle() const { return m_title; }
+  const std::string &GetDetails() const { return m_details; }
   bool IsDebuggerSpecific() const { return m_debugger_specific; }
 
 private:
-  std::string m_message;
+  /// The title of this progress event. The value is expected to remain stable
+  /// for a given progress ID.
+  std::string m_title;
+
+  /// Details associated with this progress event update. The value is expected
+  /// to change between progress events.
+  std::string m_details;
+
+  /// Unique ID used to associate progress events.
   const uint64_t m_id;
+
   uint64_t m_completed;
   const uint64_t m_total;
   const bool m_debugger_specific;
   ProgressEventData(const ProgressEventData &) = delete;
   const ProgressEventData &operator=(const ProgressEventData &) = delete;
 };
 
 class DiagnosticEventData : public EventData {

lldb/include/lldb/Core/Progress.h

public:
   /// Increment the progress and send a notification to the intalled callback.
   ///
   /// If incrementing ends up exceeding m_total, m_completed will be updated
   /// to match m_total and no subsequent progress notifications will be sent.
   /// If no total was specified in the constructor, this function will not do
   /// anything nor send any progress updates.
   ///
   /// @param [in] amount The amount to increment m_completed by.
-  void Increment(uint64_t amount = 1);
+  ///
+  /// @param [in] an optional message associated with this update.
+  void Increment(uint64_t amount = 1, std::string update = {});
 
 private:
-  void ReportProgress();
+  void ReportProgress(std::string update = {});
   static std::atomic<uint64_t> g_id;
   /// The title of the progress activity.
   std::string m_title;
   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;

lldb/source/API/SBDebugger.cpp

 }
 
 const char *SBDebugger::GetProgressFromEvent(const lldb::SBEvent &event,
                                              uint64_t &progress_id,
                                              uint64_t &completed,
                                              uint64_t &total,
                                              bool &is_debugger_specific) {
   LLDB_INSTRUMENT_VA(event);
+
   const ProgressEventData *progress_data =
       ProgressEventData::GetEventDataFromEvent(event.get());
   if (progress_data == nullptr)
     return nullptr;
   progress_id = progress_data->GetID();
   completed = progress_data->GetCompleted();
   total = progress_data->GetTotal();
   is_debugger_specific = progress_data->IsDebuggerSpecific();
-  return progress_data->GetMessage().c_str();
+  ConstString message(progress_data->GetMessage());
+  return message.AsCString();
 }
 
 lldb::SBStructuredData
 SBDebugger::GetProgressDataFromEvent(const lldb::SBEvent &event) {
   LLDB_INSTRUMENT_VA(event);
 
   StructuredData::DictionarySP dictionary_sp =
       ProgressEventData::GetAsStructuredData(event.get());

lldb/source/Core/Debugger.cpp

void Debugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
   // For simplicity's sake, I am not going to deal with how to close down any
   // open logging streams, I just redirect everything from here on out to the
   // callback.
   m_callback_handler_sp =
       std::make_shared<CallbackLogHandler>(log_callback, baton);
 }
 
 static void PrivateReportProgress(Debugger &debugger, uint64_t progress_id,
-                                  const std::string &message,
+                                  std::string title, std::string details,
                                   uint64_t completed, uint64_t total,
                                   bool is_debugger_specific) {
   // Only deliver progress events if we have any progress listeners.
   const uint32_t event_type = Debugger::eBroadcastBitProgress;
   if (!debugger.GetBroadcaster().EventTypeHasListeners(event_type))
     return;
   EventSP event_sp(new Event(
-      event_type, new ProgressEventData(progress_id, message, completed, total,
-                                        is_debugger_specific)));
+      event_type,
+      new ProgressEventData(progress_id, std::move(title), std::move(details),
+                            completed, total, is_debugger_specific)));
   debugger.GetBroadcaster().BroadcastEvent(event_sp);
 }
 
-void Debugger::ReportProgress(uint64_t progress_id, const std::string &message,
-                              uint64_t completed, uint64_t total,
+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) {
   // 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
     // still exists.
     DebuggerSP debugger_sp = FindDebuggerWithID(*debugger_id);
     if (debugger_sp)
-      PrivateReportProgress(*debugger_sp, progress_id, message, completed,
-                            total, /*is_debugger_specific*/ true);
+      PrivateReportProgress(*debugger_sp, progress_id, std::move(title),
+                            std::move(details), completed, total,
+                            /*is_debugger_specific*/ true);
     return;
   }
   // The progress event is not debugger specific, iterate over all debuggers
   // and deliver a progress event to each one.
   if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) {
     std::lock_guard<std::recursive_mutex> guard(*g_debugger_list_mutex_ptr);
     DebuggerList::iterator pos, end = g_debugger_list_ptr->end();
     for (pos = g_debugger_list_ptr->begin(); pos != end; ++pos)
-      PrivateReportProgress(*(*pos), progress_id, message, completed, total,
-                            /*is_debugger_specific*/ false);
+      PrivateReportProgress(*(*pos), progress_id, title, details, completed,
+                            total, /*is_debugger_specific*/ false);
   }
 }
 
 static void PrivateReportDiagnostic(Debugger &debugger,
                                     DiagnosticEventData::Type type,
                                     std::string message,
                                     bool debugger_specific) {
   uint32_t event_type = 0;

lldb/source/Core/DebuggerEvents.cpp

ConstString ProgressEventData::GetFlavorString() {
   return g_flavor;
 }
 
 ConstString ProgressEventData::GetFlavor() const {
   return ProgressEventData::GetFlavorString();
 }
 
 void ProgressEventData::Dump(Stream *s) const {
-  s->Printf(" id = %" PRIu64 ", message = \"%s\"", m_id, m_message.c_str());
+  s->Printf(" id = %" PRIu64 ", title = \"%s\"", m_id, m_title.c_str());
+  if (!m_details.empty())
+    s->Printf(", details = \"%s\"", m_details.c_str());
   if (m_completed == 0 || m_completed == m_total)
     s->Printf(", type = %s", m_completed == 0 ? "start" : "end");
   else
     s->PutCString(", type = update");
   // If m_total is UINT64_MAX, there is no progress to report, just "start"
   // and "end". If it isn't we will show the completed and total amounts.
   if (m_total != UINT64_MAX)
     s->Printf(", progress = %" PRIu64 " of %" PRIu64, m_completed, m_total);
 }
 
 const ProgressEventData *
 ProgressEventData::GetEventDataFromEvent(const Event *event_ptr) {
   return GetEventDataFromEventImpl<ProgressEventData>(event_ptr);
 }
 
 StructuredData::DictionarySP
 ProgressEventData::GetAsStructuredData(const Event *event_ptr) {
   const ProgressEventData *progress_data =
       ProgressEventData::GetEventDataFromEvent(event_ptr);
 
   if (!progress_data)
     return {};
 
   auto dictionary_sp = std::make_shared<StructuredData::Dictionary>();
+  dictionary_sp->AddStringItem("title", progress_data->GetTitle());
+  dictionary_sp->AddStringItem("details", progress_data->GetDetails());
   dictionary_sp->AddStringItem("message", progress_data->GetMessage());
   dictionary_sp->AddIntegerItem("progress_id", progress_data->GetID());
   dictionary_sp->AddIntegerItem("completed", progress_data->GetCompleted());
   dictionary_sp->AddIntegerItem("total", progress_data->GetTotal());
   dictionary_sp->AddBooleanItem("debugger_specific",
                                 progress_data->IsDebuggerSpecific());
 
   return dictionary_sp;

lldb/source/Core/Progress.cpp

Progress::~Progress() {
   // 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;
     ReportProgress();
   }
 }
 
-void Progress::Increment(uint64_t amount) {
+void Progress::Increment(uint64_t amount, std::string update) {
   if (amount > 0) {
     std::lock_guard<std::mutex> guard(m_mutex);
     // Watch out for unsigned overflow and make sure we don't increment too
     // much and exceed m_total.
     if (amount > (m_total - m_completed))
       m_completed = m_total;
     else
       m_completed += amount;
-    ReportProgress();
+    ReportProgress(update);
   }
 }
 
-void Progress::ReportProgress() {
+void Progress::ReportProgress(std::string update) {
   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_completed, m_total,
-                             m_debugger_id);
+    Debugger::ReportProgress(m_id, m_title, std::move(update), m_completed,
+                             m_total, m_debugger_id);
   }
 }