[Lldb-commits] [PATCH] D49990: Use rich mangling information in Symtab::InitNameIndexes()

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz created this revision.
sgraenitz added reviewers: labath, jingham, JDevlieghere, erik.pilkington.

This review is about getting your feedback for the patch. If it doesn't make it in this form, I can adjust everything that's necessary and open a new review once I am done. So please don't hesitate to share your honest opinions.

In preparation for this review, there were two earlier ones:

- https://reviews.llvm.org/D49612 introduced the ItaniumPartialDemangler to LLDB demangling without conceptual changes
- https://reviews.llvm.org/D49909 added a unit test that covers all relevant code paths in the InitNameIndexes() function

Primary goals for this patch are:
(1) Use ItaniumPartialDemangler's rich mangling info for building LLDB's name index.
(2) Make a uniform interface so that Symtab doesn't get involved with mangling details too much.
(3) Improve indexing performance.

In order to achive (1) and (2) I added two classes:

- RichManglingInfo offers a uniform interface to query symbol properties like getFunctionDeclContextName() or isCtorOrDtor(). It can switch between different providers internally. At the moment it supports llvm::ItaniumPartialDemangler and lldb_private::CPlusPlusLanguage::MethodName (legacy/fallback implementation).
- RichManglingSpec handles configuration and lifetime of RichManglingInfos. It is likely stack-allocated and can be reused for multiple queries during batch processing.

These classes are used for wrapping the input and output of DemangleWithRichManglingInfo(), our new function for explicit demangling. It will return a properly initialized RichManglingInfo on success, or otherwise null:
RichManglingInfo *Mangled::DemangleWithRichManglingInfo(RichManglingSpec, SkipMangledNameFn)

Thus the RichManglingInfo class does not need to support a None-state (it's not accessible in this state). In order to avoid an extra heap allocation per invocation for storing the result of DemangleWithRichManglingInfo(), the actual instance is owned by RichManglingSpec. This aids (3) and we want to use a single RichManglingSpec instance for the entire index anyway (it also owns the IPD). An efficient filtering function SkipMangledNameFn contributes here too and helps to mimic the original behavior of InitNameIndexes.

The old implementation only parsed and indexed Itanium mangled names. The new RichManglingInfo can be easily extended for various mangling schemes and languages.

One problem with the implementation of RichManglingInfo is the inaccessibility of class CPlusPlusLanguage::MethodName (defined in source/Plugins/Language/..), from within any header in the Core components of LLDB. The rather hacky solution is to store a type erased pointer and cast it to the correct type on access in the cpp - see RichManglingInfo::get<ParserT>(). Not sure if there's a better way to do it. IMHO CPlusPlusLanguage::MethodName should be a top-level class in order to enable forward delcarations (but that is a rather big change I guess).

I also found a few minor bugs/smells, which I will mark with inline comments. First simple profiling shows a good speedup. target create clang now takes 0.64s on average (over 5 runs). Before the change I observed runtimes between 0.76s an 1.01s. This is still no bulletproof data (I only ran it on one machine!), but it's a promising indicator I think.

What do you think?
Is this too unconventional?
Do you have ideas for improvements?


https://reviews.llvm.org/D49990

Files:
  include/lldb/Core/Mangled.h
  include/lldb/Symbol/Symtab.h
  include/lldb/Utility/ConstString.h
  source/Core/Mangled.cpp
  source/Symbol/Symtab.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D49990.157967.patch
Type: text/x-patch
Size: 27875 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20180730/2c9e38ef/attachment-0001.bin>