Use rich mangling information in Symtab::InitNameIndexes()

[sgraenitz]

Summary

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 RichManglingInfo 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?

[sgraenitz]

include/lldb/Symbol/Symtab.h

43

We don't need a None-case here.

58

This is the hackiest point I guess.

106

^^^^^ May have its own header & cpp

include/lldb/Utility/ConstString.h

357

Fixing a related issue: There is no way to determine whether or not the internal string is null or empty. In fact, operator! does the same as the above IsEmpty(). The Mangled::GetDemangledName() function, however, thinks there would be a difference and wants to benefit from it. The fixed version should be correct now.

source/Core/Mangled.cpp

198

Fixing bug: This is no dead code, but well, maybe in a rare branch.

366

Using the difference between null and empty.

397

Using the difference between null and empty.

source/Symbol/Symtab.cpp

295

This uses a raw C-string instead of llvm::StringRef in order to achieve O(1) runtime.

[labath]

I haven't read through this in detail yet, but I think this is a good start!

The part I'm not sure about is whether the RichManglingInfo vs. RichManglingSpec distinction brings any value. I mean, the lifetime of the first is tied to the lifetime of the second, and the Spec class can only have one active Info instance at any given moment. So you might as well just have one class, pass that to DemangleWithRichManglingInfo, and then query the same object when the call returns. The current interface with createItaniumInfo et al. makes it seem like one could call it multiple times in sequence, stashing the results, and then doing some post-processing on them.

I'll have to think about the C++::MethodName issue a bit more, but in general, I don't think moving that class to a separate file is a too disruptive change. If it means we don't have to mess with untyped pointers, then we should just do it. (Ideally, I wouldn't want the common code to reference that plugin at all, but that ship has already sailed, so I don't think this patch should be predicated on fixing that.)

include/lldb/Symbol/Symtab.h

81–83

This is implied by the deleted copy operations.

source/Symbol/Symtab.cpp

271–289

Could these return StringRef instead of C strings?

295

If you changed the caller to use StringRef too (it seems possible at a first glance) then this would still be O(1)

[zturner]

include/lldb/Symbol/Symtab.h

58

We have llvm::Any. Perhaps you want to use that here instead of void*?

[sgraenitz]

source/Symbol/Symtab.cpp

274

@erik.pilkington Is it acceptable/good practice to pass (nullptr, 0) here? At the moment this safes some lines of initialization checks for m_IPD_buf and m_IPD_size.

[clayborg]

include/lldb/Core/Mangled.h

25–34

move any forward decls to lldb-forward.h and remove all manual forward declarations in lldb_private from here.

include/lldb/Symbol/Symtab.h

26

move to separate files RichManglingInfo.h and RichManglingInfo.cpp

93

move to separate files RichManglingInfo.h and RichManglingInfo.cpp

[sgraenitz]

Thanks or the quick reviews! Follow-ups inline.

include/lldb/Symbol/Symtab.h

58

Thanks. I will check that.

81–83

Which are implicitly deleted too, due to the existence of the destructor right? Does LLVM/LLDB have some kind of convention for it? I like to be explicit on ctors&assignment ("rule of 5"), because it aids error messages, but I would be fine with following the existing convention here.

source/Symbol/Symtab.cpp

271–289

Yes. So far it's simply the closest superset of the two interfaces, but I will try using StringRef where possible.

295

Right, thanks there's a ConstString::GetStringRef(). Perfect.

[erik.pilkington]

source/Symbol/Symtab.cpp

274

Sure, thats fine! Those parameters act the same way as buf and size in __cxa_demangle.

getFunctionBaseName will return nullptr if the mangled name isn't a function. Is it a precondition of this function that m_IPD stores a function? If not, it looks like you'll leak the buffer.

[sgraenitz]

include/lldb/Symbol/Symtab.h

58

@zturner Where is llvm::Any? Expected it in ADT or Support, but can't find it. IIUC llvm::Optional does something similar, but uses its own optional_detail::OptionalStorage. Same for llvm::Expected. Or is it a very recent addition?

[zturner]

include/lldb/Symbol/Symtab.h

58

It's pretty recent. I was actually the one who added it, about maybe 2 weeks ago. It's in include/llvm/ADT/Any.h

[sgraenitz]

source/Symbol/Symtab.cpp

274

Oh that is a very good note. I had it as a precondition in the client function in Symtab. When I removed that and started to just check the result for nullptr, I didn't think about the buffer. Gonna fix it, the generalized interface shouldn't have that precondition anyway. Thanks!

[sgraenitz]

Simple fixes

[sgraenitz]

Moved forward decls

[sgraenitz]

Sorry, I accidentally added the tests from https://reviews.llvm.org/D49909 also to this review. I will clean this up tomorrow.

[sgraenitz]

Remove test code, that I added accidentally. Move RichManglingInfo and RichManglingSpec to their own header and cpp.

[sgraenitz]

The part I'm not sure about is whether the RichManglingInfo vs. RichManglingSpec distinction brings any value. [...] So you might as well just have one class, pass that to DemangleWithRichManglingInfo, and then query the same object when the call returns.

