[lldb] Refactor Module::LookupInfo constructor

[bulbazord]

Summary

Module::LookupInfo's constructor currently goes over supported languages
trying to figure out the best way to search for a symbol name. This
seems like a great candidate for refactoring. Specifically, this is work
that can be delegated to language plugins.

Once again, the goal here is to further decouple plugins from
non-plugins. The idea is to have each language plugin take a name and
give you back some information about the name from the perspective of
the language. Specifically, each language now implements a
GetFunctionNameInfo method which returns an object of type
Language::FunctionNameInfo. Right now, it consists of a basename,
a context, and a FunctionNameType. Module::LookupInfo's constructor will
call GetFunctionNameInfo with the appropriate language plugin(s) and
then decide what to do with that information. I have attempted to maintain
existing behavior as best as possible.

A nice side effect of this change is that lldbCore no longer links
against the ObjC Language plugin.

[bulbazord]

Delete now unused variable

[bulbazord]

ping!

[jingham]

I like the change in general, anywhere that we're explicitly listing languages in generic code we're probably doing it wrong... So far as I can tell you've reconstructed the old behaviors correctly in the new setup. I left one trivial comment, but really this LGTM.

lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp

73

The old code checked method.IsValid before pulling out the base name. It seems sensible that an invalid MethodName would return an empty base name, but that does seem to rely a little on implicit behavior? Not sure that's a big deal.

[bulbazord]

lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp

73

Yep, this does rely on implicit behavior. I don't think it's a huge deal, but it's easily changeable. :)

[aprantl]

FYI (you may have have already noticed) this causes some merge conflicts in swift-lldb. It would be great if we could work together to figure out how to best resolve them.

[bulbazord]

In D108229#2966405, @aprantl wrote:

FYI (you may have have already noticed) this causes some merge conflicts in swift-lldb. It would be great if we could work together to figure out how to best resolve them.

Yes, I figured that would happen at some point. I have an interest in seeing swift-lldb apply this change (and some of my other refactoring patches) so I would love to work with you and anyone else to get this change in smoothly. Unfortunately I had to revert this patch. It appears my environment had some issues that prevented me from running about half of lldb's test suite so I didn't catch any of the failures. I was finally able to debug the failing tests today and I realized that this change has several issues that need to be addressed. Once I have time to work through those issues and resubmit this patch, I'll add you as a reviewer and we can go from there. Does this sound good to you?

lldb/include/lldb/Core/Module.h

 //===-- Module.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_CORE_MODULE_H
 #define LLDB_CORE_MODULE_H
 
 #include "lldb/Core/Address.h"
 #include "lldb/Core/ModuleList.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Symbol/SymbolContextScope.h"
 #include "lldb/Symbol/TypeSystem.h"
 #include "lldb/Target/PathMappingList.h"
 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/ConstString.h"
public:
   /// Function lookups are done in Module.cpp, ModuleList.cpp and in
   /// BreakpointResolverName.cpp and they all now use this class to do lookups
   /// correctly.
   class LookupInfo {
   public:
     LookupInfo() : m_name(), m_lookup_name() {}
 
     LookupInfo(ConstString name, lldb::FunctionNameType name_type_mask,
-               lldb::LanguageType language);
+               lldb::LanguageType language_type);
 
     ConstString GetName() const { return m_name; }
 
     void SetName(ConstString name) { m_name = name; }
 
     ConstString GetLookupName() const { return m_lookup_name; }
 
     void SetLookupName(ConstString name) { m_lookup_name = name; }
public:
   protected:
     /// What the user originally typed
     ConstString m_name;
 
     /// The actual name will lookup when calling in the object or symbol file
     ConstString m_lookup_name;
 
     /// Limit matches to only be for this language
-    lldb::LanguageType m_language = lldb::eLanguageTypeUnknown;
+    lldb::LanguageType m_language_type = lldb::eLanguageTypeUnknown;
 
     /// One or more bits from lldb::FunctionNameType that indicate what kind of
     /// names we are looking for
     lldb::FunctionNameType m_name_type_mask = lldb::eFunctionNameTypeNone;
 
     ///< If \b true, then demangled names that match will need to contain
     ///< "m_name" in order to be considered a match
     bool m_match_name_after_lookup = false;

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:
   // 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<Language::MethodNameVariant>
   GetMethodNameVariants(ConstString method_name) const {
     return std::vector<Language::MethodNameVariant>();
   };
 
+  class FunctionNameInfo {
+  public:
+    llvm::StringRef basename;
+    llvm::StringRef context;
+    lldb::FunctionNameType func_name_type;
+  };
+
+  virtual Language::FunctionNameInfo
+  GetFunctionNameInfo(ConstString name) const {
+    FunctionNameInfo ret;
+    ret.func_name_type = lldb::eFunctionNameTypeNone;
+    return ret;
+  };
+
   /// 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; }
 
   // if an individual data formatter can apply to several types and cross a

lldb/source/Core/CMakeLists.txt

LINK_LIBS
     lldbDataFormatters
     lldbExpression
     lldbHost
     lldbInterpreter
     lldbSymbol
     lldbTarget
     lldbUtility
     lldbPluginCPlusPlusLanguage
-    lldbPluginObjCLanguage
     ${LLDB_CURSES_LIBS}
 
   CLANG_LIBS
     clangDriver
 
   LINK_COMPONENTS
     Support
     Demangle

lldb/source/Core/Module.cpp

 //===-- Module.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/Core/Module.h"
 
 #include "lldb/Core/AddressRange.h"
 #include "lldb/Core/AddressResolverFileLine.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/FileSpecList.h"
 #include "lldb/Core/Mangled.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/SearchFilter.h"

 #include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/Timer.h"
 
 #if defined(_WIN32)
 #include "lldb/Host/windows/PosixApi.h"
 #endif
 
 #include "Plugins/Language/CPlusPlus/CPlusPlusLanguage.h"
