[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/include/lldb/Symbol/SymbolFileOnDemand.h:202-203
+private:
+  bool m_debug_info_enabled{false};
+  bool m_preload_symbols{false};
+  std::unique_ptr<SymbolFile> m_sym_file_impl;
----------------



================
Comment at: lldb/source/Core/CoreProperties.td:39
+    DefaultFalse,
+    Desc<"Enable on demand symbol loading in LLDB. LLDB will load debug info on demand for each module based on breakpoint to improve performance.">;
 }
----------------
We might want to put a URL to something on our website that describes this feature in detail. It isn't just breakpoints that enable debug info, and we will need to document this feature completely so users know what to expect if they enable this.


================
Comment at: lldb/source/Core/Module.cpp:472
 
+    symfile->SetLoadDebugInfoEnabled();
+
----------------
This should only be done if someone is asking for debug info in the "resolve_scope".


================
Comment at: lldb/source/Symbol/SymbolFile.cpp:83
     if (best_symfile_up) {
+      ObjectFile::Type obj_file_type = objfile_sp->CalculateType();
+      if (ModuleList::GetGlobalModuleListProperties().GetLoadSymbolOnDemand() &&
----------------
Add a comment here explaining what is happening


================
Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:28
+
+uint32_t SymbolFileOnDemand::CalculateAbilities() {
+  if (!this->m_debug_info_enabled) {
----------------
We might consider letting this call always go through to the underlying symbol file and report the abilities from it.


================
Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:34-36
+      if (abilities != 0) {
+        LLDB_LOG(log, "{0} abilities would return if hydrated.", abilities);
+      }
----------------
remove braces for a single statement "if" per llvm coding guidelines


================
Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:62-65
+      if (langType != eLanguageTypeUnknown) {
+        LLDB_LOG(log, "Language {0} would return if hydrated.",
+                 eLanguageTypeUnknown);
+      }
----------------
remove braces for a single statement "if" per llvm coding guidelines


================
Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:79-81
+      if (!(sdk == defaultValue)) {
+        LLDB_LOG(log, "SDK {0} would return if hydrated.", sdk.GetString());
+      }
----------------
remove braces for a single statement "if" per llvm coding guidelines


================
Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:97
+
+bool SymbolFileOnDemand::ParseLineTable(CompileUnit &comp_unit) {
+  if (!this->m_debug_info_enabled) {
----------------
We might be able to always let this call through since the line tables will be parsed if the user sets a file and line breakpoint. We are mainly trying to stop the global name lookups in SymbolFileOnDemand


================
Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:172-175
+      if (succeed) {
+        LLDB_LOG(log, "{0} imported modules would be parsed if hydrated.",
+                 tmp_imported_modules.size());
+      }
----------------
remove braces for a single statement "if" per llvm coding guidelines


================
Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:206-208
+      if (resolved_type) {
+        LLDB_LOG(log, "Type would be parsed for {0} if hydrated.", type_uid);
+      }
----------------
remove braces for a single statement "if" per llvm coding guidelines


================
Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:514-516
+lldb::UnwindPlanSP
+SymbolFileOnDemand::GetUnwindPlan(const Address &address,
+                                  const RegisterInfoResolver &resolver) {
----------------
We might consider letting this always go through as unwind info doesn't involve any indexing of debug info.


================
Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:602-604
+  if (m_debug_info_enabled) {
+    return;
+  }
----------------
remove braces for a single statement "if" per llvm coding guidelines


================
Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:608-610
+  if (m_preload_symbols) {
+    PreloadSymbols();
+  }
----------------
remove braces for a single statement "if" per llvm coding guidelines


================
Comment at: lldb/source/Target/Statistics.cpp:66
+  module.try_emplace("debugInfoEnabled", debug_info_enabled);
+  module.try_emplace("symtab_stripped", symtab_stripped);
   if (!symfile_path.empty())
----------------
Other key/value pairs for symbol table stuff start with "symbolTable",


================
Comment at: lldb/source/Target/Statistics.cpp:235-237
+      if (module_stat.symtab_stripped) {
+        ++num_stripped_modules;
+      }
----------------
remove braces for a single statement "if" per llvm coding guidelines


================
Comment at: lldb/source/Target/Statistics.cpp:240-242
+      if (module_stat.debug_info_enabled) {
+        ++num_debug_info_enabled_modules;
+      }
----------------
remove braces for a single statement "if" per llvm coding guidelines


================
Comment at: lldb/source/Target/Statistics.cpp:271
+      {"totalModuleCount", num_modules},
+      {"totalDebugInfoEnabledModules", num_debug_info_enabled_modules},
+      {"totalSymbolTableStrippedModules", num_stripped_modules},
----------------
Other "total" key value pairs use the same string as the ones from the module level to help match them up.


================
Comment at: lldb/source/Target/Statistics.cpp:272
+      {"totalDebugInfoEnabledModules", num_debug_info_enabled_modules},
+      {"totalSymbolTableStrippedModules", num_stripped_modules},
   };
----------------
Other "total" key value pairs use the same string as the ones from the module level to help match them up.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121631/new/

https://reviews.llvm.org/D121631