Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

[PatriosTheGreat]

Summary

This change is connected with
https://reviews.llvm.org/D69843

In large codebases, we sometimes see Module::FindFunctions (when called from
ClangExpressionDeclMap::FindExternalVisibleDecls) returning huge amounts of
functions.

In current fix I trying to return only function_fullnames from ManualDWARFIndex::GetFunctions when eFunctionNameTypeFull is passed as argument.

I run check-lldb on this changes on linux machine and it seems like there are no additional failing tests compared with master. It seems a bit weird that there are no other places in code were it is expected that eFunctionNameTypeFull on GetFunctions will return fullnames + basenames + function_methods.
Is there any additional tests which I can run to be sure that I didn't break anything?

[labath]

There's lldb-shell :: SymbolFile/DWARF/find-basic-function.cpp, which probably didn't get run for you as you didn't have lld enabled (cmake -DLLVM_ENABLE_PROJECTS=...;lld). You'll need to update that test to match the new behavior, but other than that, I think this is a good change.

[teemperor]

This LGTM, but the TODO directly above the change in ClangExpressionDeclMap.cpp worries me a bit. I am not sure how good our test coverage is for calling functions when there are instance methods in scope (e.g., we are in instance method and try to call another instance method).

[shafik]

In D70846#1763802, @teemperor wrote:

This LGTM, but the TODO directly above the change in ClangExpressionDeclMap.cpp worries me a bit. I am not sure how good our test coverage is for calling functions when there are instance methods in scope (e.g., we are in instance method and try to call another instance method).

The comment was added by this change

I am concerned about regressions since we are not sure how well this is covered.

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

1213

I did some archeology here and this was changed from eFunctionNameTypeBase to eFunctionNameTypeFull by this change so maybe @clayborg can chime in here. Although it was a while ago.

[shafik]

In D70846#1763731, @labath wrote:

There's lldb-shell :: SymbolFile/DWARF/find-basic-function.cpp, which probably didn't get run for you as you didn't have lld enabled (cmake -DLLVM_ENABLE_PROJECTS=...;lld). You'll need to update that test to match the new behavior, but other than that, I think this is a good change.

So with this change for the find-basic-function.cpp test I no longer see any results for the full case so we should at least generate a case that has results for the full case.

Based on the logic in ManualDWARFIndex::IndexUnitImpl I would expect it return bar::foo().

[labath]

In D70846#1766204, @shafik wrote:

In D70846#1763731, @labath wrote:

There's lldb-shell :: SymbolFile/DWARF/find-basic-function.cpp, which probably didn't get run for you as you didn't have lld enabled (cmake -DLLVM_ENABLE_PROJECTS=...;lld). You'll need to update that test to match the new behavior, but other than that, I think this is a good change.

So with this change for the find-basic-function.cpp test I no longer see any results for the full case so we should at least generate a case that has results for the full case.

There's an additional check in that test which does a search by a mangled name (search for FULL-MANGLED), and this one does return some result. If this patch lands, I'm not sure if there's any other kind of a "full" lookup that we could perform. eFunctionNameTypeFull is documented as: ... For C this is the same as just the name of the function For C++ this is the mangled or demangled version of the mangled name..., which appears like we should support searching by *de*mangled names. However, I'm not sure if that is actually a good idea. Implementing that for the manual index would be simple enough, but that is something that the apple index could never support (in fact, I think I remember that the manual index once supported searching by demangled names, but then I removed this ability for consistency when adding debug_names support).

That said, I think it may be interesting to add a test searching for an extern "C" symbol (which has no "mangled" name), as right now it's not clear if it will show up because of function_fullnames.Find or function_basenames.Find...

[shafik]

In D70846#1766598, @labath wrote:

In D70846#1766204, @shafik wrote:

In D70846#1763731, @labath wrote:

There's lldb-shell :: SymbolFile/DWARF/find-basic-function.cpp, which probably didn't get run for you as you didn't have lld enabled (cmake -DLLVM_ENABLE_PROJECTS=...;lld). You'll need to update that test to match the new behavior, but other than that, I think this is a good change.

So with this change for the find-basic-function.cpp test I no longer see any results for the full case so we should at least generate a case that has results for the full case.

There's an additional check in that test which does a search by a mangled name (search for FULL-MANGLED), and this one does return some result. If this patch lands, I'm not sure if there's any other kind of a "full" lookup that we could perform. eFunctionNameTypeFull is documented as: ... For C this is the same as just the name of the function For C++ this is the mangled or demangled version of the mangled name..., which appears like we should support searching by *de*mangled names. However, I'm not sure if that is actually a good idea. Implementing that for the manual index would be simple enough, but that is something that the apple index could never support (in fact, I think I remember that the manual index once supported searching by demangled names, but then I removed this ability for consistency when adding debug_names support).

That said, I think it may be interesting to add a test searching for an extern "C" symbol (which has no "mangled" name), as right now it's not clear if it will show up because of function_fullnames.Find or function_basenames.Find...

