DWARFIndex: Reduce duplication in the GetFunctions methods

[labath]

Summary

This extracts the common bits out of the two implementations back into
the SymbolFileDWARF class. The goal was to make the function as similar
as possible to the other DWARFIndex methods (i.e, to restrict the work
of the index classes to DIE offset manipulation only, and leave the
offset interpretation to the caller).

To achieve this, i've split the function into three functions, for
looking up:

• full names
• objc methods
• methods and functions

The first two are trivial, and they just extract DIE offsets into an
array. The last one needed a slightly more complex interface, because of
the differences in how the two indexes operate -- manual index is able
to differentiate between methods and non-methods, while the apple table
is not. For this reason, the function returns three offset arrays
instead of just one. The first two are for DIEs which we know are/are
not methods, and the third one is for those we are unsure. In this case,
the caller needs to do extra work to tell them apart.

[clayborg]

Before I look too closely, we can probably add a DWARFDIE accessor function like:

bool DWARFDIE::IsClassMethod() const;

Then this would allow each DWARFIndex to just check and we might be able to get rid of the 3 lists in GetFunctionsByBaseOrMethodName? Seems like it shouldn't be hard for the index to weed these things out. Or we can add a generic layer into the DWARFIndex base class that the different indexes can call to weed out the things that aren't wanted. The earlier we can weed things out the better. so the index might return 20 matches, but it would be nice to weed out any mismatches as soon as possible so we can avoid creating any clang AST types for the functions/methods if we don't need do.

The example I am thinking of is "search for all methods called 'erase'" where the index might just be able to lookup "erase" and get 20 matches, but then before we realize/make any clang AST types, we would quickly call "DWARFDIE::IsClassMethod()" on it to weed out any mismatches before we hand them back. Let me know if that makes sense or if I am missing anything. I would like the DWARFIndex class to be as dead simple as possible.

[labath]

Changing the code to filter based on the dwarf information instead of the going through CompilerDeclContexts sounds like a good idea. I've been wondering why we are doing it this way -- the explanation I gave to myself was that this would allow the individual language plugins to decide what represents a "class" instead of just searching for DW_TAG_structure_type and similar. However, it's very likely that this is extra flexibility we don't need (and indeed, the manual index just checks the dwarf tags while building the index).

Unfortunately, something's come up, so I wasn't able to look into this closer today. Hopefully, I'll be able to get back to this patch later this week.

[labath]

This is a rewrite of the original patch with the same idea but different
approach. Now that Apple index determines method-ness straight from the debug
info, we don't need to resolve the functions into SymbolContexts. This removes
the need for the bunch of callback arguments and allows us to pull the common
part out of the two implementations of these functions back into the
SymbolFileDWARF class.

[clayborg]

Some if statements simplifications if you want to, but looks good.

source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp

193–195 ↗

