[lldb][Progress] Separate title and details (llvm#77547) (#7978)

Per this RFC:
https://discourse.llvm.org/t/rfc-improve-lldb-progress-reporting/75717
on improving progress reports, this commit separates the title field and
details field so that the title specifies the category that the progress
report falls under. The details field is added as a part of the
constructor for progress reports and by default is an empty string. In addition, changes the total amount of progress completed into a std::optional. Also
updates the test to check for details being correctly reported from the
event structured data dictionary.

Some changes were made to Swift progress reports to match the upstream
changes to `Progress`.
(cherry picked from commit f1ef910)

Author: chelcassanova

lldb/include/lldb/Core/DebuggerEvents.h

@@ -20,9 +20,10 @@ class Stream;
 
 class ProgressEventData : public EventData {
 public:
-  ProgressEventData(uint64_t progress_id, std::string title, std::string update,
-                    uint64_t completed, uint64_t total, bool debugger_specific)
-      : m_title(std::move(title)), m_details(std::move(update)),
+  ProgressEventData(uint64_t progress_id, std::string title,
+                    std::string details, uint64_t completed, uint64_t total,
+                    bool debugger_specific)
+      : m_title(std::move(title)), m_details(std::move(details)),
         m_id(progress_id), m_completed(completed), m_total(total),
         m_debugger_specific(debugger_specific) {}
 

lldb/include/lldb/Core/Progress.h

@@ -64,12 +64,13 @@ class Progress {
   /// @param [in] title The title of this progress activity.
   ///
   /// @param [in] total The total units of work to be done if specified, if
-  /// set to UINT64_MAX then an indeterminate progress indicator should be
+  /// set to std::nullopt then an indeterminate progress indicator should be
   /// displayed.
   ///
   /// @param [in] debugger An optional debugger pointer to specify that this
   /// progress is to be reported only to specific debuggers.
-  Progress(std::string title, uint64_t total = UINT64_MAX,
+  Progress(std::string title, std::string details = {},
+           std::optional<uint64_t> total = std::nullopt,
            lldb_private::Debugger *debugger = nullptr);
 
   /// Destroy the progress object.
@@ -89,20 +90,23 @@ class Progress {
   /// @param [in] amount The amount to increment m_completed by.
   ///
   /// @param [in] an optional message associated with this update.
-  void Increment(uint64_t amount = 1, std::string update = {});
+  void Increment(uint64_t amount = 1,
+                 std::optional<std::string> updated_detail = {});
 
 private:
-  void ReportProgress(std::string update = {});
+  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, UINT64_MAX for non deterministic progress.
-  const uint64_t m_total;
+  /// 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;

lldb/source/Core/Progress.cpp

@@ -11,15 +11,22 @@
 #include "lldb/Core/Debugger.h"
 #include "lldb/Utility/StreamString.h"
 
+#include <optional>
+
 using namespace lldb;
 using namespace lldb_private;
 
 std::atomic<uint64_t> Progress::g_id(0);
 
-Progress::Progress(std::string title, uint64_t total,
+Progress::Progress(std::string title, std::string details,
+                   std::optional<uint64_t> total,
                    lldb_private::Debugger *debugger)
-    : m_title(title), m_id(++g_id), m_completed(0), m_total(total) {
-  assert(total > 0);
+    : m_title(title), m_details(details), m_id(++g_id), m_completed(0),
+      m_total(1) {
+  assert(total == std::nullopt || total > 0);
+  if (total)
+    m_total = *total;
+
   if (debugger)
     m_debugger_id = debugger->GetID();
   std::lock_guard<std::mutex> guard(m_mutex);
@@ -35,25 +42,28 @@ Progress::~Progress() {
   ReportProgress();
 }
 
-void Progress::Increment(uint64_t amount, std::string update) {
+void Progress::Increment(uint64_t amount,
+                         std::optional<std::string> updated_detail) {
   if (amount > 0) {
     std::lock_guard<std::mutex> guard(m_mutex);
+    if (updated_detail)
+      m_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 (amount > (m_total - m_completed))
+    if (m_total && (amount > (m_total - m_completed)))
       m_completed = m_total;
     else
       m_completed += amount;
-    ReportProgress(update);
+    ReportProgress();
   }
 }
 
-void Progress::ReportProgress(std::string update) {
+void Progress::ReportProgress() {
   if (!m_complete) {
     // Make sure we only send one notification that indicates the progress is
-    // complete.
+    // complete
     m_complete = m_completed == m_total;
-    Debugger::ReportProgress(m_id, m_title, std::move(update), m_completed,
-                             m_total, m_debugger_id);
+    Debugger::ReportProgress(m_id, m_title, m_details, m_completed, m_total,
+                             m_debugger_id);
   }
 }

lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

@@ -2888,9 +2888,8 @@ void ObjectFileELF::ParseSymtab(Symtab &lldb_symtab) {
   if (!module_sp)
     return;
 
-  Progress progress(
-      llvm::formatv("Parsing symbol table for {0}",
-                    m_file.GetFilename().AsCString("<Unknown>")));
+  Progress progress("Parsing symbol table",
+                    m_file.GetFilename().AsCString("<Unknown>"));
   ElapsedTime elapsed(module_sp->GetSymtabParseTime());
 
   // We always want to use the main object file so we (hopefully) only have one

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

@@ -2238,7 +2238,7 @@ void ObjectFileMachO::ParseSymtab(Symtab &symtab) {
   const char *file_name = file.GetFilename().AsCString("<Unknown>");
   LLDB_SCOPED_TIMERF("ObjectFileMachO::ParseSymtab () module = %s", file_name);
   LLDB_LOG(log, "Parsing symbol table for {0}", file_name);
-  Progress progress(llvm::formatv("Parsing symbol table for {0}", file_name));
+  Progress progress("Parsing symbol table", file_name);
 
   llvm::MachO::symtab_command symtab_load_command = {0, 0, 0, 0, 0, 0};
   llvm::MachO::linkedit_data_command function_starts_load_command = {0, 0, 0, 0};

lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp

@@ -75,9 +75,8 @@ void ManualDWARFIndex::Index() {
   // Include 2 passes per unit to index for extracting DIEs from the unit and
   // indexing the unit, and then 8 extra entries for finalizing each index set.
   const uint64_t total_progress = units_to_index.size() * 2 + 8;
-  Progress progress(
-      llvm::formatv("Manually indexing DWARF for {0}", module_desc.GetData()),
-      total_progress);
+  Progress progress("Manually indexing DWARF", module_desc.GetData(),
+                    total_progress);
 
   std::vector<IndexSet> sets(units_to_index.size());
 

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

@@ -509,8 +509,6 @@ void SymbolFileDWARF::InitializeObject() {
 
     if (apple_names.GetByteSize() > 0 || apple_namespaces.GetByteSize() > 0 ||
         apple_types.GetByteSize() > 0 || apple_objc.GetByteSize() > 0) {
-      Progress progress(llvm::formatv("Loading Apple DWARF index for {0}",
-                                      module_desc.GetData()));
       m_index = AppleDWARFIndex::Create(
           *GetObjectFile()->GetModule(), apple_names, apple_namespaces,
           apple_types, apple_objc, m_context.getOrLoadStrData());
@@ -522,8 +520,7 @@ void SymbolFileDWARF::InitializeObject() {
     DWARFDataExtractor debug_names;
     LoadSectionData(eSectionTypeDWARFDebugNames, debug_names);
     if (debug_names.GetByteSize() > 0) {
-      Progress progress(
-          llvm::formatv("Loading DWARF5 index for {0}", module_desc.GetData()));
+      Progress progress("Loading DWARF5 index", module_desc.GetData());
       llvm::Expected<std::unique_ptr<DebugNamesDWARFIndex>> index_or =
           DebugNamesDWARFIndex::Create(*GetObjectFile()->GetModule(),
                                        debug_names,

lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.cpp

@@ -4208,10 +4208,9 @@ void SwiftASTContext::ValidateSectionModules(
 
   Status error;
 
-  Progress progress(
-      llvm::formatv("Loading Swift module '{0}' dependencies",
-                    module.GetFileSpec().GetFilename().AsCString()),
-      module_names.size());
+  Progress progress("Loading Swift module '{0}' dependencies",
+                    module.GetFileSpec().GetFilename().AsCString(),
+                    module_names.size());
   size_t completion = 0;
 
   for (const std::string &module_name : module_names) {
@@ -8788,10 +8787,9 @@ bool SwiftASTContext::GetCompileUnitImportsImpl(
   if (cu_imports.size() == 0)
     return true;
 
-  Progress progress(
-      llvm::formatv("Getting Swift compile unit imports for '{0}'",
-                    compile_unit->GetPrimaryFile().GetFilename()),
-      cu_imports.size());
+  Progress progress("Getting Swift compile unit imports",
+                    compile_unit->GetPrimaryFile().GetFilename().GetCString(),
+                    cu_imports.size());
   size_t completion = 0;
   for (const SourceModule &module : cu_imports) {
     progress.Increment(++completion, module.path.back().GetStringRef().str());

lldb/source/Symbol/LocateSymbolFile.cpp

@@ -267,9 +267,8 @@ Symbols::LocateExecutableSymbolFile(const ModuleSpec &module_spec,
       FileSystem::Instance().Exists(symbol_file_spec))
     return symbol_file_spec;
 
-  Progress progress(llvm::formatv(
-      "Locating external symbol file for {0}",
-      module_spec.GetFileSpec().GetFilename().AsCString("<Unknown>")));
+  Progress progress(
+      "Locating external symbol file", module_spec.GetFileSpec().GetFilename().AsCString("<Unknown>"));
 
   FileSpecList debug_file_search_paths = default_search_paths;
 

lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py

@@ -39,3 +39,5 @@ def test_dwarf_symbol_loading_progress_report_structured_data(self):
         progress_data = lldb.SBDebugger.GetProgressDataFromEvent(event)
         message = progress_data.GetValueForKey("message").GetStringValue(100)
         self.assertGreater(len(message), 0)
+        details = progress_data.GetValueForKey("details").GetStringValue(100)
+        self.assertGreater(len(details), 0)

lldb/test/API/functionalities/progress_reporting/swift_progress_reporting/TestSwiftProgressReporting.py

@@ -38,7 +38,7 @@ def test_swift_progress_report(self):
             "Loading Swift module",
             "Importing modules used in expression",
             "Setting up Swift reflection",
-            "Getting Swift compile unit imports for",
+            "Getting Swift compile unit imports",
             "Importing Swift modules",
             "Importing Swift standard library",
         ]