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

[Pavel Labath via Phabricator via lldb-commits]

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


================
Comment at: lldb/test/Shell/Breakpoint/search-support-files.test:15
+# CHECK: (lldb) breakpoint set -n inlined_42
+# CHECK-NEXT: Breakpoint 1: where = dummy.out`inlined_42() + 4 at search-support-files.h:1:20, address = 0x0{{.*}}
+
----------------
kwk wrote:
> labath wrote:
> > These check lines hardcode too much stuff. The `+4` thingy can easily change due to unrelated codegen changes, and even the `:1:20` seems unnecessarily strict.
> > Maybe something like this would be enough:
> > ```
> > CHECK-NEXT: Breakpoint 1: where = {{.8}}`inlined_42{{.*}} at search-support-files.h
> > ```
> Yes, you're right. Although I don't know what  `{{.8}}` does in this case. 
I meant `{{.*}}`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74136