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

jankratochvil added a comment. In D74136#2119549 , @labath wrote: > At this point I'm getting very lost in this patch, Me too, as you stated: In D74136#2020245 , @labath wrote: > In D74136#1983467

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

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

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

labath added a comment. In D74136#2118983 , @jankratochvil wrote: > The filename should not be checked from `SymbolContext::function` but rather > from `SymbolContext::line_entry`. As that is cheaper. And when one asks for > breakpoint at `1a.h:1`

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

jankratochvil added a comment. The filename should not be checked from `SymbolContext::function` but rather from `SymbolContext::line_entry`. As that is cheaper. And when one asks for breakpoint at `1a.h:1` then it is enough to check `.debug_line` (which needs to be checked anyway) and why

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

kwk updated this revision to Diff 273637. kwk added a comment. - Simplify logic Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74136/new/ https://reviews.llvm.org/D74136 Files: lldb/include/lldb/Core/SearchFilter.h

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

jankratochvil added inline comments. Comment at: lldb/source/Core/SearchFilter.cpp:829 + m_target_sp->GetInlineStrategy() == eInlineBreakpointsHeaders) +return flags | eSymbolContextCompUnit; + return flags; `filter_by_function` now fully overrides

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

kwk marked 2 inline comments as done. kwk added inline comments. Comment at: lldb/source/Breakpoint/BreakpointResolverName.cpp:323 + // passing. + remove_it = false; + } jankratochvil wrote: > Now `if (filter_by_function) {}` always

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

jankratochvil added inline comments. Comment at: lldb/source/Breakpoint/BreakpointResolverName.cpp:323 + // passing. + remove_it = false; + } Now `if (filter_by_function) {}` always overrides any result from `if (filter_by_cu)`. So it

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

kwk updated this revision to Diff 273307. kwk added a comment. - bring back logic to keep a symbol context when a function passes and add a comment as Jan suggested - remove test from scripted resolver that calls SearchFilterByModulesAndSupportFiles::AddressPasses - before the test checked

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

kwk updated this revision to Diff 273309. kwk added a comment. - Add newlines Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74136/new/ https://reviews.llvm.org/D74136 Files: lldb/include/lldb/Core/SearchFilter.h

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

kwk marked an inline comment as done. kwk added inline comments. Comment at: lldb/test/API/functionalities/breakpoint/scripted_bkpt/TestScriptedResolver.py:124 -file_list.Append(lldb.SBFileSpec("noFileOfThisName.xxx")) -

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

jankratochvil added inline comments. Comment at: lldb/source/Core/SearchFilter.cpp:722 + return false; + } + jankratochvil wrote: > This `IsSourceImplementationFile()` should be checking the filename requested > by user, not filename found in DWARF. It

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

jankratochvil added a comment. Just some notes of mine to find out which functions get used in which breakpoint case: -o 'breakpoint set -f main.c -l 1' -o q -o 'b main.c:1' lldb/source/Commands/CommandObjectBreakpoint.cpp:578 = eSetTypeFileAndLine lldb/source/Target/Target.cpp:330

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

kwk updated this revision to Diff 271631. kwk added a comment. - Remove old logic that was no longer needed since my search filter now adaptively adds eSymbolContextCompUnit and not always returns it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

kwk marked 6 inline comments as done. kwk added inline comments. Comment at: lldb/source/Core/SearchFilter.cpp:713 + if (!type) +return SearchFilterByModuleList::FunctionPasses(function); + jankratochvil wrote: > If we cannot determine which file the

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

kwk added a comment. @labath @jingham @jankratochvil The test change suggested by @labath is now in place and it works. @jankratochvil, I've removed the logic that checks seomthing with the CU from `AddressPasses`. That logic now lives in `FunctionPasses` where it logically makes more sense to

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

kwk updated this revision to Diff 271583. kwk added a comment. - Align tests with reviewer expectations Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74136/new/ https://reviews.llvm.org/D74136 Files: lldb/include/lldb/Core/SearchFilter.h

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