The idea here was that DemangleWithRichManglingInfo() acts like a gate keeper. If it succeeds it provides read-only access to the updated RichManglingInfo in RichManglingSpec, otherwise it returns null. IMHO the value behind it is that RichManglingInfo does not need to handle a NoInfo case next to ItaniumPartialDemangler and PluginCxxLanguage in every single getter. Instead it's just not accessible in that state. (Plus: there is no maintenance functions that confuse the public interface of RichManglingInfo.) I don't know how to do this with only one class.

Maybe RichManglingSpec is not the perfect name. What about renaming it to RichManglingContext?

I mean, the lifetime of the first is tied to the lifetime of the second, and the Spec class can only have one active Info instance at any given moment.

Yes, this was handy and avoids extra heap-allocations. const RichManglingInfo * was intended to clarify: lifetimes are handled elsewhere.

The current interface with createItaniumInfo et al. makes it seem like one could call it multiple times in sequence, stashing the results, and then doing some post-processing on them.

Yes, I can see that this is implicit knowledge. Do you have an idea how to make this more explicit? Rename to SetItaniumInfo() maybe?

In the end, I definitely prefer this approach over having a NoInfo state in RichManglingInfo. What do you think?

[labath]

I think there is still something wrong with the diff. I can't see any of the callers of e.g. createItaniumInfo but I can see the function on both LHS and RHS of the diff (which shouldn't be the case as it's a new function). It looks like you uploaded just an ammending patch instead of the entire work. Can you fix that?

In D49990#1182003, @sgraenitz wrote:

The part I'm not sure about is whether the RichManglingInfo vs. RichManglingSpec distinction brings any value. [...] So you might as well just have one class, pass that to DemangleWithRichManglingInfo, and then query the same object when the call returns.

The idea here was that DemangleWithRichManglingInfo() acts like a gate keeper. If it succeeds it provides read-only access to the updated RichManglingInfo in RichManglingSpec, otherwise it returns null. IMHO the value behind it is that RichManglingInfo does not need to handle a NoInfo case next to ItaniumPartialDemangler and PluginCxxLanguage in every single getter. Instead it's just not accessible in that state. (Plus: there is no maintenance functions that confuse the public interface of RichManglingInfo.) I don't know how to do this with only one class.

Maybe RichManglingSpec is not the perfect name. What about renaming it to RichManglingContext?

I mean, the lifetime of the first is tied to the lifetime of the second, and the Spec class can only have one active Info instance at any given moment.

Yes, this was handy and avoids extra heap-allocations. const RichManglingInfo * was intended to clarify: lifetimes are handled elsewhere.

The current interface with createItaniumInfo et al. makes it seem like one could call it multiple times in sequence, stashing the results, and then doing some post-processing on them.

Yes, I can see that this is implicit knowledge. Do you have an idea how to make this more explicit? Rename to SetItaniumInfo() maybe?

In the end, I definitely prefer this approach over having a NoInfo state in RichManglingInfo. What do you think?

Yes, I can see what you mean here. Neither of the solutions is particularly appealing. I guess if I were implementing this, I'd go with the "invalid state" option, though I am not sure why, as usually I am opposed to invalid states. Maybe we can leave this to the discretion of the implementor (you).

include/lldb/Symbol/Symtab.h

81–83

As far as I know, the presence of a destructor has no impact on the state of copy/move operations, so you still need to delete the copy operations explicitly.

I don't know if there is an official policy on explicitly deleting move operations, but I don't remember seeing that style anywhere. However, I don't care much about that either.

[sgraenitz]

include/lldb/Symbol/Symtab.h

81–83

The generation of the implicitly-defined copy constructor is deprecated if T has a user-defined destructor or user-defined copy assignment operator.

https://en.cppreference.com/w/cpp/language/copy_constructor

Actually, copy has no implications here and move won't work on the const pointer. Thus I will just remove it :)

source/Core/Mangled.cpp

356–357

I think there is still something wrong with the diff. I can't see any of the callers of e.g. createItaniumInfo

Weird. The caller is here, but not shown as a change anymore..

[sgraenitz]

Fix potential leak of m_IPD_buf. Use llvm::Any instead of void*.
Rename: RichManglingSpec -> RichManglingContext, RichManglingContext::CreateXyInfo() -> RichManglingContext::SetXyInfo()

[sgraenitz]

I think there is still something wrong with the diff. I can't see any of the callers of e.g. createItaniumInfo

Weird. The caller is here, but not shown as a change anymore..

I created a new review where all my changes are marked in green and red: https://reviews.llvm.org/D50071
If you have any more feedback, please let me know. I will keep the new one open for a few days, so Jim can review it when he is back from vacation.

include/lldb/Core/Mangled.h

 //===-- Mangled.h -----------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef liblldb_Mangled_h_
 #define liblldb_Mangled_h_
 #if defined(__cplusplus)
 
 #include "lldb/Utility/ConstString.h"
-#include "lldb/lldb-enumerations.h" // for LanguageType
-#include "llvm/ADT/StringRef.h"     // for StringRef
+#include "lldb/lldb-enumerations.h"
+#include "llvm/ADT/StringRef.h"
 
-#include <stddef.h> // for size_t
+#include <memory>
+#include <stddef.h>
 
