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

[jarin]

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

[labath]

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 to include all methods/non-methods with matching mangled name.

[jarin]

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

[labath]

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]

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]

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]

Only matching the mangled name now.

[jarin]

Pavel, could you take another look, please?

[labath]

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

45

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

[labath]

(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]

In D73191#1841372, @labath wrote:

(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?

[labath]

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]

Reopening for further investigation.

[teemperor]

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]

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

43

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]

Thanks for getting back to this, Raphael!

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

void DWARFIndex::ProcessFunctionDIE(llvm::StringRef name, DIERef ref,
   if (!looking_for_nonmethods && parent_decl_ctx.IsValid())
     return;
 
   // Otherwise, we need to also check that the context matches. If it does not
   // match, we do nothing.
   if (!SymbolFileDWARF::DIEInDeclContext(&parent_decl_ctx, die))
     return;
 
-  // In case of a full match, we just insert everything we find.
-  if (name_type_mask & eFunctionNameTypeFull) {
+  // In case of a full match, we insert all non-methods we find.
+  if (name_type_mask & eFunctionNameTypeFull && !die.IsMethod()) {
     dies.push_back(die);
     return;
   }
 
   // If looking for ObjC selectors, we need to also check if the name is a
   // possible selector.
   if (name_type_mask & eFunctionNameTypeSelector &&
       ObjCLanguage::IsPossibleObjCMethodName(die.GetName())) {
     dies.push_back(die);
     return;

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

 // BASE-DAG: name = "bar::foo()", mangled = "_ZN3bar3fooEv"
 // BASE-DAG: name = "bar::baz::foo()", mangled = "_ZN3bar3baz3fooEv"
 
 // METHOD: Found 3 functions:
 // METHOD-DAG: name = "sbar::foo()", mangled = "_ZN4sbar3fooEv"
 // METHOD-DAG: name = "sbar::foo(int)", mangled = "_ZN4sbar3fooEi"
 // METHOD-DAG: name = "ffbar()::sbaz::foo()", mangled = "_ZZ5ffbarvEN4sbaz3fooEv"
 
-// FULL-INDEXED: Found 7 functions:
+// FULL-INDEXED: Found 4 functions:
 // FULL-INDEXED-DAG: name = "foo()", mangled = "_Z3foov"
 // FULL-INDEXED-DAG: name = "foo(int)", mangled = "_Z3fooi"
 // FULL-INDEXED-DAG: name = "bar::foo()", mangled = "_ZN3bar3fooEv"
 // FULL-INDEXED-DAG: name = "bar::baz::foo()", mangled = "_ZN3bar3baz3fooEv"
-// FULL-INDEXED-DAG: name = "sbar::foo()", mangled = "_ZN4sbar3fooEv"
-// FULL-INDEXED-DAG: name = "sbar::foo(int)", mangled = "_ZN4sbar3fooEi"
-// FULL-INDEXED-DAG: name = "ffbar()::sbaz::foo()", mangled = "_ZZ5ffbarvEN4sbaz3fooEv"
 
 // FULL: Found 0 functions:
 
 // FULL-MANGLED: Found 1 functions:
 // FULL-MANGLED-DAG: name = "foo(int)", mangled = "_Z3fooi"
 
 // CONTEXT: Found 1 functions:
 // CONTEXT-DAG: name = "bar::foo()", mangled = "_ZN3bar3fooEv"