[Lldb-commits] [PATCH] D74136: [LLDB] WIP: Follow DW_AT_decl_file when setting breakpoint

Thu Jun 25 00:29:52 PDT 2020

jankratochvil added inline comments.

================
Comment at: lldb/source/Core/SearchFilter.cpp:722
+      return false;
+  }
+
----------------
jankratochvil wrote:
> This `IsSourceImplementationFile()` should be checking the filename requested by user, not filename found in DWARF. It should be in `SearchFilterByModuleListAndCU::GetFilterRequiredItems()` where it should check the filename in `m_cu_spec_list`. The whole `GetInlineStrategy()` is there only for performance, if it was fast enough it would be always best to use `eInlineBreakpointsAlways`. So here when you already have fully parsed DIE for the function+type it no longer makes sense to check for any performance optimization possibility. If `eInlineBreakpointsNever` or `eInlineBreakpointsHeaders` shows more breakpoint locations than expected (and still not more than `eInlineBreakpointsAlways`) then it is not a bug but just a missed performance optimization.
> And after you fully move this `GetInlineStrategy()+IsSourceImplementationFile()` into `SearchFilterByModuleListAndCU::GetFilterRequiredItems()` then you can drop it from `Target::CreateBreakpoint` at `lldb/source/Target/Target.cpp:330` as it no longer has any effect there.
> IMO one should implement the `DW_AT_decl_file` check also into `SymbolFileDWARF::ResolveSymbolContext` at `lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp`:1977 and other non-DWARF formats. As otherwise properly found candidates can be discarded at the end.
That last `SymbolFileDWARF::ResolveSymbolContext` needs no change, it is already checking `GetSupportFiles()`, discard my last paragraph.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74136