bpftrace icon indicating copy to clipboard operation
bpftrace copied to clipboard

Parse C++ class and inheritance from Debug Info

Open ttreyer opened this issue 10 months ago • 2 comments

Extend the Dwarf parser to make class members and parents' members accessible.

This patch handles any shape of inheritance hierarchy, but does provide access to parents' shadowed members:

struct Parent { int abc, xyz; };
struct Child : Parent { int abc; };
...
child->abc          // Access the child's member
child->xyz          // Access the parent's member
child->Parent::abc  // Not implemented: access the parent's shadowed member

Multiple inheritance suffer from a similar problem: when a parent shadows the member of another parent.

struct GrandChild : Parent, Child {};
g_child->xyz // Access Parent's member
g_child->abc // Ambiguous! Will access Parent::abc, but it should reject and require a qualified name instead
Checklist
  • [ ] Language changes are updated in man/adoc/bpftrace.adoc
  • [ ] User-visible and non-trivial changes updated in CHANGELOG.md
  • [x] The new behaviour is covered by tests

ttreyer avatar Mar 28 '24 17:03 ttreyer

Btw, I like how much simpler your method is for generating a DWARF data source - maybe we could switch the C data source across to that way in the future?

ajor avatar Jun 07 '24 13:06 ajor

Btw, I like how much simpler your method is for generating a DWARF data source - maybe we could switch the C data source across to that way in the future?

We might be able to simplify it, but the C data source has the extra requirement of generating BTF information, which is not supported for C++.

ttreyer avatar Jun 12 '24 16:06 ttreyer

Also, the individual commits are not very clean and very likely won't work on their own. I'm ok with that as long as we squash the PR in the end.

Yep, we'll squash before merging. That's the approach I think we should be taking with the vast majority of PRs anyway.

ajor avatar Jul 11 '24 10:07 ajor

To save me a bit of time reviewing: what was the reason for the recent changes to dwarf_parser, making resolve_fields() take a shared_ptr<Struct>?

The new resolve_fields() that takes the shared_ptr is a private method that populates the fields without needing to do any lookup on bpftrace_->structs or with LLDB's target.FindFirstType(). For record types, we already have the lldb::SBType, so we can populate the Struct without any lookup. The previous public version of resolve_fields() is still available and do the lookup, before calling the new resolve_fields().

Saving a lookup is nice, but I also had a problem with the type name: I'm currently using GetDisplayName() which provides a user friendly name (std::vector<int>), but there is also GetName() which returns the full name of the type (std::vector<int, std::allocator<int>>). LLDB needs the full name to lookup for a type by its name, so another fix could be to use the full name instead of the display name. However, the current approach with two resolve_fields() function solves the lookup issue I had, while preserving a user friendly type name.

Which name should we display to the user? The full one for completeness? Or the display name for friendliness?

ttreyer avatar Jul 11 '24 14:07 ttreyer