Page MenuHomePhabricator

Only match mangled name in full-name function lookup (with accelerators)
AcceptedPublic

Authored by jarin on Jan 22 2020, 6:52 AM.

Details

Summary

In the spirit of https://reviews.llvm.org/D70846, we only return functions with matching mangled name from Apple/DebugNamesDWARFIndex::GetFunction if eFunctionNameTypeFull is requested.

This speeds up lookup in the presence of large amount of class methods of the same name (a typical examples would be constructors of templates with many instantiations or overloaded operators).

Diff Detail

Event Timeline

jarin created this revision.Jan 22 2020, 6:52 AM

This doesn't sound right. Will that mean we won't be able to find a method even if we search for it by its proper full mangled name (e.g. _ZZ5ffbarvEN4sbaz3fooEv in this case)?

A better heuristic might be to check for DW_AT_linkage_name, and if the DIE has it, then only accept the DIE if the linkage name matches the search query exactly. (We still need to let the DIEs without a linkage name through because extern "C" functions don't have those).

jarin updated this revision to Diff 240144.Jan 24 2020, 3:25 AM
jarin edited the summary of this revision. (Show Details)

Updated to include all methods/non-methods with matching mangled name.

jarin added a comment.Jan 24 2020, 3:26 AM

Pavel, does the latest patch address (and test) what you were worried about?

jarin set the repository for this revision to rG LLVM Github Monorepo.Jan 24 2020, 3:42 AM
labath added inline comments.Jan 24 2020, 4:28 AM
lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
45

I don't believe the IsMethod check is really needed here -- the mangled name check should handle everything.

In fact, looking at the implementation of DWARFDebugInfoEntry::GetMangledName, I don't think you even need the extra substitute_name_allowed=false part. The default value should do exactly what we need. If the DIE has a linkage (mangled) name it will return it and we will use that for comparison. For an extern "C" function it will return the regular name, and we will compare that instead (this check is somewhat redundant because if the name doesn't match, the function should not be in the index in the first place, but I don't think it hurts to check either).

Am I missing something?

jarin marked an inline comment as done.Jan 24 2020, 6:22 AM
jarin added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
45

I want to avoid returning methods to ClangExpressionDeclMap::LookupFunction, so that ClangExpressionDeclMap::LookupFunction does not do the expensive DeclContext parsing just to find out the function is a method and must be thrown away.

The motivation is pretty much the same as for https://reviews.llvm.org/D70846.

Does it make sense?

jarin marked an inline comment as done.Jan 26 2020, 11:59 PM
jarin added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
45

I think I see what you are saying - in the test case below, FULL should be consistent with FULL-INDEXED. Let me remove the IsMethod check and merge the FULL and FULL-INDEXED test cases below.

jarin updated this revision to Diff 240480.Jan 27 2020, 12:34 AM
jarin retitled this revision from Ignore methods in full-name function lookup (with accelerators) to Only match mangled name in full-name function lookup (with accelerators).
jarin edited the summary of this revision. (Show Details)

Only matching the mangled name now.

Pavel, could you take another look, please?

labath accepted this revision.Jan 27 2020, 1:24 AM
labath added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
45

Yes, that's exactly what I meant. :)

This revision is now accepted and ready to land.Jan 27 2020, 1:24 AM

(I mean, if there is a real benefit to having some queries return only non-methods, then we can certainly do something like that as well, but that should be handled differently -- either we can create a new query mode, or change the behavior (and name?) of eFunctionNameTypeFull across the board).

jarin added a comment.Jan 27 2020, 2:01 AM

(I mean, if there is a real benefit to having some queries return only non-methods, then we can certainly do something like that as well, but that should be handled differently -- either we can create a new query mode, or change the behavior (and name?) of eFunctionNameTypeFull across the board).

No, I was confused; this simpler solution works for us just fine. I agree with you and I like that the indexed case is the same as the manual-index one now.

If you are happy with the patch, could you land it for me?

This revision was automatically updated to reflect the committed changes.

I've had to revert this because of some failures on macos (http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/7109/testReport/). @jarin, are you able to run the test suite on a mac to investigate.

@teemperor, all of the failures (except TestPrintf, which was actually fixed) were in the "import std module" tests, relating to the failure to lookup symbols like __Z12__to_addressP1C. I have a feeling this could actually be a bug in the std module handler -- I know it does something special with symbols in the std namespace, but this looks like a standard library symbol which is not in that namespace (and it's not surprising we can't find it in the binary if the program never used it). What do you think is the best way to resolve this?

jarin reopened this revision.May 5 2020, 12:43 PM

Reopening for further investigation.

This revision is now accepted and ready to land.May 5 2020, 12:43 PM