jankratochvil marked an inline comment as done. jankratochvil added inline comments. Comment at: lldb/source/Core/SearchFilter.cpp:732 +FileSpec cu_spec; +if (sym_ctx.comp_unit) { + cu_spec = sym_ctx.comp_unit->GetPrimaryFile(); kwk wrote: >

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

kwk marked an inline comment as done. kwk added inline comments. Comment at: lldb/source/Core/SearchFilter.cpp:732 +FileSpec cu_spec; +if (sym_ctx.comp_unit) { + cu_spec = sym_ctx.comp_unit->GetPrimaryFile(); jankratochvil wrote: > This condition is

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

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

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

jingham added a comment. In D74136#2087863 , @jingham wrote: > I need to read this in more detail at this point but I'm caught up in > something else. > > One of the not so great design decisions I made was to try to have "break > set" be a single

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

jingham added a comment. I need to read this in more detail at this point but I'm caught up in something else. One of the not so great design decisions I made was to try to have "break set" be a single command governed by flags. The way the command turned out, there are some flags that tell

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

labath marked an inline comment as done. labath added a comment. In D74136#2085076 , @kwk wrote: > IMPORTANT: The behavior of `target.inline-breakpoint-strategy` when set to > `headers` is still subject to change! > > I think the setting is not respected

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

kwk planned changes to this revision. kwk added a comment. IMPORTANT: The behavior of `target.inline-breakpoint-strategy` when set to `headers` is still subject to change! I think the setting is not respected correctly... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

kwk added a comment. @labath I've applied all the ideas we ping-ponged yesterday and I decided to go with alternating the `target.inline-breakpoint-strategy` from `always` (the default) to `headers`. This way you can exactly see in the test file how things are behaving. So before going into

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

kwk updated this revision to Diff 269787. kwk marked an inline comment as done. kwk added a comment. - remove debug output from test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74136/new/ https://reviews.llvm.org/D74136 Files:

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

kwk updated this revision to Diff 269784. kwk added a comment. - Fix comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74136/new/ https://reviews.llvm.org/D74136 Files: lldb/include/lldb/Core/SearchFilter.h

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

kwk updated this revision to Diff 269782. kwk added a comment. - remove commented out code Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74136/new/ https://reviews.llvm.org/D74136 Files: lldb/include/lldb/Core/SearchFilter.h

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

kwk updated this revision to Diff 269781. kwk marked an inline comment as done. kwk added a comment. - Outsource parts into SearchFilterByModulesAndSupportFiles::FunctionPasses - Tests with alternating setting target.inline-breakpoint-strategy between "always" and "headers" - Respecting

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

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

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

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

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

kwk marked an inline comment as done. kwk added a comment. @labath please see my inline comment. 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

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

kwk updated this revision to Diff 266493. kwk marked an inline comment as done. kwk added a comment. - don't hard-code --color and --dump-input on FileCheck invocation Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74136/new/

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

kwk updated this revision to Diff 266472. kwk added a comment. - make test CHECKs less strict Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74136/new/ https://reviews.llvm.org/D74136 Files: lldb/source/Breakpoint/BreakpointResolverName.cpp

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

kwk marked 2 inline comments as done. kwk added inline comments. Comment at: lldb/test/Shell/Breakpoint/Inputs/search-support-files.h:1 +int inlined_42() { return 42; } labath wrote: > Calling this `inlined` is misleading. The function won't get inlined anywhere

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

kwk updated this revision to Diff 266465. kwk marked 3 inline comments as done. kwk added a comment. - use %t in files created in tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74136/new/ https://reviews.llvm.org/D74136 Files:

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

kwk updated this revision to Diff 266463. kwk marked 3 inline comments as done. kwk added a comment. Herald added a subscriber: sstefan1. - rebase - Rename function in test from inlined_42 to function_in_header - Typo: compulation -> compilation Repository: rG LLVM Github Monorepo CHANGES

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

labath added a comment. In 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

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

kwk updated this revision to Diff 258004. kwk added a comment. - Remove not needed include Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74136/new/ https://reviews.llvm.org/D74136 Files: lldb/source/Breakpoint/BreakpointResolverName.cpp

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

kwk updated this revision to Diff 257691. kwk added a comment. - Revert "Honor the module" Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74136/new/ https://reviews.llvm.org/D74136 Files: lldb/source/Breakpoint/BreakpointResolverName.cpp

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

kwk marked an inline comment as done. kwk added a comment. In D74136#1889066 , @labath wrote: > Yes, I believe this matches the behavior we were talking about. > > I could make a bunch of comments on the implementation and the test, but I'm > not sure if

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

kwk updated this revision to Diff 257688. kwk added a comment. - Honor the module Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74136/new/ https://reviews.llvm.org/D74136 Files: lldb/source/Breakpoint/BreakpointResolverName.cpp

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

kwk updated this revision to Diff 257682. kwk added a comment. - Modify SearchFilterByModuleListAndCU - format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74136/new/ https://reviews.llvm.org/D74136 Files:

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

kwk updated this revision to Diff 257313. kwk added a comment. Ran git clang-format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74136/new/ https://reviews.llvm.org/D74136 Files: lldb/source/Breakpoint/BreakpointResolverName.cpp

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

kwk updated this revision to Diff 255648. kwk added a comment. - Simplified test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74136/new/ https://reviews.llvm.org/D74136 Files: lldb/source/Breakpoint/BreakpointResolverName.cpp

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

kwk updated this revision to Diff 255339. kwk added a comment. - rebased onto master to get rid of NFC change Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74136/new/ https://reviews.llvm.org/D74136 Files:

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

labath added a comment. Yes, I believe this matches the behavior we were talking about. I could make a bunch of comments on the implementation and the test, but I'm not sure if we're at that stage yet... Comment at: lldb/source/Core/SearchFilter.cpp:712 + // the list of

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

kwk added a comment. I figured it might be the easiest to re-use `SearchFilterByModuleListAndCU` but it needs to be renamed still. If you have a good suggestion, please let me know. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74136/new/

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

kwk added a comment. @labath @jingham Can you please have a look at the `lldb/test/Shell/Breakpoint/search-support-files.test` to see if the test reflects the desired behavior? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74136/new/