Page MenuHomePhabricator

DebugNamesDWARFIndex: Add ability to lookup variables
ClosedPublic

Authored by labath on Jun 5 2018, 6:42 AM.

Details

Summary

This adds the ability to lookup variables to the DWARF v5 index class.
find-basic-variable.cpp test is extended to cover this scenario as well,
and I have added a new test which verifies that the dwarf v5 index class
is indeed used.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Jun 5 2018, 6:42 AM
clayborg requested changes to this revision.Jun 5 2018, 10:21 AM

Check the context as noted in inline comments.

source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
70 ↗(On Diff #149967)

Need to check for the context matches here if context is not empty.

source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
14 ↗(On Diff #149967)

move to .cpp file?

This revision now requires changes to proceed.Jun 5 2018, 10:21 AM
labath added inline comments.Jun 5 2018, 12:38 PM
source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
70 ↗(On Diff #149967)

Hmm... thanks for pointing this out. There is some index-independent filtering based on CompilerDeclContexts happening in SymbolFileDWARF (which is why my context test actually passes here).

I was basing this off of how apple and manual indexes work, and neither of them check the context. In fact, the manual index does not even extract the identifier portion of the name (it just looks up the whole string), so maybe nobody passes qualified names names into this function anyway? I am starting to think I should just remove the ExtractContextAndIdentifier call. If we see failures down the line, at least we will learn why was this call there in the first place.

Feel free to check into it and modify the contract for GetGlobalVariables and possibly rename "name" to "basename" to be clear what the argument is. Then modify the headerdoc to clearly state what we are looking for if the filtering is going on higher up

labath updated this revision to Diff 150149.Jun 6 2018, 8:51 AM

Ok, so it turns out we were passing qualified names into the
GetGlobalVariables function. In the apple-tables case we were then searching
based on the basename and completely ignoring the context. This resulted in
incorrect matches being returned (I believe this is the bug Jonas is
investigating right now). In the manual-index case, we weren't doing the search
based on the basename, but things worked because our index also contained
demangled names.

This version of the patch does the following:

  • remove context handling from the index classes. The GetGlobalVariables function now takes a basename like you suggested.
  • in particular, this means that the manual index can now stop storing demangled names of global variables. This makes it consistent with the other indexes and with the function case, where we removed demangled names a couple of weeks ago.
  • puts the context extraction into SymbolFileDWARF, so that it is available to all indexes.
  • implements context filtering ("fixing" Jonas's bug), also in SymbolFileDWARF. I am not particularly proud of how this is implemented (substring search), but this is the same way that function context pruning is done (cf. Module::LookupInfo::Prune). A more sensible framework for context handling would be in order, but I consider that out of scope for this patch.
clayborg accepted this revision.Jun 6 2018, 9:48 AM

Much better. Anything we can do to clearly define and simplify the indexers is great stuff.

This revision is now accepted and ready to land.Jun 6 2018, 9:48 AM
This revision was automatically updated to reflect the committed changes.