(On Diff #149702)

if (ObjCLanguage::IsPossibleObjCMethodName(die_name))
  dies.push_back(die);

source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp

448–451 ↗

(On Diff #149702)

if (SymbolFileDWARF::DIEInDeclContext(&parent_decl_ctx, die))
  dies.push_back(die);

464 ↗

(On Diff #149702)

if (die)
  dies.push_back(die);

474–477 ↗

(On Diff #149702)

if (die)
  dies.push_back(die);

lldb/trunk/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.h

void GetGlobalVariables(const RegularExpression &regex,
                           DIEArray &offsets) override;
   void GetGlobalVariables(const DWARFUnit &cu, DIEArray &offsets) override;
   void GetObjCMethods(ConstString class_name, DIEArray &offsets) override;
   void GetCompleteObjCClass(ConstString class_name, bool must_be_implementation,
                             DIEArray &offsets) override;
   void GetTypes(ConstString name, DIEArray &offsets) override;
   void GetTypes(const DWARFDeclContext &context, DIEArray &offsets) override;
   void GetNamespaces(ConstString name, DIEArray &offsets) override;
-  void GetFunctions(
-      ConstString name, DWARFDebugInfo &info,
-      llvm::function_ref<bool(const DWARFDIE &die, bool include_inlines,
-                              lldb_private::SymbolContextList &sc_list)>
-          resolve_function,
-      llvm::function_ref<CompilerDeclContext(lldb::user_id_t type_uid)>
-          get_decl_context_containing_uid,
-      const CompilerDeclContext *parent_decl_ctx, uint32_t name_type_mask,
-      bool include_inlines, SymbolContextList &sc_list) override;
+  void GetFunctions(ConstString name, DWARFDebugInfo &info,
+                    const CompilerDeclContext &parent_decl_ctx,
+                    uint32_t name_type_mask,
+                    std::vector<DWARFDIE> &dies) override;
   void GetFunctions(
       const RegularExpression &regex, DWARFDebugInfo &info,
       llvm::function_ref<bool(const DWARFDIE &die, bool include_inlines,
                               lldb_private::SymbolContextList &sc_list)>
           resolve_function,
       bool include_inlines, SymbolContextList &sc_list) override;
 
   void ReportInvalidDIEOffset(dw_offset_t offset,

lldb/trunk/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp

 static bool KeepFunctionDIE(DWARFDIE die, uint32_t name_type_mask) {
   bool looking_for_methods = name_type_mask & eFunctionNameTypeMethod;
   bool looking_for_functions = name_type_mask & eFunctionNameTypeBase;
   if (looking_for_methods && looking_for_functions)
     return true;
   return looking_for_methods == die.IsMethod();
 }
 
-void AppleDWARFIndex::GetFunctions(
-    ConstString name, DWARFDebugInfo &info,
-    llvm::function_ref<bool(const DWARFDIE &die, bool include_inlines,
-                            lldb_private::SymbolContextList &sc_list)>
-        resolve_function,
-    llvm::function_ref<CompilerDeclContext(lldb::user_id_t type_uid)>
-        get_decl_context_containing_uid,
-    const CompilerDeclContext *parent_decl_ctx, uint32_t name_type_mask,
-    bool include_inlines, SymbolContextList &sc_list) {
-  if (!m_apple_names_up)
-    return;
-
-  std::set<const DWARFDebugInfoEntry *> resolved_dies;
-  DIEArray offsets;
-
-  uint32_t num_matches = 0;
-
+void AppleDWARFIndex::GetFunctions(ConstString name, DWARFDebugInfo &info,
+                                   const CompilerDeclContext &parent_decl_ctx,
+                                   uint32_t name_type_mask,
+                                   std::vector<DWARFDIE> &dies) {
   if (name_type_mask & eFunctionNameTypeFull) {
     // If they asked for the full name, match what they typed.  At some
     // point we may want to canonicalize this (strip double spaces, etc.
     // For now, we just add all the dies that we find by exact match.
-    num_matches = m_apple_names_up->FindByName(name.GetStringRef(), offsets);
-    for (uint32_t i = 0; i < num_matches; i++) {
-      const DIERef &die_ref = offsets[i];
+    DIEArray offsets;
+    m_apple_names_up->FindByName(name.GetStringRef(), offsets);
+    for (const DIERef &die_ref: offsets) {
       DWARFDIE die = info.GetDIE(die_ref);
-      if (die) {
-        if (!SymbolFileDWARF::DIEInDeclContext(parent_decl_ctx, die))
-          continue; // The containing decl contexts don't match
-
-        if (resolved_dies.find(die.GetDIE()) == resolved_dies.end()) {
-          if (resolve_function(die, include_inlines, sc_list))
-            resolved_dies.insert(die.GetDIE());
-        }
-      } else
+      if (!die) {
         ReportInvalidDIEOffset(die_ref.die_offset, name.GetStringRef());
+        continue;
       }
+      if (SymbolFileDWARF::DIEInDeclContext(&parent_decl_ctx, die))
+        dies.push_back(die);
     }
+  }
+  if (name_type_mask & eFunctionNameTypeSelector &&
+      !parent_decl_ctx.IsValid()) {
+    DIEArray offsets;
+    m_apple_names_up->FindByName(name.GetStringRef(), offsets);
 
-  if (name_type_mask & eFunctionNameTypeSelector) {
-    if (parent_decl_ctx && parent_decl_ctx->IsValid())
-      return; // no selectors in namespaces
-
-    num_matches = m_apple_names_up->FindByName(name.GetStringRef(), offsets);
     // Now make sure these are actually ObjC methods.  In this case we can
     // simply look up the name, and if it is an ObjC method name, we're
     // good.
-
-    for (uint32_t i = 0; i < num_matches; i++) {
-      const DIERef &die_ref = offsets[i];
+    for (const DIERef &die_ref: offsets) {
       DWARFDIE die = info.GetDIE(die_ref);
-      if (die) {
-        const char *die_name = die.GetName();
-        if (ObjCLanguage::IsPossibleObjCMethodName(die_name)) {
-          if (resolved_dies.find(die.GetDIE()) == resolved_dies.end()) {
-            if (resolve_function(die, include_inlines, sc_list))
-              resolved_dies.insert(die.GetDIE());
-          }
-        }
-      } else
+      if (!die) {
         ReportInvalidDIEOffset(die_ref.die_offset, name.GetStringRef());
+        continue;
+      }
+      const char *die_name = die.GetName();
+      if (ObjCLanguage::IsPossibleObjCMethodName(die_name))
+        dies.push_back(die);
     }
-    offsets.clear();
   }
-
-  if (((name_type_mask & eFunctionNameTypeMethod) && !parent_decl_ctx) ||
+  if (((name_type_mask & eFunctionNameTypeMethod) &&
+       !parent_decl_ctx.IsValid()) ||
       name_type_mask & eFunctionNameTypeBase) {
     // The apple_names table stores just the "base name" of C++ methods in
     // the table.  So we have to extract the base name, look that up, and
     // if there is any other information in the name we were passed in we
     // have to post-filter based on that.
 
-    // FIXME: Arrange the logic above so that we don't calculate the base
-    // name twice:
-    num_matches = m_apple_names_up->FindByName(name.GetStringRef(), offsets);
+    DIEArray offsets;
+    m_apple_names_up->FindByName(name.GetStringRef(), offsets);
 
-    for (uint32_t i = 0; i < num_matches; i++) {
-      const DIERef &die_ref = offsets[i];
+    for (const DIERef &die_ref: offsets) {
       DWARFDIE die = info.GetDIE(die_ref);
-      if (die) {
-        if (!SymbolFileDWARF::DIEInDeclContext(parent_decl_ctx, die))
-          continue; // The containing decl contexts don't match
-
-        if (!KeepFunctionDIE(die, name_type_mask))
-          continue;
-
-        if (resolved_dies.find(die.GetDIE()) != resolved_dies.end())
-          continue;
-
-        // If we get to here, the die is good, and we should add it:
-        if (resolve_function(die, include_inlines, sc_list))
-          resolved_dies.insert(die.GetDIE());
-      } else
+      if (!die) {
         ReportInvalidDIEOffset(die_ref.die_offset, name.GetStringRef());
+        continue;
+      }
+      if (SymbolFileDWARF::DIEInDeclContext(&parent_decl_ctx, die) &&
+          KeepFunctionDIE(die, name_type_mask))
+        dies.push_back(die);
     }
-    offsets.clear();
   }
 }
 
 void AppleDWARFIndex::GetFunctions(
     const RegularExpression &regex, DWARFDebugInfo &info,
     llvm::function_ref<bool(const DWARFDIE &die, bool include_inlines,
                             lldb_private::SymbolContextList &sc_list)>
         resolve_function,

lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFIndex.h

public:
   virtual void GetGlobalVariables(const DWARFUnit &cu, DIEArray &offsets) = 0;
   virtual void GetObjCMethods(ConstString class_name, DIEArray &offsets) = 0;
   virtual void GetCompleteObjCClass(ConstString class_name,
                                     bool must_be_implementation,
                                     DIEArray &offsets) = 0;
   virtual void GetTypes(ConstString name, DIEArray &offsets) = 0;
   virtual void GetTypes(const DWARFDeclContext &context, DIEArray &offsets) = 0;
   virtual void GetNamespaces(ConstString name, DIEArray &offsets) = 0;
-  virtual void GetFunctions(
-      ConstString name, DWARFDebugInfo &info,
-      llvm::function_ref<bool(const DWARFDIE &die, bool include_inlines,
-                              lldb_private::SymbolContextList &sc_list)>
-          resolve_function,
-      llvm::function_ref<CompilerDeclContext(lldb::user_id_t type_uid)>
-          get_decl_context_containing_uid,
-      const CompilerDeclContext *parent_decl_ctx, uint32_t name_type_mask,
-      bool include_inlines, SymbolContextList &sc_list) = 0;
+  virtual void GetFunctions(ConstString name, DWARFDebugInfo &info,
+                            const CompilerDeclContext &parent_decl_ctx,
+                            uint32_t name_type_mask,
+                            std::vector<DWARFDIE> &dies);
   virtual void GetFunctions(
       const RegularExpression &regex, DWARFDebugInfo &info,
       llvm::function_ref<bool(const DWARFDIE &die, bool include_inlines,
                               lldb_private::SymbolContextList &sc_list)>
           resolve_function,
       bool include_inlines, SymbolContextList &sc_list) = 0;
 
   virtual void ReportInvalidDIEOffset(dw_offset_t offset,

lldb/trunk/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.h

void GetGlobalVariables(const RegularExpression &regex,
                           DIEArray &offsets) override;
   void GetGlobalVariables(const DWARFUnit &cu, DIEArray &offsets) override;
   void GetObjCMethods(ConstString class_name, DIEArray &offsets) override;
   void GetCompleteObjCClass(ConstString class_name, bool must_be_implementation,
                             DIEArray &offsets) override;
   void GetTypes(ConstString name, DIEArray &offsets) override;
   void GetTypes(const DWARFDeclContext &context, DIEArray &offsets) override;
   void GetNamespaces(ConstString name, DIEArray &offsets) override;
-  void GetFunctions(
-      ConstString name, DWARFDebugInfo &info,
-      llvm::function_ref<bool(const DWARFDIE &die, bool include_inlines,
-                              lldb_private::SymbolContextList &sc_list)>
-          resolve_function,
-      llvm::function_ref<CompilerDeclContext(lldb::user_id_t type_uid)>
-          get_decl_context_containing_uid,
-      const CompilerDeclContext *parent_decl_ctx, uint32_t name_type_mask,
-      bool include_inlines, SymbolContextList &sc_list) override;
+  void GetFunctions(ConstString name, DWARFDebugInfo &info,
+                    const CompilerDeclContext &parent_decl_ctx,
+                    uint32_t name_type_mask,
+                    std::vector<DWARFDIE> &dies) override;
   void GetFunctions(
       const RegularExpression &regex, DWARFDebugInfo &info,
       llvm::function_ref<bool(const DWARFDIE &die, bool include_inlines,
                               lldb_private::SymbolContextList &sc_list)>
           resolve_function,
       bool include_inlines, SymbolContextList &sc_list) override;
 
   void ReportInvalidDIEOffset(dw_offset_t offset,

lldb/trunk/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp

void ManualDWARFIndex::GetTypes(const DWARFDeclContext &context,
   m_set.types.Find(ConstString(context[0].name), offsets);
 }
 
 void ManualDWARFIndex::GetNamespaces(ConstString name, DIEArray &offsets) {
   Index();
   m_set.namespaces.Find(name, offsets);
 }
 
-void ManualDWARFIndex::GetFunctions(
-    ConstString name, DWARFDebugInfo &info,
-    llvm::function_ref<bool(const DWARFDIE &die, bool include_inlines,
-                            lldb_private::SymbolContextList &sc_list)>
-        resolve_function,
-    llvm::function_ref<CompilerDeclContext(lldb::user_id_t type_uid)>
-        get_decl_context_containing_uid,
-    const CompilerDeclContext *parent_decl_ctx, uint32_t name_type_mask,
-    bool include_inlines, SymbolContextList &sc_list) {
-
+void ManualDWARFIndex::GetFunctions(ConstString name, DWARFDebugInfo &info,
+                                    const CompilerDeclContext &parent_decl_ctx,
+                                    uint32_t name_type_mask,
+                                    std::vector<DWARFDIE> &dies) {
   Index();
 
-  std::set<const DWARFDebugInfoEntry *> resolved_dies;
-  DIEArray offsets;
   if (name_type_mask & eFunctionNameTypeFull) {
-    uint32_t num_matches = m_set.function_basenames.Find(name, offsets);
-    num_matches += m_set.function_methods.Find(name, offsets);
-    num_matches += m_set.function_fullnames.Find(name, offsets);
-    for (uint32_t i = 0; i < num_matches; i++) {
-      const DIERef &die_ref = offsets[i];
+    DIEArray offsets;
+    m_set.function_basenames.Find(name, offsets);
+    m_set.function_methods.Find(name, offsets);
+    m_set.function_fullnames.Find(name, offsets);
+    for (const DIERef &die_ref: offsets) {
       DWARFDIE die = info.GetDIE(die_ref);
-      if (die) {
-        if (!SymbolFileDWARF::DIEInDeclContext(parent_decl_ctx, die))
-          continue; // The containing decl contexts don't match
-
-        if (resolved_dies.find(die.GetDIE()) == resolved_dies.end()) {
-          if (resolve_function(die, include_inlines, sc_list))
-            resolved_dies.insert(die.GetDIE());
-        }
-      }
+      if (!die)
+        continue;
+      if (SymbolFileDWARF::DIEInDeclContext(&parent_decl_ctx, die))
+        dies.push_back(die);
     }
-    offsets.clear();
   }
   if (name_type_mask & eFunctionNameTypeBase) {
-    uint32_t num_base = m_set.function_basenames.Find(name, offsets);
-    for (uint32_t i = 0; i < num_base; i++) {
-      DWARFDIE die = info.GetDIE(offsets[i]);
-      if (die) {
-        if (!SymbolFileDWARF::DIEInDeclContext(parent_decl_ctx, die))
-          continue; // The containing decl contexts don't match
-
-        // If we get to here, the die is good, and we should add it:
-        if (resolved_dies.find(die.GetDIE()) == resolved_dies.end()) {
-          if (resolve_function(die, include_inlines, sc_list))
-            resolved_dies.insert(die.GetDIE());
-        }
-      }
+    DIEArray offsets;
+    m_set.function_basenames.Find(name, offsets);
+    for (const DIERef &die_ref: offsets) {
+      DWARFDIE die = info.GetDIE(die_ref);
+      if (!die)
+        continue;
+      if (SymbolFileDWARF::DIEInDeclContext(&parent_decl_ctx, die))
+        dies.push_back(die);
     }
     offsets.clear();
   }
 
-  if (name_type_mask & eFunctionNameTypeMethod) {
-    if (parent_decl_ctx && parent_decl_ctx->IsValid())
-      return; // no methods in namespaces
-
-    uint32_t num_base = m_set.function_methods.Find(name, offsets);
-    {
-      for (uint32_t i = 0; i < num_base; i++) {
-        DWARFDIE die = info.GetDIE(offsets[i]);
-        if (die) {
-          // If we get to here, the die is good, and we should add it:
-          if (resolved_dies.find(die.GetDIE()) == resolved_dies.end()) {
-            if (resolve_function(die, include_inlines, sc_list))
-              resolved_dies.insert(die.GetDIE());
-          }
-        }
-      }
+  if (name_type_mask & eFunctionNameTypeMethod && !parent_decl_ctx.IsValid()) {
+    DIEArray offsets;
+    m_set.function_methods.Find(name, offsets);
+    for (const DIERef &die_ref: offsets) {
+      if (DWARFDIE die = info.GetDIE(die_ref))
+        dies.push_back(die);
     }
-    offsets.clear();
   }
 
-  if ((name_type_mask & eFunctionNameTypeSelector) &&
-      (!parent_decl_ctx || !parent_decl_ctx->IsValid())) {
-    uint32_t num_selectors = m_set.function_selectors.Find(name, offsets);
-    for (uint32_t i = 0; i < num_selectors; i++) {
-      DWARFDIE die = info.GetDIE(offsets[i]);
-      if (die) {
-        // If we get to here, the die is good, and we should add it:
-        if (resolved_dies.find(die.GetDIE()) == resolved_dies.end()) {
-          if (resolve_function(die, include_inlines, sc_list))
-            resolved_dies.insert(die.GetDIE());
-        }
-      }
+  if (name_type_mask & eFunctionNameTypeSelector &&
+      !parent_decl_ctx.IsValid()) {
+    DIEArray offsets;
+    m_set.function_selectors.Find(name, offsets);
+    for (const DIERef &die_ref: offsets) {
+      if (DWARFDIE die = info.GetDIE(die_ref))
+        dies.push_back(die);
     }
   }
 }
 
 void ManualDWARFIndex::GetFunctions(
     const RegularExpression &regex, DWARFDebugInfo &info,
     llvm::function_ref<bool(const DWARFDIE &die, bool include_inlines,
                             lldb_private::SymbolContextList &sc_list)>

lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

SymbolFileDWARF::FindFunctions(const ConstString &name,
   // appending the results to a variable list.
 
   const uint32_t original_size = sc_list.GetSize();
 
   DWARFDebugInfo *info = DebugInfo();
   if (info == NULL)
     return 0;
 
-  m_index->GetFunctions(name, *info,
-                        [this](const DWARFDIE &die, bool include_inlines,
-                               lldb_private::SymbolContextList &sc_list) {
-                          return ResolveFunction(die, include_inlines, sc_list);
-                        },
-                        [this](lldb::user_id_t type_uid) {
-                          return GetDeclContextContainingUID(type_uid);
-                        },
-                        parent_decl_ctx, name_type_mask, include_inlines,
-                        sc_list);
+  llvm::DenseSet<const DWARFDebugInfoEntry *> resolved_dies;
+  DIEArray offsets;
+  CompilerDeclContext empty_decl_ctx;
+  if (!parent_decl_ctx)
+    parent_decl_ctx = &empty_decl_ctx;
+
+  std::vector<DWARFDIE> dies;
+  m_index->GetFunctions(name, *info, *parent_decl_ctx, name_type_mask, dies);
+  for (const DWARFDIE &die: dies) {
+    if (resolved_dies.insert(die.GetDIE()).second)
+      ResolveFunction(die, include_inlines, sc_list);
+  }
 
   // Return the number of variable that were appended to the list
   const uint32_t num_matches = sc_list.GetSize() - original_size;
 
   if (log && num_matches > 0) {
     GetObjectFile()->GetModule()->LogMessage(
         log, "SymbolFileDWARF::FindFunctions (name=\"%s\", "
              "name_type_mask=0x%x, include_inlines=%d, append=%u, sc_list) => "