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

[jeffrey tan via Phabricator via lldb-commits]

yinghuitan marked 19 inline comments as done.
yinghuitan added inline comments.


================
Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:28
+
+uint32_t SymbolFileOnDemand::CalculateAbilities() {
+  if (!this->m_debug_info_enabled) {
----------------
clayborg wrote:
> We might consider letting this call always go through to the underlying symbol file and report the abilities from it.
Sure. It does not matter much in real world because ability is never calculated on SymbolFileOnDemand. 


================
Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:97
+
+bool SymbolFileOnDemand::ParseLineTable(CompileUnit &comp_unit) {
+  if (!this->m_debug_info_enabled) {
----------------
clayborg wrote:
> 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
If the user sets a file and line breakpoint and a compile unit/supported file is matched against the breakpoint file its parent module's SymbolFileOnDemand will be hydrated before ParseLineTable() is called anyway so manual call through is unnecessary.
I prefer to keep things not pass through by default to prevent any accidental leaking. 


================
Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:514-516
+lldb::UnwindPlanSP
+SymbolFileOnDemand::GetUnwindPlan(const Address &address,
+                                  const RegisterInfoResolver &resolver) {
----------------
clayborg wrote:
> We might consider letting this always go through as unwind info doesn't involve any indexing of debug info.
Yes, we could. But I would like to keep all functions guarded by default unless explicitly required by a hydration scenario to prevent any accidental leakage.


================
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())
----------------
clayborg wrote:
> Other key/value pairs for symbol table stuff start with "symbolTable",
Ah, thanks! Bad auto replace for variable name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121631