https://github.com/royitaqi created https://github.com/llvm/llvm-project/pull/136795
Fix a [test failure](https://github.com/llvm/llvm-project/pull/136236#issuecomment-2819772879) in #136236 and remerge. >From 85cb8cca4fe41cf4080b3dbf7ce4f98c4b15a3c7 Mon Sep 17 00:00:00 2001 From: Roy Shi <roy...@meta.com> Date: Thu, 17 Apr 2025 15:03:00 -0700 Subject: [PATCH 1/9] [lldb] Do not parse symtab during statistics dump Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D73218490 --- lldb/include/lldb/Symbol/ObjectFile.h | 2 +- lldb/include/lldb/Symbol/SymbolFile.h | 4 ++-- lldb/include/lldb/Symbol/SymbolFileOnDemand.h | 2 +- lldb/source/Core/Module.cpp | 2 +- lldb/source/Symbol/ObjectFile.cpp | 4 ++-- lldb/source/Symbol/SymbolFile.cpp | 4 ++-- 6 files changed, 9 insertions(+), 9 deletions(-) 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..ce9fc8626ac34 100644 --- a/lldb/include/lldb/Symbol/SymbolFileOnDemand.h +++ b/lldb/include/lldb/Symbol/SymbolFileOnDemand.h @@ -186,7 +186,7 @@ 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/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..292c00b9ac166 100644 --- a/lldb/source/Symbol/ObjectFile.cpp +++ b/lldb/source/Symbol/ObjectFile.cpp @@ -735,9 +735,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; >From 1fccd09dde2d1f8da6f8253516c5c908049eb270 Mon Sep 17 00:00:00 2001 From: Roy Shi <roy...@meta.com> Date: Thu, 17 Apr 2025 21:45:48 -0700 Subject: [PATCH 2/9] Fix compilation of a unit test --- lldb/unittests/Symbol/LineTableTest.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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; } >From 2c80507e49724a70688225c44e75400a18884e09 Mon Sep 17 00:00:00 2001 From: Roy Shi <roy...@meta.com> Date: Thu, 17 Apr 2025 22:38:57 -0700 Subject: [PATCH 3/9] Fixing format (hopefully) and trying to add a test --- lldb/include/lldb/Symbol/SymbolFileOnDemand.h | 4 +++- lldb/source/Symbol/ObjectFile.cpp | 1 - lldb/unittests/Symbol/SymtabTest.cpp | 14 ++++++++++++-- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/lldb/include/lldb/Symbol/SymbolFileOnDemand.h b/lldb/include/lldb/Symbol/SymbolFileOnDemand.h index ce9fc8626ac34..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(bool can_create = true) override { return m_sym_file_impl->GetSymtab(can_create); } + 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/source/Symbol/ObjectFile.cpp b/lldb/source/Symbol/ObjectFile.cpp index 292c00b9ac166..afd0d298e675e 100644 --- a/lldb/source/Symbol/ObjectFile.cpp +++ b/lldb/source/Symbol/ObjectFile.cpp @@ -734,7 +734,6 @@ void llvm::format_provider<ObjectFile::Strata>::format( } } - Symtab *ObjectFile::GetSymtab(bool can_create) { ModuleSP module_sp(GetModule()); if (module_sp && can_create) { diff --git a/lldb/unittests/Symbol/SymtabTest.cpp b/lldb/unittests/Symbol/SymtabTest.cpp index e431c31e1eac1..05208ca07ee08 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, TestSymbolFileAndSymbolTableCreatedOnDemand) { auto ExpectedFile = TestFile::fromYaml(R"( --- !ELF FileHeader: @@ -749,10 +749,20 @@ 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); + // Even if the symbol file is created, the symbol table should not be created by default. + + // TODO: + // I need to create a symbol file here, but without causing it to parse the symbol table. + // See next line as a failed attempt. + + // module_sp->GetSymbolFile(/*can_create=*/true); // Cannot do this because it will parse the symbol table. + 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); >From 34dc7de6b6a696957e2968817011a0bccff1ba5f Mon Sep 17 00:00:00 2001 From: Roy Shi <roy...@meta.com> Date: Sat, 19 Apr 2025 10:40:23 -0700 Subject: [PATCH 4/9] Add unit test --- lldb/unittests/Symbol/SymtabTest.cpp | 83 ++++++++++++++++++++++++---- 1 file changed, 72 insertions(+), 11 deletions(-) diff --git a/lldb/unittests/Symbol/SymtabTest.cpp b/lldb/unittests/Symbol/SymtabTest.cpp index 05208ca07ee08..dbc3be9190568 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, TestSymbolFileAndSymbolTableCreatedOnDemand) { +TEST_F(SymtabTest, TestSymbolFileCreatedOnDemand) { auto ExpectedFile = TestFile::fromYaml(R"( --- !ELF FileHeader: @@ -753,16 +753,6 @@ TEST_F(SymtabTest, TestSymbolFileAndSymbolTableCreatedOnDemand) { Symtab *module_symtab = module_sp->GetSymtab(/*can_create=*/false); ASSERT_EQ(module_symtab, nullptr); - // Even if the symbol file is created, the symbol table should not be created by default. - - // TODO: - // I need to create a symbol file here, but without causing it to parse the symbol table. - // See next line as a failed attempt. - - // module_sp->GetSymbolFile(/*can_create=*/true); // Cannot do this because it will parse the symbol table. - 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); @@ -771,3 +761,74 @@ TEST_F(SymtabTest, TestSymbolFileAndSymbolTableCreatedOnDemand) { 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 the code 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 = symbol_file->GetSymtab(/*can_create=*/false); + ASSERT_NE(symtab, nullptr); +} >From afeaa369a65e91065394261ce29bce1b92047a62 Mon Sep 17 00:00:00 2001 From: Roy Shi <roy...@meta.com> Date: Sat, 19 Apr 2025 10:43:16 -0700 Subject: [PATCH 5/9] Fix format --- lldb/unittests/Symbol/SymtabTest.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lldb/unittests/Symbol/SymtabTest.cpp b/lldb/unittests/Symbol/SymtabTest.cpp index dbc3be9190568..3af708b0bc8d3 100644 --- a/lldb/unittests/Symbol/SymtabTest.cpp +++ b/lldb/unittests/Symbol/SymtabTest.cpp @@ -816,12 +816,15 @@ TEST_F(SymtabTest, TestSymbolTableCreatedOnDemand) { 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 the code from calling SymbolFileDWARFDebugMap::CalculateAbilities(), which eventually loads the symbol table, which we don't want. + 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); + Symtab *symtab = symbol_file->GetSymtab(/*can_create=*/false); ASSERT_EQ(symtab, nullptr); // But it should be created on demand. >From e21be173b3e1cdd48ec31588fa8af02ec76748a4 Mon Sep 17 00:00:00 2001 From: Roy Shi <roy...@meta.com> Date: Mon, 21 Apr 2025 15:51:10 -0700 Subject: [PATCH 6/9] Add API test to verify that stats dump does not load symbols --- .../commands/statistics/basic/TestStats.py | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/lldb/test/API/commands/statistics/basic/TestStats.py b/lldb/test/API/commands/statistics/basic/TestStats.py index 0265a0d7c9948..f696c0c948778 100644 --- a/lldb/test/API/commands/statistics/basic/TestStats.py +++ b/lldb/test/API/commands/statistics/basic/TestStats.py @@ -209,6 +209,31 @@ def test_default_no_run(self): ) self.assertGreater(module_stats["symbolsLoaded"], 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() + target = self.createTestTarget() + + # Get statistics + debug_stats = self.get_stats() + + # No symbols should be loaded + self.assertEqual(debug_stats["totalSymbolsLoaded"], 0) + + # No symbols should be loaded in each module + for module_stats in debug_stats["modules"]: + self.assertEqual(module_stats["symbolsLoaded"], 0) + + def test_default_with_run(self): """Test "statistics dump" when running the target to a breakpoint. >From 4b9b7917e0a3257294307fa01a3060402cabe6a4 Mon Sep 17 00:00:00 2001 From: Roy Shi <roy...@meta.com> Date: Mon, 21 Apr 2025 15:56:47 -0700 Subject: [PATCH 7/9] Update C++ unittest according to David's suggestion --- lldb/unittests/Symbol/SymtabTest.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lldb/unittests/Symbol/SymtabTest.cpp b/lldb/unittests/Symbol/SymtabTest.cpp index 3af708b0bc8d3..d3f462fab86a1 100644 --- a/lldb/unittests/Symbol/SymtabTest.cpp +++ b/lldb/unittests/Symbol/SymtabTest.cpp @@ -832,6 +832,6 @@ TEST_F(SymtabTest, TestSymbolTableCreatedOnDemand) { ASSERT_NE(symtab, nullptr); // And we should be able to get it again once it has been created. - symtab = symbol_file->GetSymtab(/*can_create=*/false); - ASSERT_NE(symtab, nullptr); + Symtab *cached_symtab = symbol_file->GetSymtab(/*can_create=*/false); + ASSERT_EQ(symtab, cached_symtab); } >From 80ac0f557e7965850c17a52e86aab6970dbcf1b2 Mon Sep 17 00:00:00 2001 From: Roy Shi <roy...@meta.com> Date: Mon, 21 Apr 2025 16:04:20 -0700 Subject: [PATCH 8/9] Fix format --- lldb/test/API/commands/statistics/basic/TestStats.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/lldb/test/API/commands/statistics/basic/TestStats.py b/lldb/test/API/commands/statistics/basic/TestStats.py index f696c0c948778..e2c539bfaeddb 100644 --- a/lldb/test/API/commands/statistics/basic/TestStats.py +++ b/lldb/test/API/commands/statistics/basic/TestStats.py @@ -209,7 +209,6 @@ def test_default_no_run(self): ) self.assertGreater(module_stats["symbolsLoaded"], 0) - def test_default_no_run_no_preload_symbols(self): """Test "statistics dump" without running the target and without preloading symbols. @@ -233,7 +232,6 @@ def test_default_no_run_no_preload_symbols(self): for module_stats in debug_stats["modules"]: self.assertEqual(module_stats["symbolsLoaded"], 0) - def test_default_with_run(self): """Test "statistics dump" when running the target to a breakpoint. >From 06f33f325cda22f2a6f1ac0a0fc6831634e06733 Mon Sep 17 00:00:00 2001 From: Roy Shi <roy...@meta.com> Date: Tue, 22 Apr 2025 17:31:43 -0700 Subject: [PATCH 9/9] Fix test on macOS --- lldb/test/API/commands/statistics/basic/TestStats.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/lldb/test/API/commands/statistics/basic/TestStats.py b/lldb/test/API/commands/statistics/basic/TestStats.py index e2c539bfaeddb..e1f8dcfa71a5b 100644 --- a/lldb/test/API/commands/statistics/basic/TestStats.py +++ b/lldb/test/API/commands/statistics/basic/TestStats.py @@ -220,17 +220,14 @@ def test_default_no_run_no_preload_symbols(self): # Build and load the target self.build() - target = self.createTestTarget() + self.createTestTarget() # Get statistics debug_stats = self.get_stats() - # No symbols should be loaded - self.assertEqual(debug_stats["totalSymbolsLoaded"], 0) - - # No symbols should be loaded in each module - for module_stats in debug_stats["modules"]: - self.assertEqual(module_stats["symbolsLoaded"], 0) + # No symbols should be loaded in the main module. + main_module_stats = debug_stats["modules"][0] + self.assertEqual(main_module_stats["symbolsLoaded"], 0) def test_default_with_run(self): """Test "statistics dump" when running the target to a breakpoint. _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
