Page MenuHomePhabricator

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

Authored by PatriosTheGreat on Fri, Nov 29, 3:11 AM.

Details

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?

Diff Detail

Event Timeline

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.

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).

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 added a comment.EditedMon, Dec 2, 4:18 PM

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 added a comment.Tue, Dec 3, 1:37 AM

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 added a comment.Tue, Dec 3, 9:48 AM

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.

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 added a comment.EditedWed, Dec 4, 1:58 PM

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.

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?

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.