-#include "Plugins/Language/ObjC/ObjCLanguage.h"
 
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include <cassert>
if (sc.comp_unit) {
       if (FileSpec::Match(path, sc.comp_unit->GetPrimaryFile()))
         sc_list.Append(sc);
     }
   }
 }
 
 Module::LookupInfo::LookupInfo(ConstString name,
                                FunctionNameType name_type_mask,
-                               LanguageType language)
-    : m_name(name), m_lookup_name(), m_language(language),
+                               LanguageType language_type)
+    : m_name(name), m_lookup_name(name), m_language_type(language_type),
       m_name_type_mask(eFunctionNameTypeNone),
       m_match_name_after_lookup(false) {
-  const char *name_cstr = name.GetCString();
   llvm::StringRef basename;
   llvm::StringRef context;
 
+  std::vector<Language *> languages;
+  Language::ForEach([&languages](Language *l) {
+    languages.push_back(l);
+    return true;
+  });
+
   if (name_type_mask & eFunctionNameTypeAuto) {
-    if (CPlusPlusLanguage::IsCPPMangledName(name_cstr))
-      m_name_type_mask = eFunctionNameTypeFull;
-    else if ((language == eLanguageTypeUnknown ||
-              Language::LanguageIsObjC(language)) &&
-             ObjCLanguage::IsPossibleObjCMethodName(name_cstr))
-      m_name_type_mask = eFunctionNameTypeFull;
-    else if (Language::LanguageIsC(language)) {
-      m_name_type_mask = eFunctionNameTypeFull;
-    } else {
-      if ((language == eLanguageTypeUnknown ||
-           Language::LanguageIsObjC(language)) &&
-          ObjCLanguage::IsPossibleObjCSelector(name_cstr))
-        m_name_type_mask |= eFunctionNameTypeSelector;
-
-      CPlusPlusLanguage::MethodName cpp_method(name);
-      basename = cpp_method.GetBasename();
-      if (basename.empty()) {
-        if (CPlusPlusLanguage::ExtractContextAndIdentifier(name_cstr, context,
-                                                           basename))
-          m_name_type_mask |= (eFunctionNameTypeMethod | eFunctionNameTypeBase);
-        else
-          m_name_type_mask |= eFunctionNameTypeFull;
-      } else {
-        m_name_type_mask |= (eFunctionNameTypeMethod | eFunctionNameTypeBase);
+    if (language_type == eLanguageTypeUnknown) {
+      for (Language *lang : languages) {
+        Language::FunctionNameInfo info = lang->GetFunctionNameInfo(name);
+        if (info.func_name_type != eFunctionNameTypeNone) {
+          m_name_type_mask = info.func_name_type;
+          basename = info.basename;
+          context = info.context;
+          break;
         }
       }
     } else {
-    m_name_type_mask = name_type_mask;
-    if (name_type_mask & eFunctionNameTypeMethod ||
-        name_type_mask & eFunctionNameTypeBase) {
-      // If they've asked for a CPP method or function name and it can't be
-      // that, we don't even need to search for CPP methods or names.
-      CPlusPlusLanguage::MethodName cpp_method(name);
-      if (cpp_method.IsValid()) {
-        basename = cpp_method.GetBasename();
-
-        if (!cpp_method.GetQualifiers().empty()) {
-          // There is a "const" or other qualifier following the end of the
-          // function parens, this can't be a eFunctionNameTypeBase
-          m_name_type_mask &= ~(eFunctionNameTypeBase);
-          if (m_name_type_mask == eFunctionNameTypeNone)
-            return;
-        }
-      } else {
-        // If the CPP method parser didn't manage to chop this up, try to fill
-        // in the base name if we can. If a::b::c is passed in, we need to just
-        // look up "c", and then we'll filter the result later.
-        CPlusPlusLanguage::ExtractContextAndIdentifier(name_cstr, context,
-                                                       basename);
+      if (auto *lang = Language::FindPlugin(language_type)) {
+        Language::FunctionNameInfo info = lang->GetFunctionNameInfo(name);
+        m_name_type_mask = info.func_name_type;
+        basename = info.basename;
+        context = info.context;
       }
     }
 
-    if (name_type_mask & eFunctionNameTypeSelector) {
-      if (!ObjCLanguage::IsPossibleObjCSelector(name_cstr)) {
-        m_name_type_mask &= ~(eFunctionNameTypeSelector);
-        if (m_name_type_mask == eFunctionNameTypeNone)
-          return;
+    // NOTE: There are several ways to get here, but this is a fallback path in
+    // case the above does not succeed at extracting any useful information from
+    // the loaded language plugins.
+      m_name_type_mask = eFunctionNameTypeFull;
+
+  } else {
+    m_name_type_mask = name_type_mask;
+    if (language_type != eLanguageTypeUnknown) {
+      if (auto *lang = Language::FindPlugin(language_type)) {
+        Language::FunctionNameInfo info = lang->GetFunctionNameInfo(name);
+        if (info.func_name_type & m_name_type_mask) {
+          // If the user asked for FunctionNameTypes that aren't possible,
+          // then filter those out. (e.g. asking for Selectors on
+          // C++ symbols, or even if the symbol given can't be a selector in
+          // ObjC)
+          m_name_type_mask &= info.func_name_type;
+          basename = info.basename;
+          context = info.context;
         }
       }
-
-    // Still try and get a basename in case someone specifies a name type mask
-    // of eFunctionNameTypeFull and a name like "A::func"
-    if (basename.empty()) {
-      if (name_type_mask & eFunctionNameTypeFull &&
-          !CPlusPlusLanguage::IsCPPMangledName(name_cstr)) {
-        CPlusPlusLanguage::MethodName cpp_method(name);
-        basename = cpp_method.GetBasename();
-        if (basename.empty())
-          CPlusPlusLanguage::ExtractContextAndIdentifier(name_cstr, context,
-                                                         basename);
+    } else {
+      for (Language *lang : languages) {
+        Language::FunctionNameInfo info = lang->GetFunctionNameInfo(name);
+        if (info.func_name_type & m_name_type_mask) {
+          m_name_type_mask &= info.func_name_type;
+          basename = info.basename;
+          context = info.context;
+          break;
+        }
       }
     }
   }
 
   if (!basename.empty()) {
-    // The name supplied was a partial C++ path like "a::count". In this case
-    // we want to do a lookup on the basename "count" and then make sure any
-    // matching results contain "a::count" so that it would match "b::a::count"
-    // and "a::count". This is why we set "match_name_after_lookup" to true
+    // The name supplied was incomplete for lookup purposes. For example, in C++
+    // we may have gotten something like "a::count". In this case, we want to do
+    // a lookup on the basename "count" and then make sure any matching results
+    // contain "a::count" so that it would match "b::a::count" and "a::count".
+    // This is why we set match_name_after_lookup to true.
     m_lookup_name.SetString(basename);
     m_match_name_after_lookup = true;
-  } else {
-    // The name is already correct, just use the exact name as supplied, and we
-    // won't need to check if any matches contain "name"
-    m_lookup_name = name;
-    m_match_name_after_lookup = false;
   }
 }
 
 void Module::LookupInfo::Prune(SymbolContextList &sc_list,
                                size_t start_idx) const {
   if (m_match_name_after_lookup && m_name) {
     SymbolContext sc;
     size_t i = start_idx;

lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h

 //===-- CPlusPlusLanguage.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_CPLUSPLUS_CPLUSPLUSLANGUAGE_H
 #define LLDB_SOURCE_PLUGINS_LANGUAGE_CPLUSPLUS_CPLUSPLUSLANGUAGE_H
 
 #include <set>
 #include <vector>
 
 #include "llvm/ADT/StringRef.h"
 
 #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 CPlusPlusLanguage : public Language {
   ClangHighlighter m_highlighter;
public:
   static void Initialize();
 
   static void Terminate();
 
   static lldb_private::Language *CreateInstance(lldb::LanguageType language);
 
   static lldb_private::ConstString GetPluginNameStatic();
 
+  Language::FunctionNameInfo
+  GetFunctionNameInfo(ConstString name) const override;
+
   bool SymbolNameFitsToLanguage(Mangled mangled) const override;
 
   ConstString
   GetDemangledFunctionNameWithoutArguments(Mangled mangled) const override;
 
   static bool IsCPPMangledName(llvm::StringRef name);
 
   // Extract C++ context and identifier from a string using heuristic matching

lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp

void CPlusPlusLanguage::Terminate() {
   PluginManager::UnregisterPlugin(CreateInstance);
 }
 
 lldb_private::ConstString CPlusPlusLanguage::GetPluginNameStatic() {
   static ConstString g_name("cplusplus");
   return g_name;
 }
 
+Language::FunctionNameInfo
+CPlusPlusLanguage::GetFunctionNameInfo(ConstString name) const {
+  FunctionNameInfo info;
+  info.func_name_type = lldb::eFunctionNameTypeNone;
+
+  if (IsCPPMangledName(name.GetCString())) {
+    info.func_name_type = lldb::eFunctionNameTypeFull;
+  }
+
+  CPlusPlusLanguage::MethodName method(name);
+  llvm::StringRef basename = method.GetBasename();
+  if (basename.empty()) {
+    if (CPlusPlusLanguage::ExtractContextAndIdentifier(
+            name.GetCString(), info.context, info.basename)) {
+      info.func_name_type |=
+          (lldb::eFunctionNameTypeMethod | eFunctionNameTypeBase);
+    } else {
+      info.func_name_type |= lldb::eFunctionNameTypeFull;
+    }
+  } else {
+    info.func_name_type |=
+        (lldb::eFunctionNameTypeMethod | eFunctionNameTypeBase);
+  }
+
+  if (!method.GetQualifiers().empty()) {
+    // There is a 'const' or other qualifier following the end of the function
+    // parens, this can't be a eFunctionNameTypeBase.
+    info.func_name_type &= ~(lldb::eFunctionNameTypeBase);
+  }
+
+  return info;
+}
+
 bool CPlusPlusLanguage::SymbolNameFitsToLanguage(Mangled mangled) const {
   const char *mangled_name = mangled.GetMangledName().GetCString();
   return mangled_name && CPlusPlusLanguage::IsCPPMangledName(mangled_name);
 }
 
 ConstString CPlusPlusLanguage::GetDemangledFunctionNameWithoutArguments(
     Mangled mangled) const {
   const char *mangled_name_cstr = mangled.GetMangledName().GetCString();

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:
   //  variant_names[0] => "+[NSString(my_additions) myStringWithCString:]"
   //  variant_names[1] => "-[NSString(my_additions) myStringWithCString:]"
   //  variant_names[2] => "+[NSString myStringWithCString:]"
   //  variant_names[3] => "-[NSString myStringWithCString:]"
   // Also returns the FunctionNameType of each possible name.
   std::vector<Language::MethodNameVariant>
   GetMethodNameVariants(ConstString method_name) const override;
 
+  Language::FunctionNameInfo
+  GetFunctionNameInfo(ConstString name) const override;
+
   bool SymbolNameFitsToLanguage(Mangled mangled) const override;
 
   lldb::TypeCategoryImplSP GetFormatters() override;
 
   std::vector<ConstString>
   GetPossibleFormattersMatches(ValueObject &valobj,
                                lldb::DynamicValueType use_dynamic) override;
 

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

if (name_sans_category) {
       variant_names.emplace_back(ConstString(strm.GetString()),
                                  lldb::eFunctionNameTypeFull);
     }
   }
 
   return variant_names;
 }
 
+Language::FunctionNameInfo
+ObjCLanguage::GetFunctionNameInfo(ConstString name) const {
+  Language::FunctionNameInfo info;
+  info.func_name_type = lldb::eFunctionNameTypeNone;
+
+  if (ObjCLanguage::IsPossibleObjCMethodName(name.GetCString())) {
+    info.func_name_type = lldb::eFunctionNameTypeFull;
+  }
+
+  if (ObjCLanguage::IsPossibleObjCSelector(name.GetCString())) {
+    info.func_name_type |= lldb::eFunctionNameTypeSelector;
+  }
+
+  return info;
+}
+
 bool ObjCLanguage::SymbolNameFitsToLanguage(Mangled mangled) const {
   ConstString demangled_name = mangled.GetDemangledName();
   if (!demangled_name)
     return false;
   return ObjCLanguage::IsPossibleObjCMethodName(demangled_name.GetCString());
 }
 
 static void LoadObjCFormatters(TypeCategoryImplSP objc_category_sp) {