[DWARF] Align non-accelerated function fullname searching with the apple-tables path

[labath]

Summary

Before this patch the two paths were doing very different things

• the apple path searched the .apple_names section, which contained mangled names, as well as basenames of all functions. It returned any name it found.
• the non-accelerated path looked in the "full name" index we built ourselves, which contained mangled as well as demangled names of all functions (but no basenames). Then however, if it did not find a match it did an extra search in the basename index, with some special handling for anonymous namespaces.

This aligns the two paths by changing the non-accelerated path to return
the same results as in the apple-tables one. In pratice, this means we
will search in both the "basename" and "method" indexes (in the manual
indexes we keep methods separately). This means the function will return
some slightly inappropriate results (e.g. bar::baz::foo when one asks
for a "full name" foo), but this can be handled by additional filtering,
independently indexing method. To make this work, I've also started
inserting mangled names into the manual "basename" index.

I've also stopped matching demangled names when a regex search is
requested as that is not what apple tables do.

After this change the manual "fullname" is unused and can be removed.

[clayborg]

See inlined comments

source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

901–902 ↗

(On Diff #145669)

Why are we adding the mangled name to the basenames?

905–908 ↗

(On Diff #145669)

Should we remove these 4 lines now? I thought we weren't going to add demangled names to the index?

928–929 ↗

(On Diff #145669)

Why are we adding the mangled name to the basenames?

[labath]

source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

905–908 ↗

(On Diff #145669)

I was saving that for a separate patch.

928–929 ↗

(On Diff #145669)

It was the simplest way to ensure we get the same results for apple and !apple cases. With apple tables we will return a result if someone asks for a function with a "base name" _Z3foov. I am not sure if this was intended or just an accident. Alternatively, I could keep these names in a separate index and then search in both when I get a query. Or, if this is really not intended to work, I can adding extra filtering to the "apple" path to return only "real" basenames. (My goal here is to eliminate platform differences to avoid things behaving differently across platforms.)

[clayborg]

source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

928–929 ↗

(On Diff #145669)

I would search "func_fullnames" and "func_basenames" separately and avoid adding the extra mangled name to func_basenames.

The _Z3foov seems like an accident. It definitely shouldn't be a basename unless the DWARF put that into the DW_AT_name field for some reason.

[labath]

Stopped inserting demangled names into the fullname index and made the
FindFunctions index search in all three indices (fullname, basename, method).
The last two shouldn't really contain full names, but they seem to be required
for the correct operation (and they match what the apple path does).

[clayborg]

Much better.

[clayborg]

Before adding the DWARF 5 index support, we should virtualize this searching and indexing into a base class. Then we make 1 class for manual DWARF indexing, one for Apple indexes, and one for DWARF5. Then we can unit test each one easily. Thoughts?

[clayborg]

the class would be something like:

class DWARFIndex {

virtual bool Initialize(SymbolFileDWARF &dwarf); // Do manual indexing, or load indexes. Return true if successful (found apple tables, DWARF 5 tables, or had debug info to manually index)
virtual ... Lookup(ConstString name, type, etc);

};

[labath]

In D46576#1091548, @clayborg wrote:

Before adding the DWARF 5 index support, we should virtualize this searching and indexing into a base class. Then we make 1 class for manual DWARF indexing, one for Apple indexes, and one for DWARF5. Then we can unit test each one easily. Thoughts?

Yes, that's exactly where I'm heading. I started doing that, but then I noticed that two of the existing paths were sometimes very different for no apparent reason. I'm trying to bring them closer together to make the interface of the index class saner.

lldb/trunk/lit/SymbolFile/DWARF/find-basic-function.cpp

 // REQUIRES: lld
 
 // RUN: clang %s -g -c -o %t.o --target=x86_64-pc-linux
 // RUN: ld.lld %t.o -o %t
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=base %t | \
 // RUN:   FileCheck --check-prefix=BASE %s
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=method %t | \
 // RUN:   FileCheck --check-prefix=METHOD %s
 // RUN: lldb-test symbols --name=foo --find=function --function-flags=full %t | \
 // RUN:   FileCheck --check-prefix=FULL %s
+// RUN: lldb-test symbols --name=_Z3fooi --find=function --function-flags=full %t | \
+// RUN:   FileCheck --check-prefix=FULL-MANGLED %s
 // RUN: lldb-test symbols --name=foo --context=context --find=function --function-flags=base %t | \
 // RUN:   FileCheck --check-prefix=CONTEXT %s
 // RUN: lldb-test symbols --name=not_there --find=function %t | \
 // RUN:   FileCheck --check-prefix=EMPTY %s
 
 // BASE: Found 4 functions:
 // BASE-DAG: name = "foo()", mangled = "_Z3foov"
 // BASE-DAG: name = "foo(int)", mangled = "_Z3fooi"
 // BASE-DAG: name = "bar::foo()", mangled = "_ZN3bar3fooEv"
 // BASE-DAG: name = "bar::baz::foo()", mangled = "_ZN3bar3baz3fooEv"
 
 // METHOD: Found 3 functions:
 // METHOD-DAG: name = "sbar::foo()", mangled = "_ZN4sbar3fooEv"
 // METHOD-DAG: name = "sbar::foo(int)", mangled = "_ZN4sbar3fooEi"
 // METHOD-DAG: name = "ffbar()::sbar::foo()", mangled = "_ZZ5ffbarvEN4sbar3fooEv"
 
-// FULL: Found 2 functions:
+// FULL: Found 7 functions:
 // FULL-DAG: name = "foo()", mangled = "_Z3foov"
 // FULL-DAG: name = "foo(int)", mangled = "_Z3fooi"
+// FULL-DAG: name = "bar::foo()", mangled = "_ZN3bar3fooEv"
+// FULL-DAG: name = "bar::baz::foo()", mangled = "_ZN3bar3baz3fooEv"
+// FULL-DAG: name = "sbar::foo()", mangled = "_ZN4sbar3fooEv"
+// FULL-DAG: name = "sbar::foo(int)", mangled = "_ZN4sbar3fooEi"
+// FULL-DAG: name = "ffbar()::sbar::foo()", mangled = "_ZZ5ffbarvEN4sbar3fooEv"
+
+// FULL-MANGLED: Found 1 functions:
+// FULL-DAG: name = "foo(int)", mangled = "_Z3fooi"
 
 // CONTEXT: Found 1 functions:
 // CONTEXT-DAG: name = "bar::foo()", mangled = "_ZN3bar3fooEv"
 
 // EMPTY: Found 0 functions:
 
 void foo() {}
 void foo(int) {}

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

case DW_TAG_subprogram:
         if (mangled_cstr) {
           // Make sure our mangled name isn't the same string table entry as
           // our name. If it starts with '_', then it is ok, else compare the
           // string to make sure it isn't the same and we don't end up with
           // duplicate entries
           if (name && name != mangled_cstr &&
               ((mangled_cstr[0] == '_') ||
                (::strcmp(name, mangled_cstr) != 0))) {
-            Mangled mangled(ConstString(mangled_cstr), true);
-            func_fullnames.Insert(mangled.GetMangledName(),
-                                  DIERef(cu_offset, die.GetOffset()));
-            ConstString demangled = mangled.GetDemangledName(cu_language);
-            if (demangled)
-              func_fullnames.Insert(demangled,
-                                    DIERef(cu_offset, die.GetOffset()));
+            func_fullnames.Insert(ConstString(mangled_cstr),
           }
         }
       }
       break;
 
     case DW_TAG_inlined_subroutine:
       if (has_address) {
         if (name)
           func_basenames.Insert(ConstString(name),
                                 DIERef(cu_offset, die.GetOffset()));
         if (mangled_cstr) {
           // Make sure our mangled name isn't the same string table entry as
           // our name. If it starts with '_', then it is ok, else compare the
           // string to make sure it isn't the same and we don't end up with
           // duplicate entries
           if (name && name != mangled_cstr &&
               ((mangled_cstr[0] == '_') ||
                (::strcmp(name, mangled_cstr) != 0))) {
-            Mangled mangled(ConstString(mangled_cstr), true);
-            func_fullnames.Insert(mangled.GetMangledName(),
-                                  DIERef(cu_offset, die.GetOffset()));
-            ConstString demangled = mangled.GetDemangledName(cu_language);
-            if (demangled)
-              func_fullnames.Insert(demangled,
-                                    DIERef(cu_offset, die.GetOffset()));
+            func_fullnames.Insert(ConstString(mangled_cstr),
           }
         } else
           func_fullnames.Insert(ConstString(name),
                                 DIERef(cu_offset, die.GetOffset()));
       }
       break;
 
     case DW_TAG_array_type:

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

if (m_apple_names_ap.get()) {
       }
     }
   } else {
 
     // Index the DWARF if we haven't already
     if (!m_indexed)
       Index();
 
+    DIEArray die_offsets;
     if (name_type_mask & eFunctionNameTypeFull) {
-      FindFunctions(name, m_function_fullname_index, include_inlines, sc_list);
+      uint32_t num_matches = m_function_basename_index.Find(name, die_offsets);
+      num_matches += m_function_method_index.Find(name, die_offsets);
+      num_matches += m_function_fullname_index.Find(name, die_offsets);
+      for (uint32_t i = 0; i < num_matches; i++) {
+        const DIERef &die_ref = die_offsets[i];
+        DWARFDIE die = info->GetDIE(die_ref);
+        if (die) {
+          if (!DIEInDeclContext(parent_decl_ctx, die))
+            continue; // The containing decl contexts don't match
 
-      // FIXME Temporary workaround for global/anonymous namespace
-      // functions debugging FreeBSD and Linux binaries. If we didn't find any
-      // functions in the global namespace try looking in the basename index
-      // but ignore any returned functions that have a namespace but keep
-      // functions which have an anonymous namespace
-      // TODO: The arch in the object file isn't correct for MSVC
-      // binaries on windows, we should find a way to make it correct and
-      // handle those symbols as well.
-      if (sc_list.GetSize() == original_size) {
-        ArchSpec arch;
-        if (!parent_decl_ctx && GetObjectFile()->GetArchitecture(arch) &&
-            arch.GetTriple().isOSBinFormatELF()) {
-          SymbolContextList temp_sc_list;
-          FindFunctions(name, m_function_basename_index, include_inlines,
-                        temp_sc_list);
-          SymbolContext sc;
-          for (uint32_t i = 0; i < temp_sc_list.GetSize(); i++) {
-            if (temp_sc_list.GetContextAtIndex(i, sc)) {
-              ConstString mangled_name =
-                  sc.GetFunctionName(Mangled::ePreferMangled);
-              ConstString demangled_name =
-                  sc.GetFunctionName(Mangled::ePreferDemangled);
-              // Mangled names on Linux and FreeBSD are of the form:
-              // _ZN18function_namespace13function_nameEv.
-              if (strncmp(mangled_name.GetCString(), "_ZN", 3) ||
-                  !strncmp(demangled_name.GetCString(), "(anonymous namespace)",
-                           21)) {
-                sc_list.Append(sc);
-              }
-            }
+          if (resolved_dies.find(die.GetDIE()) == resolved_dies.end()) {
+            if (ResolveFunction(die, include_inlines, sc_list))
+              resolved_dies.insert(die.GetDIE());
           }
         }
       }
+      die_offsets.clear();
     }
-    DIEArray die_offsets;
     if (name_type_mask & eFunctionNameTypeBase) {
       uint32_t num_base = m_function_basename_index.Find(name, die_offsets);
       for (uint32_t i = 0; i < num_base; i++) {
         DWARFDIE die = info->GetDIE(die_offsets[i]);
         if (die) {
           if (!DIEInDeclContext(parent_decl_ctx, die))
             continue; // The containing decl contexts don't match