[lldb] Decouple ObjCLanguage from Symtab

[bulbazord]

Summary

We can extend/modify GetMethodNameVariants to suit our purposes here.
What symtab is looking for is alternate names we may want to use to
search for a specific symbol, and asking for variants of a name makes
the most sense here.

It might make more sense to wrap the ConstString and FunctionNameType
into a struct with a name, but for now I think a pair suffices.

[bulbazord]

Note that this is working towards solving the same problem as D103210 but it does so in a different way. Instead of repurposing the old diff, I made a new one so it is a little easier to compare/contrast. Hopefully it's not too confusing.

[jingham]

This looks pretty good to me.

It's a little awkward in InitNameIndexes that we look up the various NameToSymbolIndex maps by eFunctionNameType, use the function name type again to sort the names & index pairs into the bucket we looked up before. I wonder if that could be made cleaner by having an

AddToSymbolNameToIndexMap(symbol_name, index, func_name_type)

interface, which would just sort the symbol names into the right map. Not sure that's worth the bother, however.

lldb/source/Symbol/Symtab.cpp

343–353

Shouldn't this be in a loop over the supported languages?

[teemperor]

In D104067#2811834, @jingham wrote:

This looks pretty good to me.

It's a little awkward in InitNameIndexes that we look up the various NameToSymbolIndex maps by eFunctionNameType, use the function name type again to sort the names & index pairs into the bucket we looked up before. I wonder if that could be made cleaner by having an

AddToSymbolNameToIndexMap(symbol_name, index, func_name_type)

interface, which would just sort the symbol names into the right map. Not sure that's worth the bother, however.

That sounds good to me as a follow-up refactoring.

Only some small complains but otherwise this seems pretty good. Someone (*puts finger on nose*) should maybe do some stats whether

lldb/source/Breakpoint/BreakpointResolverName.cpp

222–223

Shouldn't that use the type form the variant instead of the name_type_mask? FWIW, I would prefer if we keep this code unchanged as right now we introduce the selectors to this list of lookups.

So what about adding filter here for eFunctionNameTypeFull to keep this patch NFC? And then maybe a FIXME: to figure out if we should add variants that aren't the full name.

lldb/source/Plugins/Language/ObjC/ObjCLanguage.h

104

Could we make this a custom struct? first second are always so non-descriptive and we might want to extend what we return from this function in a future patch.

class MethodNameVariant {
  ConstString m_name;
  lldb::FunctionNameType m_type;
public:
  [...]
}

lldb/source/Symbol/Symtab.cpp

343–353

+1

I also wonder if this might become quite expensive in the future if we end up with more language plugins, but I guess in that case we can make some kind of initial query pass where plugins can register whether they care about this indexing stuff here. Anyway, I don't think that's a real concern at the moment.

[shafik]

lldb/source/Plugins/Language/ObjC/ObjCLanguage.h

104

I second (pun intended?) this, if we can avoid using std::pair and instead use a custom struct we should.

[bulbazord]

Addressing comments

[bulbazord]

ping!

[teemperor]

In D104067#2825199, @bulbazord wrote:

ping!

Sorry, feel free to me directly sooner :)

I ran some benchmarks and this changed the average runtime of InitNameIndexes from 2.63s to 2.85s when debugging LLDB, which is ok-ish. The overhead seems to be from the language-plugin iteration and the new checks for the different function name types. We can't skip the new function name type checks in the new general approach, but we can avoid the expensive Language::ForEach call. If we just once collect the Language plugins in a list and then iterate over that we get down to an average runtime of 2.66s (which is pretty much just what we had before);

