[Lldb-commits] [PATCH] D129682: [lldb] Filter DIEs based on qualified name when possible

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D129682#3651175 <https://reviews.llvm.org/D129682#3651175>, @labath wrote:

> Well.. first of let me say that the performance gain is impressive. I wouldn't have expected to gain that much with a relatively simple change.

Thank you! I am also very surprised, it turned out to be a worthwhile time investment.

> - Do we really need the `GetQualifiedNameWithParams` function? My impression was that we've been moving towards ignoring function arguments for name matching, and `CPlusPlusLanguage::DemangledNameContainsPath` does not look like it is making use of that. Note that I don't think that switching to `GetQualifiedName` is the right approach either. I think we should use the mangled name (`GetMangledName`) first, and only fall back to `GetQualifiedName` if that is unavailable (you can look up how it works during SymbolContext construction, but I believe it is roughly like that. (Incidentally, that will bring in the function arguments through the back door for some/most functions, but that isn't my motivation.)

My reasoning for writing `GetQualifiedNameWithParams` instead of just using `GetQualifiedName` is because of matching overloaded names. It's valid for a user to set a breakpoint like `b a_function(int)` to disambiguate any other functions named `a_function`. Without the parameter list, we would be setting a breakpoint on every `a_function` which is not correct if you specify the argument list. I originally used `GetQualifiedName` but TestBreakOnOverload failed...
That being said, I'm not sure how we would use a mangled name to perform the match. We're specifically matching on user input for breakpoints, so if a user inputs something like `StringView::begin`, how would we match that against mangled names? We would need some way to partially mangle `StringView::begin` and match that against the mangled name from the DIE, right? It would probably be faster to match against a mangled name though, so if there is a way to make that work that would be perfect.

> - This patch is duplicating some of the logic inside `Module::LookupInfo::Prune`. I get that we can't reuse it as-is (because we'd need a `SymbolContext` object), but it seems like that function only cares about the `GetFunctionName` portion of the object, so it seems like it should be possible to refactor it such that one can reuse it from within this new context (just pass in a name instead of a full SymbolContext). (I guess this part could be a separate patch)

Specifically you're suggesting a refactor of `Module::LookupInfo::Prune` so that we can deduplicate the logic right? I'd actually like to see if we can get rid of `Module::LookupInfo::Prune` altogether so we don't have to post-process search results but instead filter them as we find them. I hope I've accomplished at least some of that with this change, but getting rid of it entirely would require a few more changes in `Symtab` and in the other SymbolFile implementations (I think...).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129682