Author: royitaqi Date: 2025-04-24T17:23:41-07:00 New Revision: 967434aa3275842637937e9ac17614a10f81bae7
URL: https://github.com/llvm/llvm-project/commit/967434aa3275842637937e9ac17614a10f81bae7 DIFF: https://github.com/llvm/llvm-project/commit/967434aa3275842637937e9ac17614a10f81bae7.diff LOG: [lldb] Remerge #136236 (Avoid force loading symbols in statistics collection (#136795) Fix a [test failure](https://github.com/llvm/llvm-project/pull/136236#issuecomment-2819772879) in #136236, apply a minor renaming of statistics, and remerge. See details below. # Changes in #136236 Currently, `DebuggerStats::ReportStatistics()` calls `Module::GetSymtab(/*can_create=*/false)`, but then the latter calls `SymbolFile::GetSymtab()`. This will load symbols if haven't yet. See stacktrace below. The problem is that `DebuggerStats::ReportStatistics` should be read-only. This is especially important because it reports stats for symtab parsing/indexing time, which could be affected by the reporting itself if it's not read-only. This patch fixes this problem by adding an optional parameter `SymbolFile::GetSymtab(bool can_create = true)` and receiving the `false` value passed down from `Module::GetSymtab(/*can_create=*/false)` when the call is initiated from `DebuggerStats::ReportStatistics()`. --- Notes about the following stacktrace: 1. This can be reproduced. Create a helloworld program on **macOS** with dSYM, add `settings set target.preload-symbols false` to `~/.lldbinit`, do `lldb a.out`, then `statistics dump`. 2. `ObjectFile::GetSymtab` has `llvm::call_once`. So the fact that it called into `ObjectFileMachO::ParseSymtab` means that the symbol table is actually being parsed. ``` (lldb) bt * thread #1, queue = 'com.apple.main-thread', stop reason = step over frame #0: 0x0000000124c4d5a0 LLDB`ObjectFileMachO::ParseSymtab(this=0x0000000111504e40, symtab=0x0000600000a05e00) at ObjectFileMachO.cpp:2259:44 * frame #1: 0x0000000124fc50a0 LLDB`lldb_private::ObjectFile::GetSymtab()::$_0::operator()(this=0x000000016d35c858) const at ObjectFile.cpp:761:9 frame #5: 0x0000000124fc4e68 LLDB`void std::__1::__call_once_proxy[abi:v160006]<std::__1::tuple<lldb_private::ObjectFile::GetSymtab()::$_0&&>>(__vp=0x000000016d35c7f0) at mutex:652:5 frame #6: 0x0000000198afb99c libc++.1.dylib`std::__1::__call_once(unsigned long volatile&, void*, void (*)(void*)) + 196 frame #7: 0x0000000124fc4dd0 LLDB`void std::__1::call_once[abi:v160006]<lldb_private::ObjectFile::GetSymtab()::$_0>(__flag=0x0000600003920080, __func=0x000000016d35c858) at mutex:670:9 frame #8: 0x0000000124fc3cb0 LLDB`void llvm::call_once<lldb_private::ObjectFile::GetSymtab()::$_0>(flag=0x0000600003920080, F=0x000000016d35c858) at Threading.h:88:5 frame #9: 0x0000000124fc2bc4 LLDB`lldb_private::ObjectFile::GetSymtab(this=0x0000000111504e40) at ObjectFile.cpp:755:5 frame #10: 0x0000000124fe0a28 LLDB`lldb_private::SymbolFileCommon::GetSymtab(this=0x0000000104865200) at SymbolFile.cpp:158:39 frame #11: 0x0000000124d8fedc LLDB`lldb_private::Module::GetSymtab(this=0x00000001113041a8, can_create=false) at Module.cpp:1027:21 frame #12: 0x0000000125125bdc LLDB`lldb_private::DebuggerStats::ReportStatistics(debugger=0x000000014284d400, target=0x0000000115808200, options=0x000000014195d6d1) at Statistics.cpp:329:30 frame #13: 0x0000000125672978 LLDB`CommandObjectStatsDump::DoExecute(this=0x000000014195d540, command=0x000000016d35d820, result=0x000000016d35e150) at CommandObjectStats.cpp:144:18 frame #14: 0x0000000124f29b40 LLDB`lldb_private::CommandObjectParsed::Execute(this=0x000000014195d540, args_string="", result=0x000000016d35e150) at CommandObject.cpp:832:9 frame #15: 0x0000000124efbd70 LLDB`lldb_private::CommandInterpreter::HandleCommand(this=0x0000000141b22f30, command_line="statistics dump", lazy_add_to_history=eLazyBoolCalculate, result=0x000000016d35e150, force_repeat_command=false) at CommandInterpreter.cpp:2134:14 frame #16: 0x0000000124f007f4 LLDB`lldb_private::CommandInterpreter::IOHandlerInputComplete(this=0x0000000141b22f30, io_handler=0x00000001419b2aa8, line="statistics dump") at CommandInterpreter.cpp:3251:3 frame #17: 0x0000000124d7b5ec LLDB`lldb_private::IOHandlerEditline::Run(this=0x00000001419b2aa8) at IOHandler.cpp:588:22 frame #18: 0x0000000124d1e8fc LLDB`lldb_private::Debugger::RunIOHandlers(this=0x000000014284d400) at Debugger.cpp:1225:16 frame #19: 0x0000000124f01f74 LLDB`lldb_private::CommandInterpreter::RunCommandInterpreter(this=0x0000000141b22f30, options=0x000000016d35e63c) at CommandInterpreter.cpp:3543:16 frame #20: 0x0000000122840294 LLDB`lldb::SBDebugger::RunCommandInterpreter(this=0x000000016d35ebd8, auto_handle_events=true, spawn_thread=false) at SBDebugger.cpp:1212:42 frame #21: 0x0000000102aa6d28 lldb`Driver::MainLoop(this=0x000000016d35ebb8) at Driver.cpp:621:18 frame #22: 0x0000000102aa75b0 lldb`main(argc=1, argv=0x000000016d35f548) at Driver.cpp:829:26 frame #23: 0x0000000198858274 dyld`start + 2840 ``` # Changes in this PR top of the above Fix a [test failure](https://github.com/llvm/llvm-project/pull/136236#issuecomment-2819772879) in `TestStats.py`. The original version of the added test checks that all modules have symbol count zero when `target.preload-symbols == false`. The test failed on macOS. Due to various reasons, on macOS, symbols can be loaded for dylibs even with that setting, but not for the main module. For now, the fix of the test is to limit the assertion to only the main module. The test now passes on macOS. In the future, when we have a way to control a specific list of plug-ins to be loaded, there may be a configuration that this test can use to assert that all modules have symbol count zero. Apply a minor renaming of statistics, per the [suggestion](https://github.com/llvm/llvm-project/pull/136226#issuecomment-2825080275) in #136226 after merge. Added: Modified: lldb/include/lldb/Symbol/ObjectFile.h lldb/include/lldb/Symbol/SymbolFile.h lldb/include/lldb/Symbol/SymbolFileOnDemand.h lldb/include/lldb/Target/Statistics.h lldb/source/Core/Module.cpp lldb/source/Symbol/ObjectFile.cpp lldb/source/Symbol/SymbolFile.cpp lldb/source/Target/Statistics.cpp lldb/test/API/commands/statistics/basic/TestStats.py lldb/unittests/Symbol/LineTableTest.cpp lldb/unittests/Symbol/SymtabTest.cpp Removed: ################################################################################ diff --git a/lldb/include/lldb/Symbol/ObjectFile.h b/lldb/include/lldb/Symbol/ObjectFile.h index cfcca04a76de8..7fca6383fa9f3 100644 --- a/lldb/include/lldb/Symbol/ObjectFile.h +++ b/lldb/include/lldb/Symbol/ObjectFile.h @@ -319,7 +319,7 @@ class ObjectFile : public std::enable_shared_from_this<ObjectFile>, /// /// \return /// The symbol table for this object file. - Symtab *GetSymtab(); + Symtab *GetSymtab(bool can_create = true); /// Parse the symbol table into the provides symbol table object. /// diff --git a/lldb/include/lldb/Symbol/SymbolFile.h b/lldb/include/lldb/Symbol/SymbolFile.h index f35d3ee9f22ae..df2f263b18e17 100644 --- a/lldb/include/lldb/Symbol/SymbolFile.h +++ b/lldb/include/lldb/Symbol/SymbolFile.h @@ -144,7 +144,7 @@ class SymbolFile : public PluginInterface { virtual uint32_t GetNumCompileUnits() = 0; virtual lldb::CompUnitSP GetCompileUnitAtIndex(uint32_t idx) = 0; - virtual Symtab *GetSymtab() = 0; + virtual Symtab *GetSymtab(bool can_create = true) = 0; virtual lldb::LanguageType ParseLanguage(CompileUnit &comp_unit) = 0; /// Return the Xcode SDK comp_unit was compiled against. @@ -533,7 +533,7 @@ class SymbolFileCommon : public SymbolFile { return m_abilities; } - Symtab *GetSymtab() override; + Symtab *GetSymtab(bool can_create = true) override; ObjectFile *GetObjectFile() override { return m_objfile_sp.get(); } const ObjectFile *GetObjectFile() const override { diff --git a/lldb/include/lldb/Symbol/SymbolFileOnDemand.h b/lldb/include/lldb/Symbol/SymbolFileOnDemand.h index 7a366bfabec86..6ed389a48880c 100644 --- a/lldb/include/lldb/Symbol/SymbolFileOnDemand.h +++ b/lldb/include/lldb/Symbol/SymbolFileOnDemand.h @@ -186,7 +186,9 @@ class SymbolFileOnDemand : public lldb_private::SymbolFile { uint32_t GetAbilities() override; - Symtab *GetSymtab() override { return m_sym_file_impl->GetSymtab(); } + Symtab *GetSymtab(bool can_create = true) override { + return m_sym_file_impl->GetSymtab(can_create); + } ObjectFile *GetObjectFile() override { return m_sym_file_impl->GetObjectFile(); diff --git a/lldb/include/lldb/Target/Statistics.h b/lldb/include/lldb/Target/Statistics.h index b87a12a8ab9cd..565f1b4351bdd 100644 --- a/lldb/include/lldb/Target/Statistics.h +++ b/lldb/include/lldb/Target/Statistics.h @@ -120,7 +120,7 @@ struct ModuleStats { llvm::StringMap<llvm::json::Value> type_system_stats; double symtab_parse_time = 0.0; double symtab_index_time = 0.0; - uint32_t num_symbols_loaded = 0; + uint32_t symtab_symbol_count = 0; double debug_parse_time = 0.0; double debug_index_time = 0.0; uint64_t debug_info_size = 0; diff --git a/lldb/source/Core/Module.cpp b/lldb/source/Core/Module.cpp index ad7046e596278..625c14e4a2153 100644 --- a/lldb/source/Core/Module.cpp +++ b/lldb/source/Core/Module.cpp @@ -997,7 +997,7 @@ SymbolFile *Module::GetSymbolFile(bool can_create, Stream *feedback_strm) { Symtab *Module::GetSymtab(bool can_create) { if (SymbolFile *symbols = GetSymbolFile(can_create)) - return symbols->GetSymtab(); + return symbols->GetSymtab(can_create); return nullptr; } diff --git a/lldb/source/Symbol/ObjectFile.cpp b/lldb/source/Symbol/ObjectFile.cpp index 2f2c59d6af620..afd0d298e675e 100644 --- a/lldb/source/Symbol/ObjectFile.cpp +++ b/lldb/source/Symbol/ObjectFile.cpp @@ -734,10 +734,9 @@ void llvm::format_provider<ObjectFile::Strata>::format( } } - -Symtab *ObjectFile::GetSymtab() { +Symtab *ObjectFile::GetSymtab(bool can_create) { ModuleSP module_sp(GetModule()); - if (module_sp) { + if (module_sp && can_create) { // We can't take the module lock in ObjectFile::GetSymtab() or we can // deadlock in DWARF indexing when any file asks for the symbol table from // an object file. This currently happens in the preloading of symbols in diff --git a/lldb/source/Symbol/SymbolFile.cpp b/lldb/source/Symbol/SymbolFile.cpp index 94e32b55572dd..870d778dca740 100644 --- a/lldb/source/Symbol/SymbolFile.cpp +++ b/lldb/source/Symbol/SymbolFile.cpp @@ -152,10 +152,10 @@ void SymbolFile::AssertModuleLock() { SymbolFile::RegisterInfoResolver::~RegisterInfoResolver() = default; -Symtab *SymbolFileCommon::GetSymtab() { +Symtab *SymbolFileCommon::GetSymtab(bool can_create) { std::lock_guard<std::recursive_mutex> guard(GetModuleMutex()); // Fetch the symtab from the main object file. - auto *symtab = GetMainObjectFile()->GetSymtab(); + auto *symtab = GetMainObjectFile()->GetSymtab(can_create); if (m_symtab != symtab) { m_symtab = symtab; diff --git a/lldb/source/Target/Statistics.cpp b/lldb/source/Target/Statistics.cpp index 2bb93dfffa5aa..12e7190ae1f74 100644 --- a/lldb/source/Target/Statistics.cpp +++ b/lldb/source/Target/Statistics.cpp @@ -71,7 +71,7 @@ json::Value ModuleStats::ToJSON() const { module.try_emplace("debugInfoHadIncompleteTypes", debug_info_had_incomplete_types); module.try_emplace("symbolTableStripped", symtab_stripped); - module.try_emplace("symbolsLoaded", num_symbols_loaded); + module.try_emplace("symbolTableSymbolCount", symtab_symbol_count); if (!symfile_path.empty()) module.try_emplace("symbolFilePath", symfile_path); @@ -311,7 +311,7 @@ llvm::json::Value DebuggerStats::ReportStatistics( uint32_t num_modules_with_variable_errors = 0; uint32_t num_modules_with_incomplete_types = 0; uint32_t num_stripped_modules = 0; - uint32_t num_symbols_loaded = 0; + uint32_t symtab_symbol_count = 0; for (size_t image_idx = 0; image_idx < num_modules; ++image_idx) { Module *module = target != nullptr ? target->GetImages().GetModuleAtIndex(image_idx).get() @@ -321,8 +321,8 @@ llvm::json::Value DebuggerStats::ReportStatistics( module_stat.symtab_index_time = module->GetSymtabIndexTime().get().count(); Symtab *symtab = module->GetSymtab(/*can_create=*/false); if (symtab) { - module_stat.num_symbols_loaded = symtab->GetNumSymbols(); - num_symbols_loaded += module_stat.num_symbols_loaded; + module_stat.symtab_symbol_count = symtab->GetNumSymbols(); + symtab_symbol_count += module_stat.symtab_symbol_count; ++symtabs_loaded; module_stat.symtab_loaded_from_cache = symtab->GetWasLoadedFromCache(); if (module_stat.symtab_loaded_from_cache) @@ -414,7 +414,7 @@ llvm::json::Value DebuggerStats::ReportStatistics( num_modules_with_incomplete_types}, {"totalDebugInfoEnabled", num_debug_info_enabled_modules}, {"totalSymbolTableStripped", num_stripped_modules}, - {"totalSymbolsLoaded", num_symbols_loaded}, + {"totalSymbolTableSymbolCount", symtab_symbol_count}, }; if (include_targets) { diff --git a/lldb/test/API/commands/statistics/basic/TestStats.py b/lldb/test/API/commands/statistics/basic/TestStats.py index a358f5178c7fa..a9a7e933a3211 100644 --- a/lldb/test/API/commands/statistics/basic/TestStats.py +++ b/lldb/test/API/commands/statistics/basic/TestStats.py @@ -170,7 +170,7 @@ def test_default_no_run(self): "totalSymbolTableIndexTime", "totalSymbolTablesLoadedFromCache", "totalSymbolTablesSavedToCache", - "totalSymbolsLoaded", + "totalSymbolTableSymbolCount", "totalSymbolTablesLoaded", "totalDebugInfoByteSize", "totalDebugInfoIndexTime", @@ -180,7 +180,7 @@ def test_default_no_run(self): ] self.verify_keys(debug_stats, '"debug_stats"', debug_stat_keys, None) if self.getPlatform() != "windows": - self.assertGreater(debug_stats["totalSymbolsLoaded"], 0) + self.assertGreater(debug_stats["totalSymbolTableSymbolCount"], 0) self.assertGreater(debug_stats["totalSymbolTablesLoaded"], 0) # Verify target stats keys. @@ -203,13 +203,34 @@ def test_default_no_run(self): # Verify module stats keys. for module_stats in debug_stats["modules"]: module_stat_keys_exist = [ - "symbolsLoaded", + "symbolTableSymbolCount", ] self.verify_keys( module_stats, '"module_stats"', module_stat_keys_exist, None ) if self.getPlatform() != "windows": - self.assertGreater(module_stats["symbolsLoaded"], 0) + self.assertGreater(module_stats["symbolTableSymbolCount"], 0) + + def test_default_no_run_no_preload_symbols(self): + """Test "statistics dump" without running the target and without + preloading symbols. + + Checks that symbol count are zero. + """ + # Make sure symbols will not be preloaded. + self.runCmd("settings set target.preload-symbols false") + + # Build and load the target + self.build() + self.createTestTarget() + + # Get statistics + debug_stats = self.get_stats() + + # No symbols should be loaded in the main module. + main_module_stats = debug_stats["modules"][0] + if self.getPlatform() != "windows": + self.assertEqual(main_module_stats["symbolTableSymbolCount"], 0) def test_default_with_run(self): """Test "statistics dump" when running the target to a breakpoint. diff --git a/lldb/unittests/Symbol/LineTableTest.cpp b/lldb/unittests/Symbol/LineTableTest.cpp index f67ac46c738e7..eadab40a37fac 100644 --- a/lldb/unittests/Symbol/LineTableTest.cpp +++ b/lldb/unittests/Symbol/LineTableTest.cpp @@ -58,7 +58,7 @@ class FakeSymbolFile : public SymbolFile { uint32_t CalculateAbilities() override { return UINT32_MAX; } uint32_t GetNumCompileUnits() override { return 1; } CompUnitSP GetCompileUnitAtIndex(uint32_t) override { return m_cu_sp; } - Symtab *GetSymtab() override { return nullptr; } + Symtab *GetSymtab(bool can_create = true) override { return nullptr; } LanguageType ParseLanguage(CompileUnit &) override { return eLanguageTypeC; } size_t ParseFunctions(CompileUnit &) override { return 0; } bool ParseLineTable(CompileUnit &) override { return true; } diff --git a/lldb/unittests/Symbol/SymtabTest.cpp b/lldb/unittests/Symbol/SymtabTest.cpp index e431c31e1eac1..d3f462fab86a1 100644 --- a/lldb/unittests/Symbol/SymtabTest.cpp +++ b/lldb/unittests/Symbol/SymtabTest.cpp @@ -721,7 +721,7 @@ TEST_F(SymtabTest, TestDecodeCStringMaps) { ASSERT_NE(symbol, nullptr); } -TEST_F(SymtabTest, TestSymtabCreatedOnDemand) { +TEST_F(SymtabTest, TestSymbolFileCreatedOnDemand) { auto ExpectedFile = TestFile::fromYaml(R"( --- !ELF FileHeader: @@ -749,7 +749,7 @@ TEST_F(SymtabTest, TestSymtabCreatedOnDemand) { ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded()); auto module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec()); - // The symbol table should not be loaded by default. + // The symbol file should not be created by default. Symtab *module_symtab = module_sp->GetSymtab(/*can_create=*/false); ASSERT_EQ(module_symtab, nullptr); @@ -761,3 +761,77 @@ TEST_F(SymtabTest, TestSymtabCreatedOnDemand) { Symtab *cached_module_symtab = module_sp->GetSymtab(/*can_create=*/false); ASSERT_EQ(module_symtab, cached_module_symtab); } + +TEST_F(SymtabTest, TestSymbolTableCreatedOnDemand) { + const char *yamldata = R"( +--- !ELF +FileHeader: + Class: ELFCLASS64 + Data: ELFDATA2LSB + Type: ET_EXEC + Machine: EM_386 +DWARF: + debug_abbrev: + - Table: + - Code: 0x00000001 + Tag: DW_TAG_compile_unit + Children: DW_CHILDREN_no + Attributes: + - Attribute: DW_AT_addr_base + Form: DW_FORM_sec_offset + debug_info: + - Version: 5 + AddrSize: 4 + UnitType: DW_UT_compile + Entries: + - AbbrCode: 0x00000001 + Values: + - Value: 0x8 # Offset of the first Address past the header + - AbbrCode: 0x0 + debug_addr: + - Version: 5 + AddressSize: 4 + Entries: + - Address: 0x1234 + - Address: 0x5678 + debug_line: + - Length: 42 + Version: 2 + PrologueLength: 36 + MinInstLength: 1 + DefaultIsStmt: 1 + LineBase: 251 + LineRange: 14 + OpcodeBase: 13 + StandardOpcodeLengths: [ 0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1 ] + IncludeDirs: + - '/tmp' + Files: + - Name: main.cpp + DirIdx: 1 + ModTime: 0 + Length: 0 +)"; + llvm::Expected<TestFile> file = TestFile::fromYaml(yamldata); + EXPECT_THAT_EXPECTED(file, llvm::Succeeded()); + auto module_sp = std::make_shared<Module>(file->moduleSpec()); + + SymbolFile *symbol_file = module_sp->GetSymbolFile(); + // At this point, the symbol table is not created. This is because the above + // yaml data contains the necessary sections in order for + // SymbolFileDWARF::CalculateAbilities() to identify all abilities, saving it + // from calling SymbolFileDWARFDebugMap::CalculateAbilities(), which + // eventually loads the symbol table, which we don't want. + + // The symbol table should not be created if asked not to. + Symtab *symtab = symbol_file->GetSymtab(/*can_create=*/false); + ASSERT_EQ(symtab, nullptr); + + // But it should be created on demand. + symtab = symbol_file->GetSymtab(/*can_create=*/true); + ASSERT_NE(symtab, nullptr); + + // And we should be able to get it again once it has been created. + Symtab *cached_symtab = symbol_file->GetSymtab(/*can_create=*/false); + ASSERT_EQ(symtab, cached_symtab); +} _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
