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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

At this point I'm getting very lost in this patch, but here's some of my thoughs (the ones I could actually formulate in a coherent manner)...



================
Comment at: lldb/source/Core/SearchFilter.cpp:713
+  if (!type)
+    return SearchFilterByModuleList::FunctionPasses(function);
+
----------------
kwk wrote:
> jankratochvil wrote:
> > If we cannot determine which file the function is from then rather ignore it (the current call returns `true`):
> > ```
> >   return false;
> > ```
> @jankratochvil so far I haven't addressed this on purpose to give @labath and @jingham time to say what they think about this.
Yes, ignoring the function if we cannot determine which file it is in sounds reasonable. (though I'm not sure if this type argument can ever be null in practice.)


================
Comment at: lldb/test/API/functionalities/breakpoint/scripted_bkpt/TestScriptedResolver.py:124
-        file_list.Append(lldb.SBFileSpec("noFileOfThisName.xxx"))
-        wrong.append(target.BreakpointCreateFromScript("resolver.Resolver", extra_args, module_list, file_list))
-
----------------
kwk wrote:
> @jingham can you please let me know if this test serves any purpose with my patch? I mentioned in my last change log (https://reviews.llvm.org/D74136#2113899) that it calls `AddressPasses` and checked for the CU before. But since we're no longer doing this, this test no longer can pass. 
After reading more about it, I'm pretty sure this is not the right solution. The scripted breakpoints are documented ([[ https://github.com/llvm/llvm-project/blame/master/lldb/docs/use/python-reference.rst#L390 | here ]]) as accepting filters based on compile units. If we don't want to change that, then I guess we need to create a separate filter, so that the scripted breakpoints can filter based on compile unit names and the name+file breakpoints can filter on definitions file names.

If we want to change that, then we'd also need to update the documentation. But, the test should stay anyway, because `noFileOfThisName.xxx` is not a valid file nor compile unit name, so the location should not be added regardless of what exactly are we choosing to filter on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74136