+namespace llvm {
+struct ItaniumPartialDemangler;
+}
+
+namespace lldb_private {
+class RichManglingInfo;
+class RichManglingSpec;
+}
 namespace lldb_private {
 class RegularExpression;
 }
 namespace lldb_private {
 class Stream;
 }
 
 namespace lldb_private {
 
 //----------------------------------------------------------------------
 /// @class Mangled Mangled.h "lldb/Core/Mangled.h"
 /// A class that handles mangled names.
 ///
 /// Designed to handle mangled names. The demangled version of any names will
public:
   /// @return
   ///     \b True if \a name matches either name, \b false otherwise.
   //----------------------------------------------------------------------
   bool NameMatches(const ConstString &name, lldb::LanguageType language) const {
     if (m_mangled == name)
       return true;
     return GetDemangledName(language) == name;
   }
-
   bool NameMatches(const RegularExpression &regex,
                    lldb::LanguageType language) const;
 
   //----------------------------------------------------------------------
   /// Get the memory cost of this object.
   ///
   /// Return the size in bytes that this object takes in memory. This returns
   /// the size in bytes of this object, not any shared string values it may
public:
   /// (there will be no mangled name).
   ///
   /// @return
   ///     The language for the mangled/demangled name, eLanguageTypeUnknown
   ///     if there is no mangled or demangled counterpart.
   //----------------------------------------------------------------------
   lldb::LanguageType GuessLanguage() const;
 
+  /// Function signature for filtering mangled names.
+  using SkipMangledNameFn = bool(const char *, ManglingScheme);
+
+  //----------------------------------------------------------------------
+  /// Trigger explicit demangling to obtain rich mangling information. This is
+  /// optimized for batch processing while populating a name index. To get the
+  /// pure demangled name string for a single entity, use GetDemangledName()
+  /// instead.
+  ///
+  /// For names that match the Itanium mangling scheme, this uses LLVM's
+  /// ItaniumPartialDemangler. All other names fall back to LLDB's builtin
+  /// parser currently.
+  ///
+  /// @param[in] spec
+  ///     The RichManglingSpec that provides the context for this function. One
+  ///     instance can be used for multiple calls. Should be stack-allocated in
+  ///     the caller's frame.
+  ///
+  /// @param[in] skip_mangled_name
+  ///     A filtering function for skipping entities based on name and mangling
+  ///     scheme. This can be null if unused.
+  ///
+  /// @return
+  ///     The rich mangling info on success, null otherwise.
+  //----------------------------------------------------------------------
+  const RichManglingInfo *
+  DemangleWithRichManglingInfo(RichManglingSpec &spec,
+                               SkipMangledNameFn *skip_mangled_name);
+
 private:
   //----------------------------------------------------------------------
   /// Mangled member variables.
   //----------------------------------------------------------------------
   ConstString m_mangled;           ///< The mangled version of the name
   mutable ConstString m_demangled; ///< Mutable so we can get it on demand with
                                    ///a const version of this object
 };
 
 Stream &operator<<(Stream &s, const Mangled &obj);
 
 } // namespace lldb_private
 
 #endif // #if defined(__cplusplus)
 #endif // liblldb_Mangled_h_

include/lldb/Symbol/Symtab.h

 
 #include <mutex>
 #include <vector>
 
 #include "lldb/Core/RangeMap.h"
 #include "lldb/Core/UniqueCStringMap.h"
 #include "lldb/Symbol/Symbol.h"
 #include "lldb/lldb-private.h"
