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

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

Just some notes of mine to find out which functions get used in which breakpoint case:

  -o 'breakpoint set -f main.c -l 1' -o q
  -o 'b main.c:1'     
  lldb/source/Commands/CommandObjectBreakpoint.cpp:578 = eSetTypeFileAndLine
  lldb/source/Target/Target.cpp:330 = GetInlineStrategy()    
  BreakpointSP Target::CreateBreakpoint(const FileSpecList *containingModules,
                                        const FileSpec &file, uint32_t line_no,
    lldb/source/Target/Target.cpp:622
  
  -o 'b main'
  -o 'breakpoint set -n main -f noFileOfThisName.xxx' containingSourceFiles="noFileOfThisName.xxx"
  lldb/source/Commands/CommandObjectBreakpoint.cpp:614 = eSetTypeFunctionName
  lldb/source/Target/Target.cpp:445 
  Target::CreateBreakpoint(const FileSpecList *containingModules,
                           const FileSpecList *containingSourceFiles,
                           const std::vector<std::string> &func_names,
    lldb/source/Target/Target.cpp:622



================
Comment at: lldb/source/Core/SearchFilter.cpp:722
+      return false;
+  }
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74136