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

Fri Jul 15 01:08:20 PDT 2022

labath added a comment.

In D129682#3653049 <https://reviews.llvm.org/D129682#3653049>, @bulbazord wrote:

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

I'm sorry, that wasn't fully clear. What I meant to say was to take the *mangled* name, then *demangle* it, and perform the matching that way. This would ensure that we are matching against the name that we are eventually going to use. This is particularly important if we're going to be matching function arguments (where, it seems, I have misunderstood what we are doing). Matching full function signatures is hard enough, so we should at least be consistent so that if the user copies a name from some lldb output into a breakpoint command, then it will match. Right now, `GetQualifiedNameWithParams` ignores template arguments so it will produce something like `llvm::find(std::vector<llvm::PassRegistrationListener*, std::allocator<llvm::PassRegistrationListener*>>&, llvm::PassRegistrationListener* const&)` instead of `auto llvm::find<std::vector<llvm::PassRegistrationListener*, std::allocator<llvm::PassRegistrationListener*>>&, llvm::PassRegistrationListener*>(std::vector<llvm::PassRegistrationListener*, std::allocator<llvm::PassRegistrationListener*>>&, llvm::PassRegistrationListener* const&)` -- although, ironically, the user might actually prefer matching the first one.

In order to avoid mismatches like this, I think we should merge this name-producing code with the code <https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp#L2305> which computes the "real" function names.

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

Well.. my main point was to centralize the code doing the filtering. Whether that code lives in `Module::LookupInfo::Prune` or somewhere else is not particularly important, but I would like to avoid having multiple copies of the logic, as that is bound to create inconsistencies. Obvously the way we *derive* the name will be different for DWARF vs. PDB vs. symtab, but I'd hope that the actual matching part can stay the same.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129682