[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

[Pavel Labath via Phabricator via lldb-commits]

labath closed this revision.
labath added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70846/new/

https://reviews.llvm.org/D70846



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

[Levon Ter-Grigoryan via Phabricator via lldb-commits]

PatriosTheGreat updated this revision to Diff 237604.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70846/new/

https://reviews.llvm.org/D70846

Files:
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
  lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp


Index: lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp
===
--- lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp
+++ lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp
@@ -21,7 +21,7 @@
 // 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:   FileCheck --check-prefix=FULL-APPLE %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 | \
@@ -55,14 +55,16 @@
 // METHOD-DAG: name = "sbar::foo(int)", mangled = "_ZN4sbar3fooEi"
 // METHOD-DAG: name = "ffbar()::sbaz::foo()", mangled = 
"_ZZ5ffbarvEN4sbaz3fooEv"
 
-// 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()::sbaz::foo()", mangled = "_ZZ5ffbarvEN4sbaz3fooEv"
+// FULL-APPLE: Found 7 functions:
+// FULL-APPLE-DAG: name = "foo()", mangled = "_Z3foov"
+// FULL-APPLE-DAG: name = "foo(int)", mangled = "_Z3fooi"
+// FULL-APPLE-DAG: name = "bar::foo()", mangled = "_ZN3bar3fooEv"
+// FULL-APPLE-DAG: name = "bar::baz::foo()", mangled = "_ZN3bar3baz3fooEv"
+// FULL-APPLE-DAG: name = "sbar::foo()", mangled = "_ZN4sbar3fooEv"
+// FULL-APPLE-DAG: name = "sbar::foo(int)", mangled = "_ZN4sbar3fooEi"
+// FULL-APPLE-DAG: name = "ffbar()::sbaz::foo()", mangled = 
"_ZZ5ffbarvEN4sbaz3fooEv"
+
+// FULL: Found 0 functions:
 
 // FULL-MANGLED: Found 1 functions:
 // FULL-MANGLED-DAG: name = "foo(int)", mangled = "_Z3fooi"
Index: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -401,8 +401,6 @@
 
   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);
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
@@ -1254,9 +1254,9 @@
 // TODO Fix FindFunctions so that it doesn't return
 //   instance methods for eFunctionNameTypeBase.
 
-target->GetImages().FindFunctions(name, eFunctionNameTypeFull,
-  include_symbols, include_inlines,
-  sc_list);
+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


Index: lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp
===
--- lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp
+++ lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp
@@ -21,7 +21,7 @@
 // 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:   FileCheck --check-prefix=FULL-APPLE %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 | \
@@ -55,14 +55,16 @@
 // METHOD-DAG: name = "sbar::foo(int)", mangled = "_ZN4sbar3fooEi"
 // METHOD-DAG: name = "ffbar()::sbaz::foo()", mangled = "_ZZ5ffbarvEN4sbaz3fooEv"
 
-// FULL: Found 7 functions:
-// FULL-DAG: name = "foo()", mangled = "_Z3foov"

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

[Levon Ter-Grigoryan via Phabricator via lldb-commits]

PatriosTheGreat added a comment.

Made clang-format with last update of diff.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70846/new/

https://reviews.llvm.org/D70846



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

[Levon Ter-Grigoryan via Phabricator via lldb-commits]

PatriosTheGreat marked an inline comment as done.
PatriosTheGreat added inline comments.



Comment at: lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp:67
+
+// FULL: Found 0 functions:
 

shafik wrote:
> You don't actually run a `FULL` test, is this purposeful? 
FULL tests runs for target=x86_64-pc-linux. 
It runs in lines 10 and 39 in this file, If I'm not mistaken.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70846/new/

https://reviews.llvm.org/D70846



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

In D70846#1809837 , @labath wrote:

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


Just a minor comment.




Comment at: lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp:67
+
+// FULL: Found 0 functions:
 

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70846/new/

https://reviews.llvm.org/D70846



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70846/new/

https://reviews.llvm.org/D70846



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

[Levon Ter-Grigoryan via Phabricator via lldb-commits]

PatriosTheGreat updated this revision to Diff 234899.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70846/new/

https://reviews.llvm.org/D70846

Files:
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
  lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp


Index: lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp
===
--- lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp
+++ lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp
@@ -21,7 +21,7 @@
 // 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:   FileCheck --check-prefix=FULL-APPLE %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 | \
