Author: David Peixotto Date: 2025-03-10T10:54:11-07:00 New Revision: 1d1b20a19ecf51741f2946976ec5f44a0ed53710
URL: https://github.com/llvm/llvm-project/commit/1d1b20a19ecf51741f2946976ec5f44a0ed53710 DIFF: https://github.com/llvm/llvm-project/commit/1d1b20a19ecf51741f2946976ec5f44a0ed53710.diff LOG: [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`. Added: lldb/test/Shell/Commands/command-statistics-dump.test Modified: lldb/include/lldb/Core/Module.h lldb/source/Core/Module.cpp lldb/source/Target/Statistics.cpp lldb/unittests/Symbol/SymtabTest.cpp Removed: ################################################################################ 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..bad7de0ecf61f --- /dev/null +++ b/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, diff --git a/lldb/unittests/Symbol/SymtabTest.cpp b/lldb/unittests/Symbol/SymtabTest.cpp index 7b8892e5b5c0f..e431c31e1eac1 100644 --- a/lldb/unittests/Symbol/SymtabTest.cpp +++ b/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); +} _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
