-
Notifications
You must be signed in to change notification settings - Fork 12.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix statistics dump to report per-target #113723
Conversation
06d232c
to
debd58e
Compare
@llvm/pr-subscribers-lldb Author: None (jeffreytan81) Changes"statistics dump" currently report the statistics of all targets in debugger instead of current target. This is wrong because there is a "statistics dump --all-targets" option that supposed to include everything. This PR fixes the issue by only report statistics for current target instead of all. It also includes the change to reset statistics debug info/symbol table parsing/indexing time during debugger destroy. This is required so that we report current statistics if we plan to reuse lldb/lldb-dap across debug sessions Full diff: https://github.com/llvm/llvm-project/pull/113723.diff 13 Files Affected:
diff --git a/lldb/include/lldb/API/SBDebugger.h b/lldb/include/lldb/API/SBDebugger.h
index 6afa1c932ab050..d80d609b3e7a28 100644
--- a/lldb/include/lldb/API/SBDebugger.h
+++ b/lldb/include/lldb/API/SBDebugger.h
@@ -426,6 +426,8 @@ class LLDB_API SBDebugger {
SBTypeSynthetic GetSyntheticForType(SBTypeNameSpecifier);
+ void ResetStatistics();
+
#ifndef SWIG
/// Run the command interpreter.
///
diff --git a/lldb/include/lldb/Symbol/SymbolFile.h b/lldb/include/lldb/Symbol/SymbolFile.h
index 8419495da73a22..7072d987e8b0ab 100644
--- a/lldb/include/lldb/Symbol/SymbolFile.h
+++ b/lldb/include/lldb/Symbol/SymbolFile.h
@@ -422,6 +422,13 @@ class SymbolFile : public PluginInterface {
/// hasn't been indexed yet, or a valid duration if it has.
virtual StatsDuration::Duration GetDebugInfoIndexTime() { return {}; }
+ /// Reset the time taken to parse the debug information.
+ virtual void ResetDebugInfoParseTime() {}
+
+ /// Reset the time it took to index the debug information in the object
+ /// file.
+ virtual void ResetDebugInfoIndexTime() {}
+
/// Get the additional modules that this symbol file uses to parse debug info.
///
/// Some debug info is stored in stand alone object files that are represented
diff --git a/lldb/include/lldb/Symbol/SymbolFileOnDemand.h b/lldb/include/lldb/Symbol/SymbolFileOnDemand.h
index 8073d1816860e3..12726e07c92e74 100644
--- a/lldb/include/lldb/Symbol/SymbolFileOnDemand.h
+++ b/lldb/include/lldb/Symbol/SymbolFileOnDemand.h
@@ -182,6 +182,9 @@ class SymbolFileOnDemand : public lldb_private::SymbolFile {
lldb_private::StatsDuration::Duration GetDebugInfoParseTime() override;
lldb_private::StatsDuration::Duration GetDebugInfoIndexTime() override;
+ void ResetDebugInfoParseTime() override;
+ void ResetDebugInfoIndexTime() override;
+
uint32_t GetAbilities() override;
Symtab *GetSymtab() override { return m_sym_file_impl->GetSymtab(); }
diff --git a/lldb/include/lldb/Target/Statistics.h b/lldb/include/lldb/Target/Statistics.h
index f3414ae314f339..91a3ffb5bfa3d3 100644
--- a/lldb/include/lldb/Target/Statistics.h
+++ b/lldb/include/lldb/Target/Statistics.h
@@ -41,6 +41,8 @@ class StatsDuration {
}
operator Duration() const { return get(); }
+ void reset() { value.store(0, std::memory_order_relaxed); }
+
StatsDuration &operator+=(Duration dur) {
value.fetch_add(std::chrono::duration_cast<InternalDuration>(dur).count(),
std::memory_order_relaxed);
@@ -311,6 +313,16 @@ class DebuggerStats {
ReportStatistics(Debugger &debugger, Target *target,
const lldb_private::StatisticsOptions &options);
+ /// Reset metrics associated with one or all targets in a debugger.
+ ///
+ /// \param debugger
+ /// The debugger to reset the target list from if \a target is NULL.
+ ///
+ /// \param target
+ /// The target to reset statistics for, or if null, reset statistics
+ /// for all targets
+ static void ResetStatistics(Debugger &debugger, Target *target);
+
protected:
// Collecting stats can be set to true to collect stats that are expensive
// to collect. By default all stats that are cheap to collect are enabled.
diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp
index 47931f1c16f9a3..4efec747aacff1 100644
--- a/lldb/source/API/SBDebugger.cpp
+++ b/lldb/source/API/SBDebugger.cpp
@@ -1667,6 +1667,12 @@ SBTypeSynthetic SBDebugger::GetSyntheticForType(SBTypeNameSpecifier type_name) {
DataVisualization::GetSyntheticForType(type_name.GetSP()));
}
+void SBDebugger::ResetStatistics() {
+ LLDB_INSTRUMENT_VA(this);
+ if (m_opaque_sp)
+ DebuggerStats::ResetStatistics(*m_opaque_sp, nullptr);
+}
+
static llvm::ArrayRef<const char *> GetCategoryArray(const char **categories) {
if (categories == nullptr)
return {};
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index e6b9eedd89b4e3..bb4caa23a7ba46 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -789,6 +789,7 @@ void Debugger::Destroy(DebuggerSP &debugger_sp) {
(*debugger_sp->GetAsyncErrorStream()) << result.GetErrorData() << '\n';
}
+ DebuggerStats::ResetStatistics(*debugger_sp, nullptr);
debugger_sp->Clear();
if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) {
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h
index fea3a4fd697389..b0655a6336fb48 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h
@@ -83,6 +83,8 @@ class DWARFIndex {
StatsDuration::Duration GetIndexTime() { return m_index_time; }
+ void ResetIndexTime() { m_index_time.reset(); }
+
protected:
Module &m_module;
StatsDuration m_index_time;
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 9287d4baf19e9c..f990fa795bc588 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -4464,6 +4464,11 @@ StatsDuration::Duration SymbolFileDWARF::GetDebugInfoIndexTime() {
return {};
}
+void SymbolFileDWARF::ResetDebugInfoIndexTime() {
+ if (m_index)
+ return m_index->ResetIndexTime();
+}
+
Status SymbolFileDWARF::CalculateFrameVariableError(StackFrame &frame) {
std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());
CompileUnit *cu = frame.GetSymbolContext(eSymbolContextCompUnit).comp_unit;
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
index 4967b37d753a09..3e5dd5c7fe62ed 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -318,6 +318,9 @@ class SymbolFileDWARF : public SymbolFileCommon {
StatsDuration &GetDebugInfoParseTimeRef() { return m_parse_time; }
+ void ResetDebugInfoParseTime() override { m_parse_time.reset(); }
+ void ResetDebugInfoIndexTime() override;
+
virtual lldb::offset_t
GetVendorDWARFOpcodeSize(const DataExtractor &data,
const lldb::offset_t data_offset,
diff --git a/lldb/source/Symbol/SymbolFileOnDemand.cpp b/lldb/source/Symbol/SymbolFileOnDemand.cpp
index 0cfe9fc1514b5a..aa3153a9d07051 100644
--- a/lldb/source/Symbol/SymbolFileOnDemand.cpp
+++ b/lldb/source/Symbol/SymbolFileOnDemand.cpp
@@ -555,6 +555,18 @@ StatsDuration::Duration SymbolFileOnDemand::GetDebugInfoIndexTime() {
return m_sym_file_impl->GetDebugInfoIndexTime();
}
+void SymbolFileOnDemand::ResetDebugInfoParseTime() {
+ LLDB_LOG(GetLog(), "[{0}] {1} is not skipped", GetSymbolFileName(),
+ __FUNCTION__);
+ return m_sym_file_impl->ResetDebugInfoParseTime();
+}
+
+void SymbolFileOnDemand::ResetDebugInfoIndexTime() {
+ LLDB_LOG(GetLog(), "[{0}] {1} is not skipped", GetSymbolFileName(),
+ __FUNCTION__);
+ return m_sym_file_impl->ResetDebugInfoIndexTime();
+}
+
void SymbolFileOnDemand::SetLoadDebugInfoEnabled() {
if (m_debug_info_enabled)
return;
diff --git a/lldb/source/Target/Statistics.cpp b/lldb/source/Target/Statistics.cpp
index ae2f65ea4c4bdc..2d572e221f1c72 100644
--- a/lldb/source/Target/Statistics.cpp
+++ b/lldb/source/Target/Statistics.cpp
@@ -236,6 +236,27 @@ void TargetStats::IncreaseSourceRealpathCompatibleCount(uint32_t count) {
bool DebuggerStats::g_collecting_stats = false;
+void DebuggerStats::ResetStatistics(Debugger &debugger, Target *target) {
+ const uint64_t num_modules = target != nullptr
+ ? target->GetImages().GetSize()
+ : Module::GetNumberAllocatedModules();
+ for (size_t image_idx = 0; image_idx < num_modules; ++image_idx) {
+ Module *module = target != nullptr
+ ? target->GetImages().GetModuleAtIndex(image_idx).get()
+ : Module::GetAllocatedModuleAtIndex(image_idx);
+ if (module == nullptr)
+ continue;
+ ModuleStats module_stat;
+ module->GetSymtabParseTime().reset();
+ module->GetSymtabIndexTime().reset();
+ SymbolFile *sym_file = module->GetSymbolFile();
+ if (sym_file) {
+ sym_file->ResetDebugInfoIndexTime();
+ sym_file->ResetDebugInfoParseTime();
+ }
+ }
+}
+
llvm::json::Value DebuggerStats::ReportStatistics(
Debugger &debugger, Target *target,
const lldb_private::StatisticsOptions &options) {
@@ -261,14 +282,18 @@ llvm::json::Value DebuggerStats::ReportStatistics(
std::vector<ModuleStats> modules;
std::lock_guard<std::recursive_mutex> guard(
Module::GetAllocationModuleCollectionMutex());
- const uint64_t num_modules = Module::GetNumberAllocatedModules();
+ const uint64_t num_modules = target != nullptr
+ ? target->GetImages().GetSize()
+ : Module::GetNumberAllocatedModules();
uint32_t num_debug_info_enabled_modules = 0;
uint32_t num_modules_has_debug_info = 0;
uint32_t num_modules_with_variable_errors = 0;
uint32_t num_modules_with_incomplete_types = 0;
uint32_t num_stripped_modules = 0;
for (size_t image_idx = 0; image_idx < num_modules; ++image_idx) {
- Module *module = Module::GetAllocatedModuleAtIndex(image_idx);
+ Module *module = target != nullptr
+ ? target->GetImages().GetModuleAtIndex(image_idx).get()
+ : Module::GetAllocatedModuleAtIndex(image_idx);
ModuleStats module_stat;
module_stat.symtab_parse_time = module->GetSymtabParseTime().get().count();
module_stat.symtab_index_time = module->GetSymtabIndexTime().get().count();
diff --git a/lldb/test/API/commands/statistics/basic/TestStats.py b/lldb/test/API/commands/statistics/basic/TestStats.py
index a0a9eeb6493207..54881c13bcb689 100644
--- a/lldb/test/API/commands/statistics/basic/TestStats.py
+++ b/lldb/test/API/commands/statistics/basic/TestStats.py
@@ -1,7 +1,8 @@
-import lldb
import json
import os
import re
+
+import lldb
from lldbsuite.test.decorators import *
from lldbsuite.test.lldbtest import *
from lldbsuite.test import lldbutil
@@ -540,7 +541,7 @@ def test_no_dsym_binary_has_symfile_identifiers_in_stats(self):
# in the stats.
self.runCmd("b main.cpp:7")
- debug_stats = self.get_stats()
+ debug_stats = self.get_stats("--all-targets")
exe_stats = self.find_module_in_metrics(exe, debug_stats)
# If we don't have a dSYM file, there should not be a key/value pair in
@@ -986,3 +987,38 @@ def test_summary_statistics_providers_vec(self):
# We may hit the std::vector C++ provider, or a summary provider string
if "c++" in summary_provider_str:
self.assertIn("std::vector", summary_provider_str)
+
+ @skipIfWindows
+ def test_multiple_targets(self):
+ """
+ Test statistics dump only reports the stats from current target and
+ "statistics dump --all-targets" includes all target stats.
+ """
+ da = {"CXX_SOURCES": "main.cpp", "EXE": self.getBuildArtifact("a.out")}
+ self.build(dictionary=da)
+ self.addTearDownCleanup(dictionary=da)
+
+ db = {"CXX_SOURCES": "second.cpp", "EXE": self.getBuildArtifact("second.out")}
+ self.build(dictionary=db)
+ self.addTearDownCleanup(dictionary=db)
+
+ main_exe = self.getBuildArtifact("a.out")
+ second_exe = self.getBuildArtifact("second.out")
+
+ (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
+ self, "// break here", lldb.SBFileSpec("main.cpp"), None, "a.out"
+ )
+ debugger_stats1 = self.get_stats()
+ self.assertIsNotNone(self.find_module_in_metrics(main_exe, debugger_stats1))
+ self.assertIsNone(self.find_module_in_metrics(second_exe, debugger_stats1))
+
+ (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
+ self, "// break here", lldb.SBFileSpec("second.cpp"), None, "second.out"
+ )
+ debugger_stats2 = self.get_stats()
+ self.assertIsNone(self.find_module_in_metrics(main_exe, debugger_stats2))
+ self.assertIsNotNone(self.find_module_in_metrics(second_exe, debugger_stats2))
+
+ all_targets_stats = self.get_stats("--all-targets")
+ self.assertIsNotNone(self.find_module_in_metrics(main_exe, all_targets_stats))
+ self.assertIsNotNone(self.find_module_in_metrics(second_exe, all_targets_stats))
diff --git a/lldb/test/API/commands/statistics/basic/second.cpp b/lldb/test/API/commands/statistics/basic/second.cpp
new file mode 100644
index 00000000000000..3af4e320c2fb53
--- /dev/null
+++ b/lldb/test/API/commands/statistics/basic/second.cpp
@@ -0,0 +1,5 @@
+// Test that the lldb command `statistics` works.
+
+int main(void) {
+ return 0; // break here
+}
|
@@ -422,6 +422,13 @@ class SymbolFile : public PluginInterface { | |||
/// hasn't been indexed yet, or a valid duration if it has. | |||
virtual StatsDuration::Duration GetDebugInfoIndexTime() { return {}; } | |||
|
|||
/// Reset the time taken to parse the debug information. | |||
virtual void ResetDebugInfoParseTime() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to a single function virtual void ResetStatistics();
/// Reset the time it took to index the debug information in the object | ||
/// file. | ||
virtual void ResetDebugInfoIndexTime() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
void ResetDebugInfoParseTime() override; | ||
void ResetDebugInfoIndexTime() override; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switch to single function ResetStatistics()
@@ -1667,6 +1667,12 @@ SBTypeSynthetic SBDebugger::GetSyntheticForType(SBTypeNameSpecifier type_name) { | |||
DataVisualization::GetSyntheticForType(type_name.GetSP())); | |||
} | |||
|
|||
void SBDebugger::ResetStatistics() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a void SBTarget::ResetStatistics()
too that calls with the right target?
lldb/source/Core/Debugger.cpp
Outdated
@@ -789,6 +789,7 @@ void Debugger::Destroy(DebuggerSP &debugger_sp) { | |||
(*debugger_sp->GetAsyncErrorStream()) << result.GetErrorData() << '\n'; | |||
} | |||
|
|||
DebuggerStats::ResetStatistics(*debugger_sp, nullptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably don't need to call this. Remove
void SymbolFileOnDemand::ResetDebugInfoParseTime() { | ||
LLDB_LOG(GetLog(), "[{0}] {1} is not skipped", GetSymbolFileName(), | ||
__FUNCTION__); | ||
return m_sym_file_impl->ResetDebugInfoParseTime(); | ||
} | ||
|
||
void SymbolFileOnDemand::ResetDebugInfoIndexTime() { | ||
LLDB_LOG(GetLog(), "[{0}] {1} is not skipped", GetSymbolFileName(), | ||
__FUNCTION__); | ||
return m_sym_file_impl->ResetDebugInfoIndexTime(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switch to a single function ResetStatistics
and fix the log text "is not skipped" seems wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"is not skipped" is correct -- it means this API call ResetStatistics
is not skipped by SymbolFileOnDemand class and pass through to the underlying real symbol file.
@@ -236,6 +236,27 @@ void TargetStats::IncreaseSourceRealpathCompatibleCount(uint32_t count) { | |||
|
|||
bool DebuggerStats::g_collecting_stats = false; | |||
|
|||
void DebuggerStats::ResetStatistics(Debugger &debugger, Target *target) { | |||
const uint64_t num_modules = target != nullptr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to lock the Module::GetAllocationModuleCollectionMutex()
when using the Module static calls for the global module list.
lldb/source/Target/Statistics.cpp
Outdated
: Module::GetAllocatedModuleAtIndex(image_idx); | ||
if (module == nullptr) | ||
continue; | ||
ModuleStats module_stat; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
lldb/source/Target/Statistics.cpp
Outdated
SymbolFile *sym_file = module->GetSymbolFile(); | ||
if (sym_file) { | ||
sym_file->ResetDebugInfoIndexTime(); | ||
sym_file->ResetDebugInfoParseTime(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be done in Module::ResetStatistics()
lldb/source/Target/Statistics.cpp
Outdated
module->GetSymtabParseTime().reset(); | ||
module->GetSymtabIndexTime().reset(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just call module->ResetStatistics();
debd58e
to
abf234c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this patch seems to just clear the module + symbol file stats, not any breakpoint resolve times, etc.
@@ -426,6 +426,8 @@ class LLDB_API SBDebugger { | |||
|
|||
SBTypeSynthetic GetSyntheticForType(SBTypeNameSpecifier); | |||
|
|||
void ResetStatistics(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a header doc comment for this to document what this will do:
- clear all stats for all modules in all targets
- clear all target breakpoint stats?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments added. I do not think we need to clear any breakpoint stats because they are not reused across debug sessions by "reuse lldb-dap"
@@ -101,6 +101,9 @@ class LLDB_API SBTarget { | |||
/// A SBStructuredData with the statistics collected. | |||
lldb::SBStructuredData GetStatistics(SBStatisticsOptions options); | |||
|
|||
/// Reset the statistics collected for this target. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document this a bit better with more explanation
lldb/source/Core/Module.cpp
Outdated
if (sym_file) { | ||
sym_file->ResetStatistics(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove braces from single line "if" statement per llvm coding guidelines
"statistics dump" currently report the statistics of all targets in debugger instead of current target. This is wrong because there is a "statistics dump --all-targets" option that supposed to include everything. This PR fixes the issue by only report statistics for current target instead of all. It also includes the change to reset statistics debug info/symbol table parsing/indexing time during debugger destroy. This is required so that we report current statistics if we plan to reuse lldb/lldb-dap across debug sessions --------- Co-authored-by: jeffreytan81 <[email protected]> (cherry picked from commit 24feaab)
"statistics dump" currently report the statistics of all targets in debugger instead of current target. This is wrong because there is a "statistics dump --all-targets" option that supposed to include everything. This PR fixes the issue by only report statistics for current target instead of all. It also includes the change to reset statistics debug info/symbol table parsing/indexing time during debugger destroy. This is required so that we report current statistics if we plan to reuse lldb/lldb-dap across debug sessions --------- Co-authored-by: jeffreytan81 <[email protected]> (cherry picked from commit 24feaab)
"statistics dump" currently report the statistics of all targets in debugger instead of current target. This is wrong because there is a "statistics dump --all-targets" option that supposed to include everything.
This PR fixes the issue by only report statistics for current target instead of all. It also includes the change to reset statistics debug info/symbol table parsing/indexing time during debugger destroy. This is required so that we report current statistics if we plan to reuse lldb/lldb-dap across debug sessions