I did see that and I was just a little confused by the mixed results I was seeing.

I had put together a few test programs to better understand how it works e.g.:

namespace A {
  int foo() {
      return 2;
  }
}

int main() {
 return A::foo() ;
}

and when I run lldb-test symbols --name=foo --find=function --function-flags=full function_full.o I see:

Module: function_full.o
Found 1 functions:
0x00007ffee5ad3438: SymbolContextList
       Module: file = "function_full.o", arch = "x86_64"
  CompileUnit: id = {0xffffffff00000000}, file = "/Users/shafik/code/function_full.cpp", language = "c++"
     Function: id = {0xffffffff0000002f}, name = "A::foo()", mangled = "_ZN1A3fooEv", range = function_full.o[0x0000000000000000-0x000000000000000b)
     FuncType: id = {0xffffffff0000002f}, byte-size = 0, decl = /Users/shafik/code/function_full.cpp:2, compiler_type = "int (void)"

which seems inconsistent with what I am seeing with find-basic-function.cpp or at least it is not obvious to me what the difference is.

I think at least adding a test case for the extern "C" case would be good as well.

[labath]

I guess you were building darwin binaries, right? If that's the case, then you weren't using this code at all, as apple uses AppleIndex by default. The test in question uses all three indexes (it forces their generation by altering compile flags) and the failures you were seeing were most likely coming the "manual" case...

[shafik]

In D70846#1767455, @labath wrote:

I guess you were building darwin binaries, right? If that's the case, then you weren't using this code at all, as apple uses AppleIndex by default. The test in question uses all three indexes (it forces their generation by altering compile flags) and the failures you were seeing were most likely coming the "manual" case...

Correct, so I understand now. So with -accel-tables=Disable we will get less results.

[PatriosTheGreat]

It seems like with this CL find-basic-functions logic became inconsistent for macos target and pc-linux.
In cases of pc-linux it returns no function with --function-flags=full flag and in macos it returns 7 functions.

I think there are 3 ways to fix this:

1. Fix the find-basic-functions by splitting checks for mac and pc.
2. Make mac logic same as linux. Fix this on mac GetFunctions method as well.
3. Revert to old logic for eFunctionNameTypeFull and add one more enum value like "eFunctionNameTypeFullMangled" which will return only function_fullnames and will be supported only in pc version.

Which are the most consistent way for you?

[labath]

I wouldn't want to do (3).

I think the ideal solution would be (2). AppleIndex should also be able to do this kind of filtering, only it'd have to be done differently -- after looking up the name in the accelerator table, it would go to the debug info, and check what exactly does that name refer to. It wouldn't be as efficient as manual index, but it would still be better than doing this filtering somewhere down the pipeline (as it happens now), after we burn more cycles processing entries which are going to be ignored.

However, I think (1) would be also acceptable, particularly if (2) is hard for some reason.

[labath]

This looks fine to me. @shafik, @teemperor, do you have any more comments?

[shafik]

In D70846#1809837, @labath wrote:

This looks fine to me. @shafik, @teemperor, do you have any more comments?

Just a minor comment.

lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp

67 ↗

(On Diff #234899)

You don't actually run a FULL test, is this purposeful?

[PatriosTheGreat]

lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp

67 ↗

(On Diff #234899)

FULL tests runs for target=x86_64-pc-linux.
It runs in lines 10 and 39 in this file, If I'm not mistaken.

[PatriosTheGreat]

Made clang-format with last update of diff.

[labath]

Committed as a705cf1ac. It should be worth noting that this actually fixes TestPrintf.py (pr36715), because we no longer end up calling the member printf function. So doing something similar for the apple index will not only improve speed, but also correctness...

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

if (namespace_decl && module_sp) {
     module_sp->FindFunctions(name, &namespace_decl, eFunctionNameTypeBase,
                              include_symbols, include_inlines, sc_list);
   } else if (target && !namespace_decl) {
     const bool include_symbols = true;
 
     // TODO Fix FindFunctions so that it doesn't return
     //   instance methods for eFunctionNameTypeBase.
 
-    target->GetImages().FindFunctions(name, eFunctionNameTypeFull,
+    target->GetImages().FindFunctions(name, eFunctionNameTypeFull | eFunctionNameTypeBase,
                                       include_symbols, include_inlines,
                                       sc_list);
   }
 
   // If we found more than one function, see if we can use the frame's decl
   // context to remove functions that are shadowed by other functions which
   // match in type but are nearer in scope.
   //

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

 void ManualDWARFIndex::GetFunctions(ConstString name, SymbolFileDWARF &dwarf,
                                     const CompilerDeclContext &parent_decl_ctx,
                                     uint32_t name_type_mask,
                                     std::vector<DWARFDIE> &dies) {
   Index();
 
   if (name_type_mask & eFunctionNameTypeFull) {
     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 = dwarf.GetDIE(die_ref);
       if (!die)
         continue;
       if (SymbolFileDWARF::DIEInDeclContext(&parent_decl_ctx, die))
         dies.push_back(die);
     }