[lldb-dev] What should SymbolFile::FindFunctions(..., eFunctionNameTypeFull, ...) do ?

[Pavel Labath via lldb-dev]

Thank you for the detailed response. My replies are below.


On Thu, 3 May 2018 at 16:53, Greg Clayton <clayborg at gmail.com> wrote:
> On May 3, 2018, at 7:38 AM, Pavel Labath via lldb-dev <
lldb-dev at lists.llvm.org> wrote:
> - for the manual case (SymbolFileDWARF.cpp:2626), the function will look
> into a special "full name" index, which contains mangled and
> fully-qualified (with parameters and all) demangled names of all
functions.
> This would seem reasonable if it was not followed by a
hac^H^H^Hworkaround,
> which will, in case the previous search finds no match, look in the
> *basename* index, and then accept any function whose demangled name begins
> with the string "(anonymous namespace)" (which means it will include also
> functions with mismatched arguments types or namespace qualifiers).


> So I would have the ::Index method skip creating the fully qualified
names and populating an index based off of that because:
> 1 - it is expensive to create the qualified names and no one looks them
up that way.
> 2 - the qualified names would need to ensure that they exactly match what
the demangler would do if a mangled name were actually there
Agreed. I'd like to get rid of that as well.




> So, what should be the correct behavior here? Both of these seem so wrong
> (and different) that I am surprised that we did not run into issues here
> before. Is it possible there is some additional filtering happening at a
> different level that I am not aware of?


> So I believe the best way to proceed is the way the apple tables do
things. Expect that lookups will happen on base names and filtering will
happen elsewhere. This keeps the support needed for indexing to an
acceptable minimum for all debug info formats and will still allow people
to look things up.

Great, I was hoping you would propose that. :) Getting rid of the demangled
names should save us some memory and processing time, and will align the
manual-index behavior with the apple tables then any kind of post-filtering
we need to do can be done the same way regardless of how we obtained the
unfiltered list. I'll create a patch for that.



> PS: I tried adding assert(!name.contains("::")) into this function to see
> how often we are calling this function with a "FQN" which is not simply a
> basename. (This is interesting because for the apple case, this function
> would always return 0 results for such queries.) Only 5 tests failed, and
> in all of these, the asserting calls were originating from
> IRExecutionUnit.cpp, which was trying to link the jitted expression with
> the running program. What this code is doing is it takes a mangled name,
> and then tries searching for functions with several names derived from it
> (demangled names, the "const" version of the mangled name, etc.). So it
> seems, that in this case, the value returned by FindFunctions for the
> non-demangled names doesn't really matter, since IRExecutionUnit will just
> retry the search with the mangled name, which will return the same answer
> in both cases.


> I agree that for the expression parser what you noticed with the assert
is ok. The real issue is when people don't fully qualify names they type
(such as with "b::foo" mentioned above), and that means function
breakpoints by name are the other main searching factor here.

Is it really OK? If our indexes will never contain the demangled names,
then the IRExecutionUnit lookups using the demangled names will always
fail. (Right now they will only succeed for manually indexed dwarf, but
this will change if I stop putting these names in the full index) Shouldn't
we fix the IRExecutionUnit to not attempt these lookups in the first place?



> So things we need to think about:
> - do we make SymbolFile interfaces simpler and make the filtering logic
at a high level, or do we increase the complexity of these searches so they
return fewer results and have each SymbolFile::FindXXXX() function more
complex.
> - do we agnostify the CompilerDeclContext so it works on any type system,
or require the find function that searches multiple modules have to create
or lookup a valid CompilerDeclContext prior to calling into each
SymbolFile::FindXXX() call?

These are very good questions, but I am afraid I don't know enough about
this part of the codebase to say what would be best. I think that a
declarative method of specifying the context would be better than a
callback-based, because then the search can be optimized better (e.g. for
DWARF 5 indexes, knowing the offset of the compile unit we are searching
can make the searches much faster, but I'm not sure about the details. It
looks like I will be digging in this part of the code for a while now, so I
may get a better idea of how it is used...