diff --git a/lldb/source/Symbol/Symtab.cpp b/lldb/source/Symbol/Symtab.cpp
index b61af37a356d..95c57243f20d 100644
--- a/lldb/source/Symbol/Symtab.cpp
+++ b/lldb/source/Symbol/Symtab.cpp
@@ -263,6 +263,13 @@ void Symtab::InitNameIndexes() {
     m_name_indexes_computed = true;
     LLDB_SCOPED_TIMER();
 
+    // Collect all loaded language plugins.
+    std::vector<Language *> languages;
+    Language::ForEach([&languages](Language *l) {
+      languages.push_back(l);
+      return true;
+    });
+
     auto &name_to_index = GetNameToSymbolIndexMap(lldb::eFunctionNameTypeNone);
     auto &basename_to_index =
         GetNameToSymbolIndexMap(lldb::eFunctionNameTypeBase);
@@ -329,7 +336,7 @@ void Symtab::InitNameIndexes() {
 
         // If the demangled name turns out to be an ObjC name, and is a category
         // name, add the version without categories to the index too.
-        auto map_variant_names = [&](Language *lang) {
+        for (Language *lang : languages) {
           for (auto variant : lang->GetMethodNameVariants(name)) {
             if (variant.GetType() & lldb::eFunctionNameTypeSelector)
               selector_to_index.Append(variant.GetName(), value);
@@ -340,10 +347,7 @@ void Symtab::InitNameIndexes() {
             else if (variant.GetType() & lldb::eFunctionNameTypeBase)
               basename_to_index.Append(variant.GetName(), value);
           }
-          return true;
         };
-
-        Language::ForEach(map_variant_names);
       }
     }

Beside that just two nits and this is ready to go. Thanks!

lldb/include/lldb/Target/Language.h

194

Both functions can be const

lldb/source/Plugins/Language/ObjC/ObjCLanguage.h

103–104

// Also returns the (the "We..." isn't something that is usually in doxygen comments which this comment is aspiring to be).

[bulbazord]

Addressed comments! :)

[teemperor]

LGTM, thanks!

lldb/include/lldb/Target/Language.h

 #ifndef LLDB_TARGET_LANGUAGE_H
 #define LLDB_TARGET_LANGUAGE_H
 
 #include <functional>
 #include <memory>
 #include <set>
 #include <vector>
 
 #include "lldb/Core/Highlighter.h"
 #include "lldb/Core/PluginInterface.h"
 #include "lldb/DataFormatters/DumpValueObjectOptions.h"
 #include "lldb/DataFormatters/FormatClasses.h"
 #include "lldb/DataFormatters/StringPrinter.h"
 #include "lldb/Symbol/TypeSystem.h"
 #include "lldb/lldb-private.h"
 #include "lldb/lldb-public.h"
 
public:
   virtual std::vector<ConstString>
   GetPossibleFormattersMatches(ValueObject &valobj,
                                lldb::DynamicValueType use_dynamic);
 
   virtual std::unique_ptr<TypeScavenger> GetTypeScavenger();
 
   virtual const char *GetLanguageSpecificTypeLookupHelp();
 
+  class MethodNameVariant {
+    ConstString m_name;
+    lldb::FunctionNameType m_type;
+
+  public:
+    MethodNameVariant(ConstString name, lldb::FunctionNameType type)
+        : m_name(name), m_type(type) {}
+    ConstString GetName() const { return m_name; }
+    lldb::FunctionNameType GetType() const { return m_type; }
+  };
   // If a language can have more than one possible name for a method, this
   // function can be used to enumerate them. This is useful when doing name
   // lookups.
-  virtual std::vector<ConstString>
+  virtual std::vector<Language::MethodNameVariant>
   GetMethodNameVariants(ConstString method_name) const {
-    return std::vector<ConstString>();
+    return std::vector<Language::MethodNameVariant>();
   };
 
   /// Returns true iff the given symbol name is compatible with the mangling
   /// scheme of this language.
   ///
   /// This function should only return true if there is a high confidence
   /// that the name actually belongs to this language.
   virtual bool SymbolNameFitsToLanguage(Mangled name) const { return false; }

lldb/source/Breakpoint/BreakpointResolverName.cpp

 //===-- BreakpointResolverName.cpp ----------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Breakpoint/BreakpointResolverName.h"
 
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Core/Architecture.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Symbol/Block.h"
 #include "lldb/Symbol/Function.h"
 #include "lldb/Symbol/Symbol.h"
 #include "lldb/Symbol/SymbolContext.h"

 }
 
 void BreakpointResolverName::AddNameLookup(ConstString name,
                                            FunctionNameType name_type_mask) {
 
   Module::LookupInfo lookup(name, name_type_mask, m_language);
   m_lookups.emplace_back(lookup);
 
   auto add_variant_funcs = [&](Language *lang) {
-    for (ConstString variant_name : lang->GetMethodNameVariants(name)) {
-      Module::LookupInfo variant_lookup(name, name_type_mask,
+    for (Language::MethodNameVariant variant :
+         lang->GetMethodNameVariants(name)) {
+      // FIXME: Should we be adding variants that aren't of type Full?
+      if (variant.GetType() & lldb::eFunctionNameTypeFull) {
+        Module::LookupInfo variant_lookup(name, variant.GetType(),
                                           lang->GetLanguageType());
-      variant_lookup.SetLookupName(variant_name);
+        variant_lookup.SetLookupName(variant.GetName());
         m_lookups.emplace_back(variant_lookup);
       }
+    }
     return true;
   };
 
   if (Language *lang = Language::FindPlugin(m_language)) {
     add_variant_funcs(lang);
   } else {
     // Most likely m_language is eLanguageTypeUnknown. We check each language for
     // possible variants or more qualified names and create lookups for those as

lldb/source/Plugins/Language/ObjC/ObjCLanguage.h

 //===-- ObjCLanguage.h ------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_SOURCE_PLUGINS_LANGUAGE_OBJC_OBJCLANGUAGE_H
 #define LLDB_SOURCE_PLUGINS_LANGUAGE_OBJC_OBJCLANGUAGE_H
 
 #include <cstring>
 #include <vector>
 
 #include "Plugins/Language/ClangCommon/ClangHighlighter.h"
 #include "lldb/Target/Language.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/lldb-private.h"
 
 namespace lldb_private {
 
 class ObjCLanguage : public Language {
   ClangHighlighter m_highlighter;
public:
   // If method_name is "+[NSString(my_additions) myStringWithCString:]"
   //   variant_names[0] => "+[NSString myStringWithCString:]"
   // If name is specified without the leading '+' or '-' like
   // "[NSString(my_additions) myStringWithCString:]"
   //  variant_names[0] => "+[NSString(my_additions) myStringWithCString:]"
   //  variant_names[1] => "-[NSString(my_additions) myStringWithCString:]"
   //  variant_names[2] => "+[NSString myStringWithCString:]"
   //  variant_names[3] => "-[NSString myStringWithCString:]"
-  std::vector<ConstString>
+  // Also returns the FunctionNameType of each possible name.
+  std::vector<Language::MethodNameVariant>
   GetMethodNameVariants(ConstString method_name) const override;
 
   bool SymbolNameFitsToLanguage(Mangled mangled) const override;
 
   lldb::TypeCategoryImplSP GetFormatters() override;
 
   std::vector<ConstString>
   GetPossibleFormattersMatches(ValueObject &valobj,

lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp

if (IsValid(false)) {
     if (!empty_if_no_category) {
       // Just return the full name since it doesn't have a category
       return GetFullName();
     }
   }
   return ConstString();
 }
 
-std::vector<ConstString>
+std::vector<Language::MethodNameVariant>
 ObjCLanguage::GetMethodNameVariants(ConstString method_name) const {
-  std::vector<ConstString> variant_names;
+  std::vector<Language::MethodNameVariant> variant_names;
   ObjCLanguage::MethodName objc_method(method_name.GetCString(), false);
   if (!objc_method.IsValid(false)) {
     return variant_names;
   }
 
+  variant_names.emplace_back(objc_method.GetSelector(),
+                             lldb::eFunctionNameTypeSelector);
+
   const bool is_class_method =
       objc_method.GetType() == MethodName::eTypeClassMethod;
   const bool is_instance_method =
       objc_method.GetType() == MethodName::eTypeInstanceMethod;
   ConstString name_sans_category =
       objc_method.GetFullNameWithoutCategory(/*empty_if_no_category*/ true);
 
   if (is_class_method || is_instance_method) {
     if (name_sans_category)
-      variant_names.emplace_back(name_sans_category);
+      variant_names.emplace_back(name_sans_category,
+                                 lldb::eFunctionNameTypeFull);
   } else {
     StreamString strm;
 
     strm.Printf("+%s", objc_method.GetFullName().GetCString());
-    variant_names.emplace_back(strm.GetString());
+    variant_names.emplace_back(ConstString(strm.GetString()),
+                               lldb::eFunctionNameTypeFull);
     strm.Clear();
 
     strm.Printf("-%s", objc_method.GetFullName().GetCString());
-    variant_names.emplace_back(strm.GetString());
+    variant_names.emplace_back(ConstString(strm.GetString()),
+                               lldb::eFunctionNameTypeFull);
     strm.Clear();
 
     if (name_sans_category) {
       strm.Printf("+%s", name_sans_category.GetCString());
-      variant_names.emplace_back(strm.GetString());
+      variant_names.emplace_back(ConstString(strm.GetString()),
+                                 lldb::eFunctionNameTypeFull);
       strm.Clear();
 
       strm.Printf("-%s", name_sans_category.GetCString());
-      variant_names.emplace_back(strm.GetString());
+      variant_names.emplace_back(ConstString(strm.GetString()),
+                                 lldb::eFunctionNameTypeFull);
     }
   }
 
   return variant_names;
 }
 
 bool ObjCLanguage::SymbolNameFitsToLanguage(Mangled mangled) const {
   ConstString demangled_name = mangled.GetDemangledName();

lldb/source/Symbol/CMakeLists.txt

add_lldb_library(lldbSymbol
   ${PLATFORM_SOURCES}
 
   LINK_LIBS
     lldbCore
     lldbExpression
     lldbHost
     lldbTarget
     lldbUtility
-    lldbPluginObjCLanguage
 
   LINK_COMPONENTS
     Support
   )

lldb/source/Symbol/Symtab.cpp

 //===-- Symtab.cpp --------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include <map>
 #include <set>
 
-#include "Plugins/Language/ObjC/ObjCLanguage.h"
-
 #include "lldb/Core/Module.h"
 #include "lldb/Core/RichManglingContext.h"
 #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/Target/Language.h"
 #include "lldb/Utility/RegularExpression.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/Timer.h"
 
 #include "llvm/ADT/StringRef.h"
 
 using namespace lldb;
 using namespace lldb_private;

 }
 
 void Symtab::InitNameIndexes() {
   // Protected function, no need to lock mutex...
   if (!m_name_indexes_computed) {
     m_name_indexes_computed = true;
     LLDB_SCOPED_TIMER();
 
+    // Collect all loaded language plugins.
+    std::vector<Language *> languages;
+    Language::ForEach([&languages](Language *l) {
+      languages.push_back(l);
+      return true;
+    });
+
     auto &name_to_index = GetNameToSymbolIndexMap(lldb::eFunctionNameTypeNone);
     auto &basename_to_index =
         GetNameToSymbolIndexMap(lldb::eFunctionNameTypeBase);
     auto &method_to_index =
         GetNameToSymbolIndexMap(lldb::eFunctionNameTypeMethod);
     auto &selector_to_index =
         GetNameToSymbolIndexMap(lldb::eFunctionNameTypeSelector);
     // Create the name index vector to be able to quickly search by name
for (uint32_t value = 0; value < num_symbols; ++value) {
           // the annotations.
           name = ConstString(
               m_objfile->StripLinkerSymbolAnnotations(name.GetStringRef()));
           name_to_index.Append(name, value);
         }
 
         // If the demangled name turns out to be an ObjC name, and is a category
         // name, add the version without categories to the index too.
-        ObjCLanguage::MethodName objc_method(name.GetStringRef(), true);
-        if (objc_method.IsValid(true)) {
-          selector_to_index.Append(objc_method.GetSelector(), value);
-
-          if (ConstString objc_method_no_category =
-                  objc_method.GetFullNameWithoutCategory(true))
-            name_to_index.Append(objc_method_no_category, value);
+        for (Language *lang : languages) {
+          for (auto variant : lang->GetMethodNameVariants(name)) {
+            if (variant.GetType() & lldb::eFunctionNameTypeSelector)
+              selector_to_index.Append(variant.GetName(), value);
+            else if (variant.GetType() & lldb::eFunctionNameTypeFull)
+              name_to_index.Append(variant.GetName(), value);
+            else if (variant.GetType() & lldb::eFunctionNameTypeMethod)
+              method_to_index.Append(variant.GetName(), value);
+            else if (variant.GetType() & lldb::eFunctionNameTypeBase)
+              basename_to_index.Append(variant.GetName(), value);
+          }
         }
       }
     }
 
     for (const auto &record : backlog) {
       RegisterBacklogEntry(record.first, record.second, class_contexts);
     }