This is an archive of the discontinued LLVM Phabricator instance.

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

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
43–44

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
43–44

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
43–44

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
43–44

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
teemperor accepted this revision.Aug 4 2021, 3:42 AM

From what I can see the underlying problem with the import-std-module tests has been fixed at some point in the past (at least it doesn't reproduce anymore). I don't recall seeing a fix for the related issue, but maybe a libc++ change just moved things around so that we no longer try to call the wrong function. I will try to reland this and if the bots are happy then this seems good.

I wish I could say I finally got through my TODO list far enough to end up back here, but we actually do have a bug that is fixed by this patch. I'll add the respective tests as follow-up to the reland.

teemperor added inline comments.Aug 4 2021, 4:16 AM
lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
44

FWIW, I believe neither the old nor the new version of this check are fully correct (but this version is at least much closer to the intended functionality).

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. For ObjC this is
///< the full function signature with the + or - and the square brackets and
///< the class and selector

The C++ mangled name case is clearly covered. The C and Obj-C are covered because of the (rather interesting...) property for GetMangledName to return the DW_AT_NAME (which is the demangled name for C/Obj-C) when the mangled name is not specified in DWARF. But the demangled C++ name case isn't covered unless I'm missing something.

jarin added a comment.Aug 4 2021, 6:22 AM

Thanks for getting back to this, Raphael!