[lldb] Avoid force loading symbol files in statistics collection (#129593)

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`.

Author: dmpots

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.
   ///

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;
 }

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())

lldb/test/Shell/Commands/command-statistics-dump.test

@@ -0,0 +1,40 @@
+# This test validates that statistics generation does not force loading
+# symbol tables. In order to avoid other sources of symbol loading we
+# create the target without loading dependents and do not actually
+# run it. Running the target is a problem because there are various
+# instrumentation plugins (e.g. ASAN) that are always enabled and force
+# symbol loading. If you see this test start to fail we may have added
+# a new source of symbol loading unexpectedly.
+
+# Build a simple test executable.
+# 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 -O "settings set plugin.jit-loader.gdb.enable off" \
+# RUN:       -O "settings set target.preload-symbols true" \
+# RUN:       -o 'target create --no-dependents "%t-main.exe"' \
+# 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 -O "settings set plugin.jit-loader.gdb.enable off" \
+# RUN:       -O "settings set target.preload-symbols false" \
+# RUN:       -o 'target create --no-dependents "%t-main.exe"' \
+# RUN:       -o "statistics dump" \
+# RUN:       -o "q" \
+# RUN:       | FileCheck %s -check-prefixes=CHECK,PRELOAD_FALSE
+
+# PRELOAD_FALSE: "symbolTableParseTime": 0,

lldb/unittests/Symbol/SymtabTest.cpp

@@ -6,8 +6,10 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "Plugins/ObjectFile/ELF/ObjectFileELF.h"
 #include "Plugins/ObjectFile/Mach-O/ObjectFileMachO.h"
 #include "Plugins/SymbolFile/DWARF/SymbolFileDWARF.h"
+#include "Plugins/SymbolFile/Symtab/SymbolFileSymtab.h"
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
 #include "TestingSupport/SubsystemRAII.h"
 #include "TestingSupport/TestUtilities.h"
@@ -31,7 +33,7 @@ using namespace lldb_private::plugin::dwarf;
 
 class SymtabTest : public testing::Test {
   SubsystemRAII<FileSystem, HostInfo, ObjectFileMachO, SymbolFileDWARF,
-                TypeSystemClang>
+                ObjectFileELF, SymbolFileSymtab, TypeSystemClang>
       subsystem;
 };
 
@@ -718,3 +720,44 @@ TEST_F(SymtabTest, TestDecodeCStringMaps) {
                                                  Symtab::eVisibilityAny);
   ASSERT_NE(symbol, nullptr);
 }
+
+TEST_F(SymtabTest, TestSymtabCreatedOnDemand) {
+  auto ExpectedFile = TestFile::fromYaml(R"(
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+  Entry:           0x0000000000400180
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x0000000000400180
+    AddressAlign:    0x0000000000000010
+    Content:         554889E58B042500106000890425041060005DC3
+Symbols:
+  - Name:            _start
+    Type:            STT_FUNC
+    Section:         .text
+    Value:           0x0000000000400180
+    Size:            0x0000000000000014
+    Binding:         STB_GLOBAL
+...
+)");
+  ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded());
+  auto module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec());
+
+  // The symbol table should not be loaded by default.
+  Symtab *module_symtab = module_sp->GetSymtab(/*can_create=*/false);
+  ASSERT_EQ(module_symtab, nullptr);
+
+  // But it should be created on demand.
+  module_symtab = module_sp->GetSymtab(/*can_create=*/true);
+  ASSERT_NE(module_symtab, nullptr);
+
+  // And we should be able to get it again once it has been created.
+  Symtab *cached_module_symtab = module_sp->GetSymtab(/*can_create=*/false);
+  ASSERT_EQ(module_symtab, cached_module_symtab);
+}