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

[Jim Ingham via Phabricator via lldb-commits]

jingham added inline comments.


================
Comment at: lldb/source/Breakpoint/BreakpointResolverName.cpp:315
+      if (filter_by_cu && filter_by_function) {
+        // Keep this symbol context if it is a function call to a function
+        // whose declaration is located in a file that passes. This is needed
----------------
labath wrote:
> kwk wrote:
> > labath wrote:
> > > This mention of a "function call" is confusing. Either a function is in a file or it isn't. Why do we care about who calls who?
> > @labath we have this `filter_by_function` as an indirection to let `BreakpointResolverName::SearchCallback` know that the `SearchFilter` needs a special handling. But while reviewing the code now I noticed that only the implementations of `SearchFilter::GetFilterRequiredItems` currently either return
> > 
> >   - `eSymbolContextModule` (for `SearchFilterByModule` and `SearchFilterByModuleList`) or  
> >   - `eSymbolContextModule | eSymbolContextCompUnit | eSymbolContextFunction` (for `SearchFilterByModuleListAndCU`). 
> > 
> > I thought this was clever at a time when the old `SearchFilterByModuleListAndCU` which then only returned `eSymbolContextModule | eSymbolContextCompUnit `. With my new search filter I wanted a special handling which is why I wrote a new search filter class and had it return `eSymbolContextModule | eSymbolContextCompUnit | eSymbolContextFunction`. But when you confirmed my question to change the default behavior (https://reviews.llvm.org/D74136#1869622), the new class became the default implementation and there no longer is a need to distinguish the required items for this search filter.
> > 
> > Makes sense?
> I'm afraid I got a bit lost here. Jim is more familiar with his stuff, so maybe he can answer this question. Jim?
> 
> Looking at this again, I get the impression that a lot of this confusion is coming from the fact that this patch assigns a special meaning of  `eSymbolContextCompUnit | eSymbolContextFunction` combination to mean "match the file the function is defined in". I think it would be more consistent with the spirit of these functions if this code just did something like:
> ```
> if (filter_by_function && !filter.FunctionPasses(function))
>   remove_it = true;
> ```
> and then the code which extracts the file from the function definition lived inside the `FunctionPasses` method. That may require adding a new filter class or it could be that an existing filter could be adapted to do that (SearchFilterByModuleListAndCU, I guess).
> 
> Take this with a grain of salt though, as I don't frequent this part of code very often.
I think Pavel is right here.

The point of the built-in checks: ModulePasses, CompUnitPasses, FunctionPasses is that they mirror the search space that the Searcher iterates over when calling the Resolvers.  It allows a Resolver to have the Searcher bypass calling the resolver at all.  Adding in the DeclFile to the CompUnit check isn't right for that use, since that isn't one of the spaces the Searcher iterates over.

The way I thought about this is that all the higher level checks are for the searcher, but ultimately you always have to call AddressPasses because your filter might be below the chunks that the Searcher searches for.

So it seems to me better not to try to glom the CompUnit check and the DeclFile check together, but rather to have the CompUnit check do what it says, eliminate comp units.  And then add the DeclFile check to the AddressPasses for your filter.

It also seems a shame that if I really only want to look only in a CompUnit, I end up having to call into the Resolver for every comp unit, and then filter there.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74136