[lldb] Avoid force loading symbol files in statistics collection

[dmpots]

This commit modifies the DebuggerStats::ReportStatistics implementation to avoid loading symbol files for unloaded symbols. We collect stats on debugger shutdown and without this change it can cause the debugger to hang for a long while on shutdown if they symbols were not previously loaded (e.g. settings set target.preload-symbols false).

The implementation is done by adding an optional parameter to Module::GetSymtab to control if the corresponding symbol file will be loaded in the same way that can control it for Module::GetSymbolFile.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: David Peixotto (dmpots)

Changes

This commit modifies the DebuggerStats::ReportStatistics implementation to avoid loading symbol files for unloaded symbols. We collect stats on debugger shutdown and without this change it can cause the debugger to hang for a long while on shutdown if they symbols were not previously loaded (e.g. settings set target.preload-symbols false).

The implementation is done by adding an optional parameter to Module::GetSymtab to control if the corresponding symbol file will be loaded in the same way that can control it for Module::GetSymbolFile.

---

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

4 Files Affected:

• (modified) lldb/include/lldb/Core/Module.h (+5-1)
• (modified) lldb/source/Core/Module.cpp (+2-2)
• (modified) lldb/source/Target/Statistics.cpp (+2-2)
• (added) lldb/test/Shell/Commands/command-statistics-dump.test (+31)

diff --git a/lldb/include/lldb/Core/Module.h b/lldb/include/lldb/Core/Module.h
index 90e0f4b6a3aac..9aa05ed3eb074 100644
--- a/lldb/include/lldb/Core/Module.h
+++ b/lldb/include/lldb/Core/Module.h
@@ -608,7 +608,11 @@ class Module : public std::enable_shared_from_this<Module>,
   virtual SymbolFile *GetSymbolFile(bool can_create = true,
                                     Stream *feedback_strm = nullptr);
 
-  Symtab *GetSymtab();
+  /// Get the module's symbol table
+  ///
+  /// If the symbol table has already been loaded, this function returns it.
+  /// Otherwise, it will only be loaded when can_create is true.
+  Symtab *GetSymtab(bool can_create = true);
 
   /// Get a reference to the UUID value contained in this object.
   ///
diff --git a/lldb/source/Core/Module.cpp b/lldb/source/Core/Module.cpp
index 33668c5d20dde..d70f292abaea4 100644
--- a/lldb/source/Core/Module.cpp
+++ b/lldb/source/Core/Module.cpp
@@ -1022,8 +1022,8 @@ SymbolFile *Module::GetSymbolFile(bool can_create, Stream *feedback_strm) {
   return m_symfile_up ? m_symfile_up->GetSymbolFile() : nullptr;
 }
 
-Symtab *Module::GetSymtab() {
-  if (SymbolFile *symbols = GetSymbolFile())
+Symtab *Module::GetSymtab(bool can_create) {
+  if (SymbolFile *symbols = GetSymbolFile(can_create))
     return symbols->GetSymtab();
   return nullptr;
 }
diff --git a/lldb/source/Target/Statistics.cpp b/lldb/source/Target/Statistics.cpp
index 8173d20457e21..b5d2e7bda1edf 100644
--- a/lldb/source/Target/Statistics.cpp
+++ b/lldb/source/Target/Statistics.cpp
@@ -316,7 +316,7 @@ llvm::json::Value DebuggerStats::ReportStatistics(
     ModuleStats module_stat;
     module_stat.symtab_parse_time = module->GetSymtabParseTime().get().count();
     module_stat.symtab_index_time = module->GetSymtabIndexTime().get().count();
-    Symtab *symtab = module->GetSymtab();
+    Symtab *symtab = module->GetSymtab(/*can_create=*/false);
     if (symtab) {
       module_stat.symtab_loaded_from_cache = symtab->GetWasLoadedFromCache();
       if (module_stat.symtab_loaded_from_cache)
@@ -325,7 +325,7 @@ llvm::json::Value DebuggerStats::ReportStatistics(
       if (module_stat.symtab_saved_to_cache)
         ++symtabs_saved;
     }
-    SymbolFile *sym_file = module->GetSymbolFile();
+    SymbolFile *sym_file = module->GetSymbolFile(/*can_create=*/false);
     if (sym_file) {
       if (!summary_only) {
         if (sym_file->GetObjectFile() != module->GetObjectFile())
diff --git a/lldb/test/Shell/Commands/command-statistics-dump.test b/lldb/test/Shell/Commands/command-statistics-dump.test
new file mode 100644
index 0000000000000..d490a0c374057
--- /dev/null
+++ b/lldb/test/Shell/Commands/command-statistics-dump.test
@@ -0,0 +1,31 @@
+# RUN: %clang_host -g %S/Inputs/main.c -o %t-main.exe
+
+# When we enable symbol preload and dump stats there should be a non-zero
+# time for parsing symbol tables for the main module.
+# RUN: %lldb %t-main.exe \
+# RUN:       -O "settings set plugin.jit-loader.gdb.enable off" \
+# RUN:       -O "settings set target.preload-symbols true" \
+# RUN:       -o "statistics dump" \
+# RUN:       -o "q" \
+# RUN:       | FileCheck %s -check-prefixes=CHECK,PRELOAD_TRUE
+
+# Find the module stats for the main executable and make sure
+# we are looking at the symbol parse time for that module.
+# CHECK: "modules": [
+# CHECK: {
+# CHECK: "path": {{.*}}-main.exe
+# CHECK-NOT: }
+
+# PRELOAD_TRUE: "symbolTableParseTime":
+# PRELOAD_TRUE-SAME: {{[1-9]+}}
+
+# When we disable symbol preload and dump stats the symbol table
+# for main should not be parsed and have a time of 0.
+# RUN: %lldb %t-main.exe \
+# RUN:       -O "settings set plugin.jit-loader.gdb.enable off" \
+# RUN:       -O "settings set target.preload-symbols false" \
+# RUN:       -o "statistics dump" \
+# RUN:       -o "q" \
+# RUN:       | FileCheck %s -check-prefixes=CHECK,PRELOAD_FALSE
+
+# PRELOAD_FALSE: "symbolTableParseTime": 0,

[Jlalond]

I will let Jeffrey and Greg also chime in but this LGTM. My only concern would be testing the did not parse case by looking at a parse time of 0

[dmpots]

My only concern would be testing the did not parse case by looking at a parse time of 0

@Jlalond
I added a unit test to directly exercise the new overload and test that the symbol table is loaded on demand (when can_create is true).

[jeffreytan81]

Looks good other than one comment.