+#include "llvm/Demangle/Demangle.h"
 
 namespace lldb_private {
 
+/// Uniform wrapper for access to rich mangling information from different
+/// providers. See Mangled::DemangleWithRichManglingInfo()
+class RichManglingInfo {
+public:
+  /// If this symbol describes a constructor or destructor.
+  bool isCtorOrDtor() const;
+
+  /// If this symbol describes a function.
+  bool isFunction() const;
+
+  /// Get the base name of a function. This doesn't include trailing template
+  /// arguments, ie for "a::b<int>" this function returns "b".
+  const char *getFunctionBaseName() const;
+
+  /// Get the context name for a function. For "a::b::c", this function returns
+  /// "a::b".
+  const char *getFunctionDeclContextName() const;
+
+private:
+  enum InfoProvider { ItaniumPartialDemangler, PluginCxxLanguage };
+
+  /// Selects the rich mangling info provider. Initially undefined, but
+  /// initialized in RichManglingSpec::CreateX (instance not accessible before).
+  InfoProvider m_provider;
+
+  /// Members for ItaniumPartialDemangler
+  llvm::ItaniumPartialDemangler *m_IPD = nullptr;
+  mutable size_t m_IPD_size = 0;
+  mutable char *m_IPD_buf = nullptr;
+
+  /// Members for PluginCxxLanguage
+  /// Cannot forward declare inner class CPlusPlusLanguage::MethodName. The
+  /// respective header is in Plugins and including it from here causes cyclic
+  /// dependency. Keep a void* here instead and cast it on-demand on the cpp.
+  void *m_legacy_parser = nullptr;
+
+  /// Obtain the legacy parser casted to the given type. Ideally we had a type
+  /// trait to deduce \a ParserT from a given InfoProvider, but unfortunately we
+  /// can't access CPlusPlusLanguage::MethodName from within the header.
+  template <class ParserT> ParserT *get() const {
+    assert(m_legacy_parser);
+    return reinterpret_cast<ParserT *>(m_legacy_parser);
+  }
+
+  /// Reset the provider and clean up memory before reassigning/destroying.
+  void ResetProvider();
+
+  // Default construction in undefined state from RichManglingSpec.
+  RichManglingInfo() = default;
+
+  // Destruction from RichManglingSpec.
+  ~RichManglingInfo();
+
+  // No copy
+  RichManglingInfo(const RichManglingInfo &) = delete;
+  RichManglingInfo &operator=(const RichManglingInfo &) = delete;
+
+  // No move
+  RichManglingInfo(RichManglingInfo &&) = delete;
+  RichManglingInfo &operator=(RichManglingInfo &&) = delete;
+
+  // Declare RichManglingSpec as friend so it can access the default ctor and
+  // assign to members in its CreateX methods.
+  friend class RichManglingSpec;
+};
+
+//----------------------------------------------------------------------
+
+/// Unique owner of RichManglingInfo. Handles initialization and lifetime.
+class RichManglingSpec {
+public:
+  RichManglingInfo *CreateItaniumInfo();
+  RichManglingInfo *CreateLegacyCxxParserInfo(const ConstString &mangled);
+
+  llvm::ItaniumPartialDemangler &GetIPD() { return m_IPD; }
+
+private:
+  RichManglingInfo m_info;
+  llvm::ItaniumPartialDemangler m_IPD;
+};
+
+//----------------------------------------------------------------------
+
 class Symtab {
 public:
   typedef std::vector<uint32_t> IndexCollection;
   typedef UniqueCStringMap<uint32_t> NameToIndexMap;
 
   typedef enum Debug {
     eDebugNo,  // Not a debug symbol
     eDebugYes, // A debug symbol
case eVisibilityPrivate:
       return !m_symbols[idx].IsExternal();
     }
     return false;
   }
 
   void SymbolIndicesToSymbolContextList(std::vector<uint32_t> &symbol_indexes,
                                         SymbolContextList &sc_list);
 
+  void RegisterMangledNameEntry(
+      NameToIndexMap::Entry &entry, std::set<const char *> &class_contexts,
+      UniqueCStringMap<uint32_t> &mangled_name_to_index,
+      std::vector<const char *> &symbol_contexts, const RichManglingInfo &info);
+
   DISALLOW_COPY_AND_ASSIGN(Symtab);
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_Symtab_h_

include/lldb/Utility/ConstString.h

public:
   ///
   /// @return
   ///     @li \b true if the contained string is empty.
   ///     @li \b false if the contained string is not empty.
   //------------------------------------------------------------------
   bool IsEmpty() const { return m_string == nullptr || m_string[0] == '\0'; }
 
   //------------------------------------------------------------------
+  /// Test for null string.
+  ///
+  /// @return
+  ///     @li \b true if there is no string associated with this instance.
+  ///     @li \b false if there is a string associated with this instance.
+  //------------------------------------------------------------------
+  bool IsNull() const { return m_string == nullptr; }
+
+  //------------------------------------------------------------------
   /// Set the C string value.
   ///
   /// Set the string value in the object by uniquing the \a cstr string value
   /// in our global string pool.
   ///
   /// If the C string already exists in the global string pool, it finds the
   /// current entry and returns the existing value. If it doesn't exist, it is
   /// added to the string pool.

source/Core/Mangled.cpp

 }
 
 //----------------------------------------------------------------------
 // Compare the string values.
 //----------------------------------------------------------------------
 int Mangled::Compare(const Mangled &a, const Mangled &b) {
   return ConstString::Compare(
       a.GetName(lldb::eLanguageTypeUnknown, ePreferMangled),
-      a.GetName(lldb::eLanguageTypeUnknown, ePreferMangled));
+      b.GetName(lldb::eLanguageTypeUnknown, ePreferMangled));
 }
 
 //----------------------------------------------------------------------
 // Set the string value in this objects. If "mangled" is true, then the mangled
 // named is set with the new value in "s", else the demangled name is set.
 //----------------------------------------------------------------------
 void Mangled::SetValue(const ConstString &s, bool mangled) {
   if (s) {
if (name) {
     }
   } else {
     m_demangled.Clear();
     m_mangled.Clear();
   }
 }
 
 //----------------------------------------------------------------------