@@ -55,14 +55,16 @@
 // METHOD-DAG: name = "sbar::foo(int)", mangled = "_ZN4sbar3fooEi"
 // METHOD-DAG: name = "ffbar()::sbaz::foo()", mangled = 
"_ZZ5ffbarvEN4sbaz3fooEv"
 
-// 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()::sbaz::foo()", mangled = "_ZZ5ffbarvEN4sbaz3fooEv"
+// FULL-APPLE: Found 7 functions:
+// FULL-APPLE-DAG: name = "foo()", mangled = "_Z3foov"
+// FULL-APPLE-DAG: name = "foo(int)", mangled = "_Z3fooi"
+// FULL-APPLE-DAG: name = "bar::foo()", mangled = "_ZN3bar3fooEv"
+// FULL-APPLE-DAG: name = "bar::baz::foo()", mangled = "_ZN3bar3baz3fooEv"
+// FULL-APPLE-DAG: name = "sbar::foo()", mangled = "_ZN4sbar3fooEv"
+// FULL-APPLE-DAG: name = "sbar::foo(int)", mangled = "_ZN4sbar3fooEi"
+// FULL-APPLE-DAG: name = "ffbar()::sbaz::foo()", mangled = 
"_ZZ5ffbarvEN4sbaz3fooEv"
+
+// FULL: Found 0 functions:
 
 // FULL-MANGLED: Found 1 functions:
 // FULL-MANGLED-DAG: name = "foo(int)", mangled = "_Z3fooi"
Index: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -401,8 +401,6 @@
 
   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);
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
@@ -1273,7 +1273,7 @@
 // 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);
   }


Index: lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp
===
--- lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp
+++ lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp
@@ -21,7 +21,7 @@
 // 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:   FileCheck --check-prefix=FULL-APPLE %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 | \
@@ -55,14 +55,16 @@
 // METHOD-DAG: name = "sbar::foo(int)", mangled = "_ZN4sbar3fooEi"
 // METHOD-DAG: name = "ffbar()::sbaz::foo()", mangled = "_ZZ5ffbarvEN4sbaz3fooEv"
 
-// 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:

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70846/new/

https://reviews.llvm.org/D70846



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

[Levon Ter-Grigoryan via Phabricator via lldb-commits]

PatriosTheGreat added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70846/new/

https://reviews.llvm.org/D70846



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70846/new/

https://reviews.llvm.org/D70846



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70846/new/

https://reviews.llvm.org/D70846



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

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:
  0x7ffee5ad3438: SymbolContextList
 Module: file = "function_full.o", arch = "x86_64"
CompileUnit: id = {0x}, file = 
"/Users/shafik/code/function_full.cpp", language = "c++"
   Function: id = {0x002f}, name = "A::foo()", mangled = 
"_ZN1A3fooEv", range = function_full.o[0x-0x000b)
   FuncType: id = {0x002f}, 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70846/new/

https://reviews.llvm.org/D70846



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70846/new/

https://reviews.llvm.org/D70846



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70846/new/

https://reviews.llvm.org/D70846



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added subscribers: clayborg, shafik.
shafik added a comment.

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.




Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:1213
 
-target->GetImages().FindFunctions(name, eFunctionNameTypeFull,
+target->GetImages().FindFunctions(name, eFunctionNameTypeFull | 
eFunctionNameTypeBase,
   include_symbols, include_inlines,

I did some archeology here and this was changed from `eFunctionNameTypeBase` to 
`eFunctionNameTypeFull` by [this 
change](https://github.com/llvm/llvm-project/commit/43fe217b119998264be04a773c4770592663c306)
 so maybe @clayborg can chime in here. Although it was a while ago.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70846/new/

https://reviews.llvm.org/D70846



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70846/new/

https://reviews.llvm.org/D70846



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70846/new/

https://reviews.llvm.org/D70846



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

[Levon Ter-Grigoryan via Phabricator via lldb-commits]

PatriosTheGreat created this revision.
PatriosTheGreat added reviewers: labath, jarin, aprantl.
PatriosTheGreat added a project: LLDB.
Herald added a subscriber: arphaman.

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?


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70846

Files:
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp


Index: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -401,8 +401,6 @@
 
   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);
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
@@ -1210,7 +1210,7 @@
 // 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);
   }


Index: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -401,8 +401,6 @@
 
   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);
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
@@ -1210,7 +1210,7 @@
 // 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);
   }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits