[Lldb-commits] [PATCH] D118814: [lldb] Don't keep demangled names in memory after indexing

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D118814#3297198 <https://reviews.llvm.org/D118814#3297198>, @JDevlieghere wrote:

> In D118814#3297075 <https://reviews.llvm.org/D118814#3297075>, @jingham wrote:
>
>> In D118814#3296008 <https://reviews.llvm.org/D118814#3296008>, @labath wrote:
>>
>>> This seems fine, though it's not clear to me what is the effect of this patch in terms of functionality. Does the "side-effect" mentioned by Jim still apply here, or is this NFC now? Either is probably fine, but I'd like to understand what is going on. It seems like it should be NFC, but does that mean that the demangling (and the cpu/memory cost) is delayed until the first operation which requests it (such as matching a breakpoint by the full demangled name) ?
>>
>> I haven't gone back to read our lookups in detail, but I certainly hope that the first time we see a breakpoint on a symbol name we don't recognize, we wouldn't go demangling every symbol name in the system.  We really try to keep mistypings from cascading into "unpack the entire world" events.
>
> Yes, this does break the ability to set breakpoints on full demangled names. Based on the code and the comments, it really looks like it was always the intention to avoid demangling the whole name, but then (accidentally?) made it work by storing it in the ConstString. The continue on line 333 is what prevents us from indexing the full name. Before this patch,  `GetDemangledName` would return the cached full demanged name, which now isn't cached and would have to be computed on demand (effectively defeating the purpose of this patch and making things slower).

We really should come up with a good story for symbol lookup on function names that include arguments, but "match the exact output of the demangler" was never a good story.  And I don't think we need pre-demangled names to do it right, rather we should pull the method name out of the user specification, find the matches to that - which goes quickly 'cause it uses the name chopper indices - then winnow down the matches.  Since this is just for function name matches, we could even do smart stuff like (this is spitballing, not a proposed design):

(lldb) break set -n foo::bar::bar(int, *)

meaning, the first parameter has to be an int, I don't care about the others, etc.


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

https://reviews.llvm.org/D118814