LGTM but a test would indeed be nice.

In D83580#2144871, @serge-sans-paille wrote:

Possible followup: shouldn't all the other Get* be flagged as const too?

• Simplify logic

• Add newlines

• 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 that a non existing file doesn't cause setting a breakpoint location
  • the logic in AddressPasses before was to identify the CU of a symbol context and check if it's file is in the list of files that are allowed to pass. We agreed to change this logic so that not only CU's are checked but also files in which a function is declared. SearchFilterByModulesAndSupportFiles::FunctionPasses now does exactly that but it is not called from the BreakPointResolverScripted class, only from BreakpointResolverName.
  • It is an open question how to deal with this and I hope Jim can help here.
  • Shall we maybe take another file from the SymbolContext to see if we can filter by that in AddressPasses? Here's a dump of the filter context for the removed test:

• Remove old logic that was no longer needed since my search filter now adaptively adds eSymbolContextCompUnit and not always returns it.

@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 me. The whole filtering by CU logic from AddressPasses is gone now and I wonder if @jingham has some thoughts on this.

• Align tests with reviewer expectations

@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 the actual code review I'd like to ask you and @jingham to take a look at the test file. Is it the behavior described there the desired outcome? Then we can discuss the implementation.

• remove debug output from test

• Fix comment

• remove commented out code

• Outsource parts into SearchFilterByModulesAndSupportFiles::FunctionPasses
• Tests with alternating setting target.inline-breakpoint-strategy between "always" and "headers"
• Respecting target.inline-breakpoint-strategy setting in SearchFilterByModulesAndSupportFiles and maded adjustments in BreakpointResolverName::SearchCallback
• Renamed SearchFilterByModuleListAndCUList to SearchFilterByModulesAndSupportFiles

In D80531#2069383, @njames93 wrote:

LGTM with 2 small nits, but I'd still give a few days see if anyone else wants to weight in.

• Return macro name by reference
• Add colon to paragraph showing code

Done.

Rebased

Marked more comments as "done" after going through the code again.

@njames93 I've addressed all your comments and hope the patch is good to land now :)

• Don't store SourceManager but get it from Preprocessor
• Use getName instead of getNameStart
• Remove FIXME
• change warning message
• doxygen: \a -> \p
• Make MacroName private and add getter

@labath please see my inline comment.

@njames93 and @Eugene.Zelenko do you guys think this patch is ready to be approved and land?

• don't hard-code --color and --dump-input on FileCheck invocation

• make test CHECKs less strict

• use %t in files created in tests

• rebase
• Rename function in test from inlined_42 to function_in_header
• Typo: compulation -> compilation

@Eugene.Zelenko thank you for your patience with me. I fixed the link.

• fix link in doc

• Adjust link to documentation

• Refer to modernize-use-equals-delete for warning about deleted functions in private sections

Thanks @njames93 and @Eugene.Zelenko for your review. Most of it, I addressed but for some I have comments. See inline.

• Avoid constructing string
• CamelCase for vars
• more readable snippets for documentation
• Added empty line after header in documentation
• Remove sentence from doc
• Remove unimportant tests

@njames93 I've implemented everything you've mentioned. This simplified a lot.

• Wrap RUN lines
• Use CamelCase variable name
• Simplify access to options
• Make check options const
• Use two-line placeholder
• Automate placement of semicolon at the end

[lldb] Manual remove of DISALLOW_COPY_AND_ASSIGN def and one expansion

Sorry, wrong revisions uploaded

Thank you @njames93 for the review. I'm implementing what you wrote.

• Move comment about FinalizeWithSemicolon into code

@Eugene.Zelenko thank you for the review. I've fixed all places that you've mentioned. And have a question about one thing in particular. See inline.

• Make ReplaceDisallowCopyAndAssignMacroCheck only work in C++11 and above
• Remove unrelated changes from clang-tools-extra/docs/clang-tidy/checks/list.rst
• documentation simplification
• Move comment into code
• use single ticks

I've marked some mistakes that were not addressed yet but are marked as "Done".

The only code that is currently after addDecl is changing whether the special members are
defaulted/trivial. I'm not sure if this actually fixes anything but it's more correct than what we
did before.

In D75750#1988330, @labath wrote:

lldb-dev is indeed a better place for the architectural discussion. However, moving the discussion there does not automatically unblock this patch. "get something in now and improve the architecture later" almost never works out in practice. In fact I would say that adding debuginfod is a good way to cement the status quo.

All tests pass. I first thought that the lldb/test/Shell/Reproducer/TestGDBRemoteRepro.test test didn't work but it seems to be flaky.

I resign because I think @labath made some good points that I cannot argue about.

In D75750#1971446, @labath wrote:

In D75750#1967019, @fche2 wrote:

So it might be good to have the SymbolVendors use one or more SymbolServer plug-ins.

I don't believe we have anything that would require all modules in a given target (or whatever) to use the same symbol vendor type. [...]

Just for clarity, is someone proposing to undertake such a rework of that infrastructure? It sounds like this is becoming a prerequisite for Konrad's patch, but if no one's actually doing it, that means Konrad's work is on hold indefinitely. Is that the intent?

Yes, I believe that is becoming a prerequisite. I believe Konrad is willing to try to implement that, but I have advised him to hold on a bit until the exact details are hashed out.

This looks good overall. I would add a section describing which test suite to use when you're interested in a particular DWARF feature for example. I heard from my GDB colleagues that the don't use a compiler, because that might change and produce a different DWARF. Instead they use a DWARF assembler (IIRC). I guess the analogy to LLDB would be yaml2obj which I see not mentioned here. Does it make sense to at least reference it in a section?

• Remove not needed include

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 we're at that stage yet...

• Revert "Honor the module"

• Honor the module

• Modify SearchFilterByModuleListAndCU
• format

Ran git clang-format

Sorry, wrong arc diff invocation.

• Simplified test

• rebased onto master to get rid of NFC change

@jankratochvil thanks for the tip.
@labath, addressed your comments and will now push it.

• Simplify comments