[Lldb-commits] [lldb] [lldb][Progress] Separate title and details (PR #77547)
Chelsea Cassanova via lldb-commits
lldb-commits at lists.llvm.org
Wed Jan 10 10:56:50 PST 2024
https://github.com/chelcassanova updated https://github.com/llvm/llvm-project/pull/77547
>From 44a3cdca21bc9c2aa24eeaf5d82c8b8af382bfa7 Mon Sep 17 00:00:00 2001
From: Chelsea Cassanova <chelsea_cassanova at apple.com>
Date: Tue, 9 Jan 2024 18:32:06 -0800
Subject: [PATCH 1/3] [lldb][Progress] Separate title and details
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.
---
lldb/include/lldb/Core/DebuggerEvents.h | 4 ++--
lldb/include/lldb/Core/Progress.h | 3 ++-
lldb/source/Core/Progress.cpp | 7 ++++---
lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp | 2 +-
lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp | 4 +---
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp | 6 ++----
.../Plugins/SymbolLocator/Default/SymbolLocatorDefault.cpp | 4 +---
.../progress_reporting/TestProgressReporting.py | 2 ++
8 files changed, 15 insertions(+), 17 deletions(-)
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)
>From 75de25c88f13130db8e8b806fa928b6d858daef5 Mon Sep 17 00:00:00 2001
From: Chelsea Cassanova <chelsea_cassanova at apple.com>
Date: Tue, 9 Jan 2024 21:47:36 -0800
Subject: [PATCH 2/3] fixup! [lldb][Progress] Separate title and details
Formatting
---
lldb/include/lldb/Core/DebuggerEvents.h | 5 +++--
lldb/include/lldb/Core/Progress.h | 3 ++-
lldb/source/Core/Progress.cpp | 7 ++++---
lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp | 3 ++-
.../Plugins/SymbolLocator/Default/SymbolLocatorDefault.cpp | 4 +++-
5 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/lldb/include/lldb/Core/DebuggerEvents.h b/lldb/include/lldb/Core/DebuggerEvents.h
index 1fadb96a4b565d..4a27766e94e3aa 100644
--- a/lldb/include/lldb/Core/DebuggerEvents.h
+++ b/lldb/include/lldb/Core/DebuggerEvents.h
@@ -20,8 +20,9 @@ class Stream;
class ProgressEventData : public EventData {
public:
- ProgressEventData(uint64_t progress_id, std::string title, std::string details,
- uint64_t completed, uint64_t total, bool debugger_specific)
+ 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) {}
diff --git a/lldb/include/lldb/Core/Progress.h b/lldb/include/lldb/Core/Progress.h
index 1ea6df01a4bd4a..fd5b8463ccc5c2 100644
--- a/lldb/include/lldb/Core/Progress.h
+++ b/lldb/include/lldb/Core/Progress.h
@@ -69,7 +69,8 @@ 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, std::string details = {}, 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.
diff --git a/lldb/source/Core/Progress.cpp b/lldb/source/Core/Progress.cpp
index 8c99b561373362..6d27763f85acbf 100644
--- a/lldb/source/Core/Progress.cpp
+++ b/lldb/source/Core/Progress.cpp
@@ -18,7 +18,8 @@ std::atomic<uint64_t> Progress::g_id(0);
Progress::Progress(std::string title, std::string details, uint64_t total,
lldb_private::Debugger *debugger)
- : m_title(title), m_details(details), 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();
@@ -54,7 +55,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, m_details, m_completed,
- m_total, m_debugger_id);
+ Debugger::ReportProgress(m_id, m_title, m_details, m_completed, m_total,
+ m_debugger_id);
}
}
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp b/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
index 52555908865b0b..92275600f99caa 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -75,7 +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("Manually indexing DWARF", 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/SymbolLocator/Default/SymbolLocatorDefault.cpp b/lldb/source/Plugins/SymbolLocator/Default/SymbolLocatorDefault.cpp
index 52ae7adb8d3d97..6f0126b16cdc00 100644
--- a/lldb/source/Plugins/SymbolLocator/Default/SymbolLocatorDefault.cpp
+++ b/lldb/source/Plugins/SymbolLocator/Default/SymbolLocatorDefault.cpp
@@ -98,7 +98,9 @@ std::optional<FileSpec> SymbolLocatorDefault::LocateExecutableSymbolFile(
FileSystem::Instance().Exists(symbol_file_spec))
return symbol_file_spec;
- Progress progress("Locating external symbol file", 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;
>From 8d7c2bfe26c4a0917b0c9eaea3cfc3edee2e6d8c Mon Sep 17 00:00:00 2001
From: Chelsea Cassanova <chelsea_cassanova at apple.com>
Date: Wed, 10 Jan 2024 10:54:40 -0800
Subject: [PATCH 3/3] Update ELF progress report, remove Apple DWARF report,
use std::optional
Updates the ELF index progress report, remove the Apple DWARF progress
report and use a std::optional for the `m_total` field in `Progress`
---
lldb/include/lldb/Core/Progress.h | 8 +++----
lldb/source/Core/Progress.cpp | 24 +++++++++++--------
.../Plugins/ObjectFile/ELF/ObjectFileELF.cpp | 5 ++--
.../SymbolFile/DWARF/SymbolFileDWARF.cpp | 1 -
4 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/lldb/include/lldb/Core/Progress.h b/lldb/include/lldb/Core/Progress.h
index fd5b8463ccc5c2..1d8a5db8a5c436 100644
--- a/lldb/include/lldb/Core/Progress.h
+++ b/lldb/include/lldb/Core/Progress.h
@@ -64,13 +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, std::string details = {},
- uint64_t total = UINT64_MAX,
+ std::optional<uint64_t> total = std::nullopt,
lldb_private::Debugger *debugger = nullptr);
/// Destroy the progress object.
@@ -103,8 +103,8 @@ class Progress {
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 for non deterministic progress.
+ const std::optional<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;
diff --git a/lldb/source/Core/Progress.cpp b/lldb/source/Core/Progress.cpp
index 6d27763f85acbf..3c9b476d6c8082 100644
--- a/lldb/source/Core/Progress.cpp
+++ b/lldb/source/Core/Progress.cpp
@@ -11,16 +11,19 @@
#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, std::string details, uint64_t total,
+Progress::Progress(std::string title, std::string details,
+ std::optional<uint64_t> total,
lldb_private::Debugger *debugger)
: m_title(title), m_details(details), m_id(++g_id), m_completed(0),
m_total(total) {
- assert(total > 0);
+ assert(total == std::nullopt || total > 0);
if (debugger)
m_debugger_id = debugger->GetID();
std::lock_guard<std::mutex> guard(m_mutex);
@@ -31,8 +34,8 @@ Progress::~Progress() {
// Make sure to always report progress completed when this object is
// 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;
+ if (m_total.has_value() && !m_completed)
+ m_completed = m_total.value();
ReportProgress();
}
@@ -42,8 +45,8 @@ void Progress::Increment(uint64_t amount, std::string update) {
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))
- m_completed = m_total;
+ if (m_total.has_value() && (amount > (m_total.value() - m_completed)))
+ m_completed = m_total.value();
else
m_completed += amount;
ReportProgress(update);
@@ -53,9 +56,10 @@ void Progress::Increment(uint64_t amount, std::string update) {
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_details, m_completed, m_total,
- m_debugger_id);
+ // complete, and only modify m_complete is m_total isn't null.
+ if (m_total.has_value())
+ m_complete = m_completed == m_total.value();
+ Debugger::ReportProgress(m_id, m_title, m_details, m_completed,
+ m_total.value_or(0), m_debugger_id);
}
}
diff --git a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
index 43ab87f08e1925..0d95a1c12bde35 100644
--- a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -2897,9 +2897,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
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 1ea14366ad7dd7..755222d8330b96 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -519,7 +519,6 @@ void SymbolFileDWARF::InitializeObject() {
if (apple_names.GetByteSize() > 0 || apple_namespaces.GetByteSize() > 0 ||
apple_types.GetByteSize() > 0 || apple_objc.GetByteSize() > 0) {
- 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());
More information about the lldb-commits
mailing list