+// Local helpers for different demangling implementations.
+//----------------------------------------------------------------------
+namespace {
+
+char *GetMSVCDemangledCStr(const char *M) {
+#if defined(_MSC_VER)
+  const size_t demangled_length = 2048;
+  char *demangled_cstr = static_cast<char *>(::malloc(demangled_length));
+  ::ZeroMemory(demangled_cstr, demangled_length);
+  DWORD result = safeUndecorateName(M, demangled_cstr, demangled_length);
+
+  if (Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_DEMANGLE)) {
+    if (demangled_cstr && demangled_cstr[0])
+      log->Printf("demangled msvc: %s -> \"%s\"", M, demangled_cstr);
+    else
+      log->Printf("demangled msvc: %s -> error: 0x%lu", M, result);
+  }
+
+  if (result != 0) {
+    return demangled_cstr;
+  } else {
+    ::free(demangled_cstr);
+    return nullptr;
+  }
+#else
+  return nullptr;
+#endif
+}
+
+char *GetItaniumRichDemangleInfo(const char *M,
+                                 llvm::ItaniumPartialDemangler &IPD) {
+  char *demangled_cstr = nullptr;
+  bool err = IPD.partialDemangle(M);
+  if (!err) {
+    // Default buffer and size (will realloc in case it's too small).
+    size_t demangled_size = 80;
+    demangled_cstr = static_cast<char *>(std::malloc(demangled_size));
+    demangled_cstr = IPD.finishDemangle(demangled_cstr, &demangled_size);
+  }
+
+  if (Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_DEMANGLE)) {
+    if (demangled_cstr)
+      log->Printf("demangled itanium: %s -> \"%s\"", M, demangled_cstr);
+    else
+      log->Printf("demangled itanium: %s -> error: failed to demangle", M);
+  }
+
+  return demangled_cstr;
+}
+} // namespace
+
+//----------------------------------------------------------------------
+// Explicit demangling for scheduled requests during batch processing. This
+// makes use of ItaniumPartialDemangler's rich demangle info
+//----------------------------------------------------------------------
+const RichManglingInfo *
+Mangled::DemangleWithRichManglingInfo(RichManglingSpec &spec,
+                                      SkipMangledNameFn *skip_mangled_name) {
+  // We need to generate and cache the demangled name.
+  static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
+  Timer scoped_timer(func_cat,
+                     "Mangled::DemangleWithRichNameIndexInfo (m_mangled = %s)",
+                     m_mangled.GetCString());
+
+  // Others are not meant to arrive here. ObjC names or C's main() for example
+  // have their names stored in m_demangled, while m_mangled is empty.
+  assert(m_mangled);
+
+  // Check whether or not we are interested in this name at all.
+  const char *M = m_mangled.GetCString();
+  ManglingScheme S = cstring_mangling_scheme(M);
+  if (skip_mangled_name && skip_mangled_name(M, S))
+    return nullptr;
+
+  switch (S) {
+  case eManglingSchemeNone:
+    // The current mangled_name_filter would allow llvm_unreachable here.
+    return nullptr;
+
+  case eManglingSchemeItanium:
+    // We want the rich mangling info here, so we don't care whether or not
+    // there is a demangled string in the pool already.
+    if (char *D = GetItaniumRichDemangleInfo(M, spec.GetIPD())) {
+      // Connect the counterparts in the string pool to accelerate subsequent
+      // access in GetDemangledName().
+      m_demangled.SetCStringWithMangledCounterpart(D, m_mangled);
+      std::free(D);
+
+      return spec.CreateItaniumInfo();
+    } else {
+      m_demangled.SetCString("");
+      return nullptr;
+    }
+
+  case eManglingSchemeMSVC: {
+    // We have no rich mangling for MSVC-mangled names yet, so first try to
+    // demangle it if necessary.
+    if (!m_demangled && !m_mangled.GetMangledCounterpart(m_demangled)) {
+      if (char *D = GetMSVCDemangledCStr(M)) {
+        // Connect the counterparts in the string pool to accelerate
+        // subsequent access in GetDemangledName().
+        m_demangled.SetCStringWithMangledCounterpart(D, m_mangled);
+        ::free(D);
+      } else {
+        m_demangled.SetCString("");
+      }
+    }
+
+    if (m_demangled.IsEmpty()) {
+      // Cannot demangle it, so don't try parsing.
+      return nullptr;
+    } else {
+      // Demangled successfully, we can try and parse it with
+      // CPlusPlusLanguage::MethodName.
+      return spec.CreateLegacyCxxParserInfo(m_mangled);
+    }
+  }
+  }
+}
+
+//----------------------------------------------------------------------
 // Generate the demangled name on demand using this accessor. Code in this
 // class will need to use this accessor if it wishes to decode the demangled
 // name. The result is cached and will be kept until a new string value is
 // supplied to this object, or until the end of the object's lifetime.
 //----------------------------------------------------------------------
 const ConstString &
 Mangled::GetDemangledName(lldb::LanguageType language) const {
   // Check to make sure we have a valid mangled name and that we haven't
   // already decoded our mangled name.
-  if (m_mangled && !m_demangled) {
+  if (m_mangled && m_demangled.IsNull()) {
     // We need to generate and cache the demangled name.
     static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
     Timer scoped_timer(func_cat, "Mangled::GetDemangledName (m_mangled = %s)",
                        m_mangled.GetCString());
 
-    Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_DEMANGLE);
-
     // Don't bother running anything that isn't mangled
     const char *mangled_name = m_mangled.GetCString();
     ManglingScheme mangling_scheme{cstring_mangling_scheme(mangled_name)};
     if (mangling_scheme != eManglingSchemeNone &&
         !m_mangled.GetMangledCounterpart(m_demangled)) {
       // We didn't already mangle this name, demangle it and if all goes well
       // add it to our map.
       char *demangled_name = nullptr;
       switch (mangling_scheme) {
-      case eManglingSchemeMSVC: {
-#if defined(_MSC_VER)
-        if (log)
-          log->Printf("demangle msvc: %s", mangled_name);
-        const size_t demangled_length = 2048;
-        demangled_name = static_cast<char *>(::malloc(demangled_length));
-        ::ZeroMemory(demangled_name, demangled_length);
-        DWORD result =
-            safeUndecorateName(mangled_name, demangled_name, demangled_length);
-        if (log) {
-          if (demangled_name && demangled_name[0])
-            log->Printf("demangled msvc: %s -> \"%s\"", mangled_name,
-                        demangled_name);
-          else
-            log->Printf("demangled msvc: %s -> error: 0x%lu", mangled_name,
-                        result);
-        }
-
-        if (result == 0) {
-          free(demangled_name);
-          demangled_name = nullptr;
-        }
-#endif
+      case eManglingSchemeMSVC:
+        demangled_name = GetMSVCDemangledCStr(mangled_name);
         break;
-      }
       case eManglingSchemeItanium: {
         llvm::ItaniumPartialDemangler IPD;
-        bool demangle_err = IPD.partialDemangle(mangled_name);
-        if (!demangle_err) {
-          // Default buffer and size (realloc is used in case it's too small).
-          size_t demangled_size = 80;
-          demangled_name = static_cast<char *>(::malloc(demangled_size));
-          demangled_name = IPD.finishDemangle(demangled_name, &demangled_size);
-        }
-
-        if (log) {
-          if (demangled_name)
-            log->Printf("demangled itanium: %s -> \"%s\"", mangled_name,
-                        demangled_name);
-          else
-            log->Printf("demangled itanium: %s -> error: failed to demangle",
-                        mangled_name);
-        }
+        demangled_name = GetItaniumRichDemangleInfo(mangled_name, IPD);
         break;
       }
       case eManglingSchemeNone:
-        break;
+        llvm_unreachable("eManglingSchemeNone was handled already");
       }
       if (demangled_name) {
         m_demangled.SetCStringWithMangledCounterpart(demangled_name, m_mangled);
         free(demangled_name);
       }
     }
-    if (!m_demangled) {
+    if (m_demangled.IsNull()) {
       // Set the demangled string to the empty string to indicate we tried to
       // parse it once and failed.
       m_demangled.SetCString("");
     }
   }
 
   return m_demangled;
 }

source/Symbol/Symtab.cpp

 #include "lldb/Core/Section.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Symbol/Symbol.h"
 #include "lldb/Symbol/SymbolContext.h"
 #include "lldb/Symbol/Symtab.h"
 #include "lldb/Utility/RegularExpression.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/Timer.h"
+#include "llvm/Demangle/Demangle.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 Symtab::Symtab(ObjectFile *objfile)
     : m_objfile(objfile), m_symbols(), m_file_addr_to_index(),
       m_name_to_index(), m_mutex(), m_file_addr_to_index_computed(false),
       m_name_indexes_computed(false) {}
const Symbol *Symtab::SymbolAtIndex(size_t idx) const {
   // Clients should grab the mutex from this symbol table and lock it manually
   // when calling this function to avoid performance issues.
   if (idx < m_symbols.size())
     return &m_symbols[idx];
   return nullptr;
 }
 
 //----------------------------------------------------------------------
+// RichManglingInfo
+//----------------------------------------------------------------------
+
+void RichManglingInfo::ResetProvider() {
+  // If we want to support parsers for other languages some day, we need a
+  // switch here to delete the correct parser type.
+  if (m_legacy_parser) {
+    assert(m_provider == RichManglingInfo::PluginCxxLanguage);
+    delete get<CPlusPlusLanguage::MethodName>();
+    m_legacy_parser = nullptr;
+  }
+}
+
+RichManglingInfo *RichManglingSpec::CreateItaniumInfo() {
+  m_info.ResetProvider();
+  m_info.m_provider = RichManglingInfo::ItaniumPartialDemangler;
+  m_info.m_IPD = &m_IPD;
+  return &m_info;
+}
+
+RichManglingInfo *
+RichManglingSpec::CreateLegacyCxxParserInfo(const ConstString &mangled) {
+  m_info.ResetProvider();
+  m_info.m_provider = RichManglingInfo::PluginCxxLanguage;
+  m_info.m_legacy_parser = new CPlusPlusLanguage::MethodName(mangled);
+  return &m_info;
+}
+
+RichManglingInfo::~RichManglingInfo() { 
+  ResetProvider();
+  delete m_IPD_buf;
+}
+
+bool RichManglingInfo::isCtorOrDtor() const {
+  switch (m_provider) {
+  case ItaniumPartialDemangler:
+    return m_IPD->isCtorOrDtor();
+  case PluginCxxLanguage: {
+    // We can only check for destructors here.
+    auto base_name = get<CPlusPlusLanguage::MethodName>()->GetBasename();
+    return base_name.front() == '~';
+  }
+  }
+}
+
+bool RichManglingInfo::isFunction() const {
+  switch (m_provider) {
+  case ItaniumPartialDemangler:
+    return m_IPD->isFunction();
+  case PluginCxxLanguage:
+    return get<CPlusPlusLanguage::MethodName>()->IsValid();
+  }
+}
+
+const char *RichManglingInfo::getFunctionBaseName() const {
+  switch (m_provider) {
+  case ItaniumPartialDemangler:
+    m_IPD_buf = m_IPD->getFunctionBaseName(m_IPD_buf, &m_IPD_size);
+    return m_IPD_buf;
+  case PluginCxxLanguage:
+    return get<CPlusPlusLanguage::MethodName>()->GetBasename().data();
+  }
+}
+
+const char *RichManglingInfo::getFunctionDeclContextName() const {
+  switch (m_provider) {
+  case ItaniumPartialDemangler:
+    m_IPD_buf = m_IPD->getFunctionDeclContextName(m_IPD_buf, &m_IPD_size);
+    return m_IPD_buf;
+  case PluginCxxLanguage:
+    return get<CPlusPlusLanguage::MethodName>()->GetContext().data();
+  }
+}
+
+//----------------------------------------------------------------------
 // InitNameIndexes
 //----------------------------------------------------------------------
+namespace {
+bool lldb_skip_name(const char *mangled_cstr, Mangled::ManglingScheme scheme) {
+  switch (scheme) {
+  case Mangled::eManglingSchemeItanium: {
+    // Equivalent to !llvm::StringRef::startswith("_Z")
+    if (mangled_cstr == nullptr || mangled_cstr[0] != '_' ||
+        mangled_cstr[1] != 'Z')
+      return true;
+
+    // Avoid the following types of symbols in the index.
+    switch (mangled_cstr[2]) {
+    case 'G': // guard variables
+    case 'T': // virtual tables, VTT structures, typeinfo structures + names
+    case 'Z': // named local entities (if we eventually handle
+              // eSymbolTypeData, we will want this back)
+      return true;
+
+    default:
+      break;
+    }
+
+    // Include this name in the index.
+    return false;
+  }
+
+  // No filters for this scheme yet. Include all names in indexing.
+  case Mangled::eManglingSchemeMSVC:
+    return false;
+
+  // Don't try and demangle things we can't categorize.
+  case Mangled::eManglingSchemeNone:
+    return true;
+  }
+}
+} // namespace
+
 void Symtab::InitNameIndexes() {
   // Protected function, no need to lock mutex...
   if (!m_name_indexes_computed) {
     m_name_indexes_computed = true;
     static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
     Timer scoped_timer(func_cat, "%s", LLVM_PRETTY_FUNCTION);
     // Create the name index vector to be able to quickly search by name
     const size_t num_symbols = m_symbols.size();
for (const_iterator pos = m_symbols.begin(), end = m_symbols.end();
 
       if (mangled.GetDemangledName())
         ++actual_count;
     }
 
     m_name_to_index.Reserve(actual_count);
 #endif
 
-    NameToIndexMap::Entry entry;
-
     // The "const char *" in "class_contexts" must come from a
     // ConstString::GetCString()
     std::set<const char *> class_contexts;
     UniqueCStringMap<uint32_t> mangled_name_to_index;
     std::vector<const char *> symbol_contexts(num_symbols, nullptr);
 
+    // Instantiation of the demangler is expensive, so better use a single one
+    // for all entries during batch processing.
+    RichManglingSpec spec;
+    NameToIndexMap::Entry entry;
+
     for (entry.value = 0; entry.value < num_symbols; ++entry.value) {
-      const Symbol *symbol = &m_symbols[entry.value];
+      Symbol *symbol = &m_symbols[entry.value];
 
       // Don't let trampolines get into the lookup by name map If we ever need
       // the trampoline symbols to be searchable by name we can remove this and
       // then possibly add a new bool to any of the Symtab functions that
       // lookup symbols by name to indicate if they want trampolines.
       if (symbol->IsTrampoline())
         continue;
 
-      const Mangled &mangled = symbol->GetMangled();
+      // If the symbol's name string matched a Mangled::ManglingScheme, it is
+      // stored in the mangled field.
+      Mangled &mangled = symbol->GetMangled();
       entry.cstring = mangled.GetMangledName();
       if (entry.cstring) {
         m_name_to_index.Append(entry);
 
         if (symbol->ContainsLinkerAnnotations()) {
           // If the symbol has linker annotations, also add the version without
           // the annotations.
           entry.cstring = ConstString(m_objfile->StripLinkerSymbolAnnotations(
                                         entry.cstring.GetStringRef()));
           m_name_to_index.Append(entry);
         }
 
-        const SymbolType symbol_type = symbol->GetType();
-        if (symbol_type == eSymbolTypeCode ||
-            symbol_type == eSymbolTypeResolver) {
-          llvm::StringRef entry_ref(entry.cstring.GetStringRef());
-          if (entry_ref[0] == '_' && entry_ref[1] == 'Z' &&
-              (entry_ref[2] != 'T' && // avoid virtual table, VTT structure,
-                                      // typeinfo structure, and typeinfo
-                                      // name
-               entry_ref[2] != 'G' && // avoid guard variables
-               entry_ref[2] != 'Z'))  // named local entities (if we
-                                          // eventually handle eSymbolTypeData,
-                                          // we will want this back)
-          {
-            CPlusPlusLanguage::MethodName cxx_method(
-                mangled.GetDemangledName(lldb::eLanguageTypeC_plus_plus));
-            entry.cstring = ConstString(cxx_method.GetBasename());
-            if (entry.cstring) {
-              // ConstString objects permanently store the string in the pool
-              // so calling GetCString() on the value gets us a const char *
-              // that will never go away
-              const char *const_context =
-                  ConstString(cxx_method.GetContext()).GetCString();
-
-              if (!const_context || const_context[0] == 0) {
-                // No context for this function so this has to be a basename
-                m_basename_to_index.Append(entry);
-                // If there is no context (no namespaces or class scopes that
-                // come before the function name) then this also could be a
-                // fullname.
-                m_name_to_index.Append(entry);
-              } else {
-                entry_ref = entry.cstring.GetStringRef();
-                if (entry_ref[0] == '~' ||
-                    !cxx_method.GetQualifiers().empty()) {
-                  // The first character of the demangled basename is '~' which
-                  // means we have a class destructor. We can use this
-                  // information to help us know what is a class and what
-                  // isn't.
-                  if (class_contexts.find(const_context) == class_contexts.end())
-                    class_contexts.insert(const_context);
-                  m_method_to_index.Append(entry);
-                } else {
-                  if (class_contexts.find(const_context) !=
-                      class_contexts.end()) {
-                    // The current decl context is in our "class_contexts"
-                    // which means this is a method on a class
-                    m_method_to_index.Append(entry);
-                  } else {
-                    // We don't know if this is a function basename or a
-                    // method, so put it into a temporary collection so once we
-                    // are done we can look in class_contexts to see if each
-                    // entry is a class or just a function and will put any
-                    // remaining items into m_method_to_index or
-                    // m_basename_to_index as needed
-                    mangled_name_to_index.Append(entry);
-                    symbol_contexts[entry.value] = const_context;
-                  }
-                }
-              }
-            }
-          }
+        const SymbolType type = symbol->GetType();
+        if (type == eSymbolTypeCode || type == eSymbolTypeResolver) {
+          if (const RichManglingInfo *info =
+                  mangled.DemangleWithRichManglingInfo(spec, lldb_skip_name))
+            RegisterMangledNameEntry(entry, class_contexts,
+                                     mangled_name_to_index, symbol_contexts,
+                                     *info);
         }
       }
 
+      // Symbol name strings that didn't match a Mangled::ManglingScheme, are
+      // stored in the demangled field.
       entry.cstring = mangled.GetDemangledName(symbol->GetLanguage());
       if (entry.cstring) {
         m_name_to_index.Append(entry);
 
         if (symbol->ContainsLinkerAnnotations()) {
           // If the symbol has linker annotations, also add the version without
           // the annotations.
           entry.cstring = ConstString(m_objfile->StripLinkerSymbolAnnotations(
if (!mangled_name_to_index.IsEmpty()) {
         if (mangled_name_to_index.GetValueAtIndex(i, entry.value)) {
           entry.cstring = mangled_name_to_index.GetCStringAtIndex(i);
           if (symbol_contexts[entry.value] &&
               class_contexts.find(symbol_contexts[entry.value]) !=
                   class_contexts.end()) {
             m_method_to_index.Append(entry);
           } else {
             // If we got here, we have something that had a context (was inside
-            // a namespace or class) yet we don't know if the entry
+            // a namespace or class) yet we don't know the entry
             m_method_to_index.Append(entry);
             m_basename_to_index.Append(entry);
           }
         }
       }
     }
     m_name_to_index.Sort();
     m_name_to_index.SizeToFit();
     m_selector_to_index.Sort();
     m_selector_to_index.SizeToFit();
     m_basename_to_index.Sort();
     m_basename_to_index.SizeToFit();
     m_method_to_index.Sort();
     m_method_to_index.SizeToFit();
   }
 }
 
+void Symtab::RegisterMangledNameEntry(
+    NameToIndexMap::Entry &entry, std::set<const char *> &class_contexts,
+    UniqueCStringMap<uint32_t> &mangled_name_to_index,
+    std::vector<const char *> &symbol_contexts, const RichManglingInfo &info) {
+  // Only register functions that have a base name.
+  const char *base_name_cstr = info.getFunctionBaseName();
+  if (base_name_cstr == nullptr)
+    return;
+
+  // The base name will be our entry's name.
+  entry.cstring = ConstString(base_name_cstr);
+
+  // Register functions with no context.
+  ConstString decl_context(info.getFunctionDeclContextName());
+  if (decl_context.IsEmpty()) {
+    // This has to be a basename
+    m_basename_to_index.Append(entry);
+    // If there is no context (no namespaces or class scopes that come before
+    // the function name) then this also could be a fullname.
+    m_name_to_index.Append(entry);
+    return;
+  }
+
+  // See if we already know this context.
+  auto it = class_contexts.find(decl_context.GetCString());
+
+  // Register constructors and destructors. They are methods and create
+  // declaration contexts.
+  if (info.isCtorOrDtor()) {
+    m_method_to_index.Append(entry);
+    if (it == class_contexts.end())
+      class_contexts.insert(it, decl_context.GetCString());
+    return;
+  }
+
+  // Register regular methods with a known declaration context.
+  if (it != class_contexts.end()) {
+    m_method_to_index.Append(entry);
+    return;
+  }
+
+  // Regular methods in unknown declaration contexts are put to the backlog. We
+  // will revisit them once we processed all remaining symbols.
+  mangled_name_to_index.Append(entry);
+  symbol_contexts[entry.value] = decl_context.GetCString();
+}
+
 void Symtab::PreloadSymbols() {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
   InitNameIndexes();
 }
 
 void Symtab::AppendSymbolNamesToMap(const IndexCollection &indexes,
                                     bool add_demangled, bool add_mangled,
                                     NameToIndexMap &name_to_index_map) const {