[Lldb-commits] [PATCH] D73191: Only match mangled name in full-name function lookup (with accelerators)

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp:45
   if (name_type_mask & eFunctionNameTypeFull) {
-    dies.push_back(die);
-    return;
+    if (!die.IsMethod() || die.GetMangledName(false) == name) {
+      dies.push_back(die);
----------------
jarin wrote:
> jarin wrote:
> > labath wrote:
> > > 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?
> > I want to avoid returning methods to [[ https://github.com/llvm/llvm-project/blob/808142876c10b52e7ee57cdc6dcf0acc5c97c1b7/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp#L1258 | ClangExpressionDeclMap::LookupFunction ]], so that ClangExpressionDeclMap::LookupFunction does not do the expensive [[ https://github.com/llvm/llvm-project/blob/808142876c10b52e7ee57cdc6dcf0acc5c97c1b7/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp#L1298 | DeclContext parsing ]] just to find out the function [[ https://github.com/llvm/llvm-project/blob/808142876c10b52e7ee57cdc6dcf0acc5c97c1b7/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp#L1304 | 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?
> 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.
Yes, that's exactly what I meant. :)


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

https://reviews.llvm.org/D73191