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

I would separate/remove all [nfc] changes from this patch as they complicate it a bit. Such cleaned up patch I have prepared for myself: https://people.redhat.com/jkratoch/D74136-cleanup.patch

Then also did you notice there is a regression for (I do not know why):

  FAIL: LLDB (/home/jkratoch/redhat/llvm-monorepo-clangassert/bin/clang-11-x86_64) :: test_scripted_resolver (TestScriptedResolver.TestScriptedResolver)
    File "/home/jkratoch/redhat/llvm-monorepo/lldb/test/API/functionalities/breakpoint/scripted_bkpt/TestScriptedResolver.py", line 23, in test_scripted_resolver
      self.do_test()
    File "/home/jkratoch/redhat/llvm-monorepo/lldb/test/API/functionalities/breakpoint/scripted_bkpt/TestScriptedResolver.py", line 152, in do_test
      self.assertEqual(wrong[i].GetNumLocations(), 0, "Breakpoint %d has locations."%(i))
  AssertionError: 1 != 0 : Breakpoint 1 has locations.



================
Comment at: lldb/source/Breakpoint/BreakpointResolverName.cpp:292
+            filter_by_function), // include symbols only if we
+                                 // aren't filtering by CU or function
           include_inlines, func_list);
----------------
[nfc] It could use `include_symbols` here.



================
Comment at: lldb/source/Core/SearchFilter.cpp:713
+  if (!type)
+    return SearchFilterByModuleList::FunctionPasses(function);
+
----------------
If we cannot determine which file the function is from then rather ignore it (the current call returns `true`):
```
  return false;
```


================
Comment at: lldb/source/Core/SearchFilter.cpp:726
+  if (m_target_sp &&
+      m_target_sp->GetInlineStrategy() == eInlineBreakpointsHeaders) {
+    if (!sym_ctx.comp_unit) {
----------------
This value should be cached.
But besides that there should be `GetInlineStrategy() == eInlineBreakpointsNever || (GetInlineStrategy() == eInlineBreakpointsHeaders && cu_spec.IsSourceImplementationFile()modulosomeReverseRemapPath?`.
As for `eInlineBreakpointsAlways` the check is being delegated to `FunctionPasses()`.



================
Comment at: lldb/source/Core/SearchFilter.cpp:732
+    FileSpec cu_spec;
+    if (sym_ctx.comp_unit) {
+      cu_spec = sym_ctx.comp_unit->GetPrimaryFile();
----------------
This condition is always `true` as there is already above:
```
if (!sym_ctx.comp_unit)
```



================
Comment at: lldb/source/Core/SearchFilter.cpp:835
+  if (m_target_sp &&
+      m_target_sp->GetInlineStrategy() == eInlineBreakpointsHeaders)
+    return flags | eSymbolContextCompUnit;
----------------
Here should be `eInlineBreakpointsNever || eInlineBreakpointsHeaders` (and probably cache the `GetInlineStrategy()` value).
For `eInlineBreakpointsHeaders` we do not know for sure as we cannot call `IsSourceImplementationFile()` for `cu_spec` which is not known yet here.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74136