[lldb] Remerge llvm#136236 (Avoid force loading symbols in statistics collection (llvm#136795)

Fix a [test
failure](llvm#136236 (comment))
in llvm#136236, apply a minor renaming of statistics, and remerge. See
details below.

# Changes in llvm#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](llvm#136236 (comment))
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](llvm#136226 (comment))
in llvm#136226 after merge.

Author: royitaqi

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

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 {

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

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;

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

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

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;
 

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

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.

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

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