Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parse C class and inheritance from Debug Info #3094

Merged
merged 8 commits into from
Aug 14, 2024

Conversation

ttreyer
Copy link
Contributor

@ttreyer ttreyer commented Mar 28, 2024

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

This patch handles any shape of inheritance hierarchy, but doesn't 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
  • The new behaviour is covered by tests

tests/testprogs/uprobe_test_cxx.cpp Fixed Show fixed Hide fixed
tests/data/data_source_cxx.cpp Fixed Show fixed Hide fixed
tests/data/data_source_cxx.cpp Fixed Show fixed Hide fixed
tests/data/data_source_cxx.cpp Dismissed Show dismissed Hide dismissed
tests/data/data_source_cxx.cpp Fixed Show fixed Hide fixed
tests/data/data_source_cxx.cpp Dismissed Show dismissed Hide dismissed
@ttreyer ttreyer marked this pull request as draft April 9, 2024 13:44
@ttreyer ttreyer force-pushed the cxx-inheritance branch 3 times, most recently from ed523b6 to a6d232f Compare April 30, 2024 16:01
@ttreyer ttreyer force-pushed the cxx-inheritance branch 6 times, most recently from d61b5dd to 4580a6d Compare May 23, 2024 19:15
tests/testprogs/uprobe_test_cxx.cpp Fixed Show fixed Hide fixed
tests/testprogs/uprobe_test_cxx.cpp Fixed Show fixed Hide fixed
tests/testprogs/uprobe_test_cxx.cpp Dismissed Show dismissed Hide dismissed
@ttreyer ttreyer force-pushed the cxx-inheritance branch 3 times, most recently from 6316e7f to c45e5b1 Compare May 28, 2024 15:40
@ttreyer ttreyer marked this pull request as ready for review May 28, 2024 16:46
@ajor ajor self-assigned this Jun 7, 2024
Copy link
Member

@ajor ajor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to cause an abort with this script when it's triggered:

uprobe:./tests/testprogs/uprobe_test_cxx:cpp:uprobeFunction1 { print(args.foo) }

This change could do with some semantic analyser unit tests as well, to confirm the AST transformation occurs as expected.

tests/runtime/dwarf Outdated Show resolved Hide resolved
tests/runtime/dwarf Outdated Show resolved Hide resolved
tests/testprogs/uprobe_test_cxx.cpp Outdated Show resolved Hide resolved
src/ast/passes/semantic_analyser.cpp Outdated Show resolved Hide resolved
@ajor
Copy link
Member

ajor commented Jun 7, 2024

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?

@ttreyer ttreyer force-pushed the cxx-inheritance branch 8 times, most recently from 7c69caa to 7ee32d7 Compare July 30, 2024 13:20
@ttreyer ttreyer force-pushed the cxx-inheritance branch 4 times, most recently from 0af0130 to 5321c6c Compare August 7, 2024 15:58
Copy link
Member

@ajor ajor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only have one suggestion on functionality: it'd seem better to me to store the fields of structs in order of their offsets, even when they're inherited fields from parents.

e.g. for these types:

struct Base {
  int baseMember;
};
struct Child : public Base {
  int childMember;
};

Printing a Child type would currently display:

{ .childMember = 123, .baseMember = 456 }

But should probably display this instead:

{ .baseMember = 456, .childMember = 123 }

I'm imagining that this'll be an easy fix, but feel free to open up an issue and deal with it at a later date if not.

tests/utils.cpp Outdated Show resolved Hide resolved
tests/mocks.cpp Outdated Show resolved Hide resolved
tests/field_analyser.cpp Outdated Show resolved Hide resolved
src/ast/passes/semantic_analyser.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
@ttreyer ttreyer force-pushed the cxx-inheritance branch 2 times, most recently from cb95b1e to 4821ff7 Compare August 12, 2024 16:14
src/dwarf_parser.cpp Outdated Show resolved Hide resolved
Copy link
Member

@ajor ajor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Copy link
Contributor

@viktormalik viktormalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, great work!

tests/field_analyser.cpp Outdated Show resolved Hide resolved
@ajor ajor merged commit c2bfea6 into bpftrace:master Aug 14, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants