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

In D74136#1983467 <https://reviews.llvm.org/D74136#1983467>, @kwk wrote:

> I'd be happy to hear about your comments @labath because I'm kind of stuck in what I can think of. I will obviously rename `SearchFilterByModuleListAndCU` but that can come in a child revision.


Sorry, I'm kinda busy these days, so and I did not see the embedded question while glossing over the WIP changes.

I think this looks pretty clean (surprisingly clean even) now. I don't have any major comments, but we should have Jim take a look at this again, as he's more familiar with this stuff.

Given that the name `SearchFilterByModuleListAndCU` is not very widespread (20 hits in three files) and that this patch actually makes that name wrong, I think it'd be best to do the rename as a part of this patch.



================
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
----------------
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?


================
Comment at: lldb/source/Breakpoint/BreakpointResolverName.cpp:321
+          FileSpec &source_file = decl.GetFile();
+          if (!filter.CompUnitPasses(source_file))
+            remove_it = true;
----------------
This `CompUnitPasses` call smells, as `source_file` does not really correspond to any compilation unit (in the scenario that you care about). It seems like this is actually the first usage of this function, so let's just rename it (and make it take a `const FileSpec &` while we're at it).


================
Comment at: lldb/test/Shell/Breakpoint/Inputs/search-support-files.h:1
+int inlined_42() { return 42; }
----------------
Calling this `inlined` is misleading. The function won't get inlined anywhere at -O0, and in fact your test would not work if it got inlined. Maybe just call it `function_in_header` ?


================
Comment at: lldb/test/Shell/Breakpoint/search-support-files.test:8
+
+# RUN: %build %p/Inputs/search-support-files.cpp -o dummy.out
+# RUN: %lldb -b -s %s dummy.out | FileCheck --color --dump-input=fail %s 
----------------
All of the tests execute in the same directory, so it's a good practice to embed `%t` into the files you create. As you don't care about the actual file name in the CHECK lines, you can just replace dummy.out with `{{.*}}` everywhere.


================
Comment at: lldb/test/Shell/Breakpoint/search-support-files.test:9
+# RUN: %build %p/Inputs/search-support-files.cpp -o dummy.out
+# RUN: %lldb -b -s %s dummy.out | FileCheck --color --dump-input=fail %s 
+
----------------
It's not standard/polite to hard-code --color like this. Using --dump-input is also not very common though there are definitely precedents for that, and I can see how it can be useful (though one should be careful not to overwhelm the terminal if he's using it).


================
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{{.*}}
+
----------------
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
```


================
Comment at: lldb/test/Shell/Breakpoint/search-support-files.test:32
+#   NOTE: This test is the really interesting one as it shows that we can
+#         search by source files that are themselves no compulation units.
+
----------------
compilation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74136