[Lldb-commits] [lldb] [lldb][Progress] Separate title and details (PR #77547)

Tue Jan 9 18:38:37 PST 2024

llvmbot wrote:




@llvm/pr-subscribers-lldb

Author: Chelsea Cassanova (chelcassanova)

<details>
<summary>Changes</summary>

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. Also updates the test to check for details being correctly reported from the event structured data dictionary.

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


8 Files Affected:

- (modified) lldb/include/lldb/Core/DebuggerEvents.h (+2-2) 
- (modified) lldb/include/lldb/Core/Progress.h (+2-1) 
- (modified) lldb/source/Core/Progress.cpp (+4-3) 
- (modified) lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp (+1-1) 
- (modified) lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp (+1-3) 
- (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp (+2-4) 
- (modified) lldb/source/Plugins/SymbolLocator/Default/SymbolLocatorDefault.cpp (+1-3) 
- (modified) lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py (+2) 


``````````diff

diff --git a/lldb/include/lldb/Core/DebuggerEvents.h b/lldb/include/lldb/Core/DebuggerEvents.h
index 982b22229701f8..1fadb96a4b565d 100644
--- a/lldb/include/lldb/Core/DebuggerEvents.h
+++ b/lldb/include/lldb/Core/DebuggerEvents.h
@@ -20,9 +20,9 @@ class Stream;
 
 class ProgressEventData : public EventData {
 public:
-  ProgressEventData(uint64_t progress_id, std::string title, std::string 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(update)),
+      : 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) {}
 
diff --git a/lldb/include/lldb/Core/Progress.h b/lldb/include/lldb/Core/Progress.h
index b2b8781a43b059..1ea6df01a4bd4a 100644
--- a/lldb/include/lldb/Core/Progress.h
+++ b/lldb/include/lldb/Core/Progress.h
@@ -69,7 +69,7 @@ class Progress {
   ///
   /// @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 = {}, uint64_t total = UINT64_MAX,
            lldb_private::Debugger *debugger = nullptr);
 
   /// Destroy the progress object.
@@ -96,6 +96,7 @@ class Progress {
   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;
diff --git a/lldb/source/Core/Progress.cpp b/lldb/source/Core/Progress.cpp
index ea3f874916a999..8c99b561373362 100644
--- a/lldb/source/Core/Progress.cpp
+++ b/lldb/source/Core/Progress.cpp
@@ -16,9 +16,9 @@ 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, uint64_t total,
                    lldb_private::Debugger *debugger)
-    : m_title(title), m_id(++g_id), m_completed(0), m_total(total) {
+    : m_title(title), m_details(details), m_id(++g_id), m_completed(0), m_total(total) {
   assert(total > 0);
   if (debugger)
     m_debugger_id = debugger->GetID();
@@ -38,6 +38,7 @@ Progress::~Progress() {
 void Progress::Increment(uint64_t amount, std::string update) {
   if (amount > 0) {
     std::lock_guard<std::mutex> guard(m_mutex);
+    m_details = update;
     // Watch out for unsigned overflow and make sure we don't increment too
     // much and exceed m_total.
     if (amount > (m_total - m_completed))
@@ -53,7 +54,7 @@ void Progress::ReportProgress(std::string update) {
     // 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, std::move(update), m_completed,
+    Debugger::ReportProgress(m_id, m_title, m_details, m_completed,
                              m_total, m_debugger_id);
   }
 }
diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index 182a9f2afaeb2e..f451bac598b874 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -2225,7 +2225,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};
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp b/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
index 16ff5f7d4842ca..52555908865b0b 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -75,9 +75,7 @@ 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());
 
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 1a16b70f42fe1f..1ea14366ad7dd7 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -519,8 +519,7 @@ 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()));
+      Progress progress("Loading Apple DWARF index", module_desc.GetData());
       m_index = AppleDWARFIndex::Create(
           *GetObjectFile()->GetModule(), apple_names, apple_namespaces,
           apple_types, apple_objc, m_context.getOrLoadStrData());
@@ -532,8 +531,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,
diff --git a/lldb/source/Plugins/SymbolLocator/Default/SymbolLocatorDefault.cpp b/lldb/source/Plugins/SymbolLocator/Default/SymbolLocatorDefault.cpp
index ed014f99fdb5e7..52ae7adb8d3d97 100644
--- a/lldb/source/Plugins/SymbolLocator/Default/SymbolLocatorDefault.cpp
+++ b/lldb/source/Plugins/SymbolLocator/Default/SymbolLocatorDefault.cpp
@@ -98,9 +98,7 @@ std::optional<FileSpec> SymbolLocatorDefault::LocateExecutableSymbolFile(
       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;
 
diff --git a/lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py b/lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py
index 0e72770e350366..9af53845ca1b77 100644
--- a/lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py
+++ b/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)

``````````

</details>


https://github.com/llvm/llvm-project/pull/77547
