This is an archive of the discontinued LLVM Phabricator instance.

AppleDWARFIndex: Get function method-ness directly from debug info
ClosedPublic

Authored by labath on May 29 2018, 3:58 AM.

Details

Summary

When searching for methods only, we need to do extra work to make sure
the functions we get from the apple tables are indeed methods.
Previously we were resolving the DIE into a SymbolContext and then
checked whether the enclosing CompilerDeclContext is a
class (or struct, or union).

This patch changes that to operate on the debug info directly. This
should be:

  • simpler
  • faster
  • more consistent with the ManualDWARFIndex (which does the same check, only at indexing time).

What we lose this ways is for the language plugin to have a say in what
it considers to be a "class", but that's probably more flexibility than
we need (and if we really wanted to do that in the future, we could
implement a more direct way to consult the plugin about this).

This also fixes the find-method-local-struct test, which was failing
because we were not able to construct a CompilerDeclContext for a local
struct correctly.

As a drive-by, I rename the DWARFDIE's IsStructClassOrUnion method to
match the name on the CompilerDeclContext class.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.May 29 2018, 3:58 AM
JDevlieghere accepted this revision.May 29 2018, 5:38 AM

Thanks Pavel, LGTM.

This revision is now accepted and ready to land.May 29 2018, 5:38 AM
clayborg added inline comments.May 29 2018, 9:16 AM
source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
153 ↗(On Diff #148879)

move up to line 150 and use this variable in the if statement instead of repeating "name_type_mask & eFunctionNameTypeMethod" twice?

source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
222 ↗(On Diff #148879)

I can never remember when a DW_AT_abstract_origin might be used. Might be nice to have a DWARFDIE method:

DWARFDIE DWARFDIE::GetAbstractOriginOrSpecification();

this would return either the the DW_AT_specification or the DW_AT_abstract_origin. If we go that route the this coce would become:

GetAbstractOriginOrSpecification().GetParent().IsStructUnionOrClass();
labath added inline comments.May 29 2018, 9:59 AM
source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
222 ↗(On Diff #148879)

How would this method know which DIE to return? In case of inlined methods you can have a chain of DIEs:
DIE1 --- DW_AT_abstract_origin --> DIE2 --- DW_AT_specification --> DIE3
Each of these dies will have a different parent.

The current function will check for the parent of DIE1 and DIE3 (this is what the manual index does) though to be fully correct maybe we should check all three of them (?) Or do you think checking the last DIE in that list should be always enough? That would seem to be the case for the dwarf I've seen, but I'm not sure if that is always correct..

clayborg added inline comments.May 29 2018, 10:54 AM
source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
222 ↗(On Diff #148879)

Yeah, unfortunately I can't elaborate too much without seeing a bad example. If anything in the chain has a parent that is a struct/union or class might be enough, so we could ask each DIE in the specification or abstract origin chain if its parent is a struct/union/class and just return true if so?

So my previous code suggestion might not work as we would want to recurse through the specification or abstract origin chain.

labath updated this revision to Diff 149253.May 31 2018, 2:38 AM

Implement recursive search in IsMethod()

labath marked 4 inline comments as done.May 31 2018, 2:41 AM
labath added inline comments.
source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
153 ↗(On Diff #148879)

Done. I've created variables for both enum values, as having just one of them in the if statement looked weird.

source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
222 ↗(On Diff #148879)

I've reimplemented the function to do a proper recursive search for the parent, including a infinite-recursion guard.

JDevlieghere added inline comments.May 31 2018, 3:04 AM
source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
222 ↗(On Diff #148879)

Looks good, we have almost the exact same implementation in findRecursively in llvm.

labath marked 4 inline comments as done.May 31 2018, 3:07 AM
labath added inline comments.
source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
222 ↗(On Diff #148879)

It should be. I copied it from there. :)

clayborg accepted this revision.May 31 2018, 10:34 AM

Very nice

After seeing this, it might be nice to put the recursion that follows the DW_AT_specification and DW_AT_abstract_origin and avoids infinite recursion into a DWARFDIE function that takes a callback:

typedef bool (*DIECallback)(DWARFDIE &die);
void DWARFDIE::VisitAbstractOriginsAndSpecifications(DIECallback callback);

The contents would be very similar to the DWARFDIE::IsMethod() function, but it would listen to the "bool" return value from calling the "callback" function and stop recursing if true or false is returned so we can stop the recursion if we find what we need. Then we can re-use this function for more

labath updated this revision to Diff 149423.Jun 1 2018, 3:19 AM
labath marked an inline comment as done.

I have a feeling we are starting to over-engineer this. Visiting referenced dies
sounds like a useful utility in general, though I think most of the users will
be content with just visiting the attributes of those dies (and that's a
functionality we already have). Nonetheless, I implemented something like that.
I made it an iterator-based api, as callback-based apis are generally more
complicated to use.

labath added a comment.Jun 1 2018, 3:22 AM

ps: the specificationsAndAbstractOrigins sounded like too big of a mouthful. I've invented the name "elaborating dies" for that.

clayborg accepted this revision.Jun 1 2018, 9:33 AM

I can't think of a better name, looks good. Thanks for taking the time, this will be great.

This revision was automatically updated to reflect the committed changes.