This is an archive of the discontinued LLVM Phabricator instance.

DWARFIndex: Reduce duplication in the GetFunctions methods
ClosedPublic

Authored by labath on May 21 2018, 9:27 AM.

Details

Summary

This extracts the common bits out of the two implementations back into
the SymbolFileDWARF class. The goal was to make the function as similar
as possible to the other DWARFIndex methods (i.e, to restrict the work
of the index classes to DIE offset manipulation only, and leave the
offset interpretation to the caller).

To achieve this, i've split the function into three functions, for
looking up:

  • full names
  • objc methods
  • methods and functions

The first two are trivial, and they just extract DIE offsets into an
array. The last one needed a slightly more complex interface, because of
the differences in how the two indexes operate -- manual index is able
to differentiate between methods and non-methods, while the apple table
is not. For this reason, the function returns three offset arrays
instead of just one. The first two are for DIEs which we know are/are
not methods, and the third one is for those we are unsure. In this case,
the caller needs to do extra work to tell them apart.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.May 21 2018, 9:27 AM

Before I look too closely, we can probably add a DWARFDIE accessor function like:

bool DWARFDIE::IsClassMethod() const;

Then this would allow each DWARFIndex to just check and we might be able to get rid of the 3 lists in GetFunctionsByBaseOrMethodName? Seems like it shouldn't be hard for the index to weed these things out. Or we can add a generic layer into the DWARFIndex base class that the different indexes can call to weed out the things that aren't wanted. The earlier we can weed things out the better. so the index might return 20 matches, but it would be nice to weed out any mismatches as soon as possible so we can avoid creating any clang AST types for the functions/methods if we don't need do.

The example I am thinking of is "search for all methods called 'erase'" where the index might just be able to lookup "erase" and get 20 matches, but then before we realize/make any clang AST types, we would quickly call "DWARFDIE::IsClassMethod()" on it to weed out any mismatches before we hand them back. Let me know if that makes sense or if I am missing anything. I would like the DWARFIndex class to be as dead simple as possible.

Changing the code to filter based on the dwarf information instead of the going through CompilerDeclContexts sounds like a good idea. I've been wondering why we are doing it this way -- the explanation I gave to myself was that this would allow the individual language plugins to decide what represents a "class" instead of just searching for DW_TAG_structure_type and similar. However, it's very likely that this is extra flexibility we don't need (and indeed, the manual index just checks the dwarf tags while building the index).

Unfortunately, something's come up, so I wasn't able to look into this closer today. Hopefully, I'll be able to get back to this patch later this week.

labath updated this revision to Diff 149702.Jun 4 2018, 3:33 AM

This is a rewrite of the original patch with the same idea but different
approach. Now that Apple index determines method-ness straight from the debug
info, we don't need to resolve the functions into SymbolContexts. This removes
the need for the bunch of callback arguments and allows us to pull the common
part out of the two implementations of these functions back into the
SymbolFileDWARF class.

clayborg accepted this revision.Jun 4 2018, 3:59 PM

Some if statements simplifications if you want to, but looks good.

source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
193–195 ↗(On Diff #149702)
if (ObjCLanguage::IsPossibleObjCMethodName(die_name))
  dies.push_back(die);
source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
448–451 ↗(On Diff #149702)
if (SymbolFileDWARF::DIEInDeclContext(&parent_decl_ctx, die))
  dies.push_back(die);
464 ↗(On Diff #149702)
if (die)
  dies.push_back(die);
474–477 ↗(On Diff #149702)
if (die)
  dies.push_back(die);
This revision is now accepted and ready to land.Jun 4 2018, 3:59 PM
This revision was automatically updated to reflect the committed changes.