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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

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 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
  • 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
},
5 // Bottom's w
};
func_3(m, b);

Check warning

Code scanning / CodeQL

Expression has no effect Warning

This expression has no effect (because
func_3
has no external side effects).
{
Parent p{ 1, 2, 3, 4 };
Child c{ 1, 2, 3, 4, 5, 6 };
func_1(c, p);

Check warning

Code scanning / CodeQL

Expression has no effect Warning

This expression has no effect (because
func_1
has no external side effects).
func_1(c, p);

LittleChild lc{ 1, 2, 3, 4, 5, 6, 7 };
func_2(lc);

Check warning

Code scanning / CodeQL

Expression has no effect Warning

This expression has no effect (because
func_2
has no external side effects).
@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 3 times, most recently from 099b9f5 to e44449c Compare May 23, 2024 16:27
@ttreyer ttreyer force-pushed the cxx-inheritance branch 3 times, most recently from d61b5dd to 4580a6d Compare May 23, 2024 19:15
tests/testprogs/uprobe_test_cxx.cpp Fixed Show fixed Hide fixed

int x = 42;
Foo foo{ 1, 2, 3, x };
uprobeFunction1(x, foo);

Check warning

Code scanning / CodeQL

Expression has no effect Warning

This expression has no effect (because
uprobeFunction1
has no external side effects).
Child c{ 1, 2, 3, 4, 5, 6 };

int arr[10] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };
uprobeArray(arr);

Check warning

Code scanning / CodeQL

Expression has no effect Warning

This expression has no effect (because
uprobeArray
has no external side effects).
@ttreyer ttreyer force-pushed the cxx-inheritance branch 2 times, most recently from dd69afc to 6316e7f Compare May 24, 2024 12:07
@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.

Comment on lines +29 to +30
EXPECT Child & c
EXPECT Parent & p
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
EXPECT Child & c
EXPECT Parent & p
EXPECT uprobe:./testprogs/uprobe_test_cxx:cpp:"uprobeFunction3(Child&, Parent&)"
Child & c
Parent & p

The multi-line EXPECT will verify that the order of the arguments is printed correct too

TIMEOUT 5

NAME list uprobe args - array reference type
RUN {{BPFTRACE}} -lv 'uprobe:./testprogs/uprobe_test_cxx:_Z11uprobeArrayRA10_i'
Copy link
Member

Choose a reason for hiding this comment

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

Why not use the demangled name here?

Comment on lines +3 to +29
class Parent {
private:
int a;

protected:
int b;

public:
int c;
int d; // Shadowed by Child::d, but should be reachable with a cast

Parent(int a, int b, int c, int d) : a(a), b(b), c(c), d(d)
{
}
};

class Child : public Parent {
public:
int d;
int e;
int f;

Child(int a, int b, int c, int d, int e, int f)
: Parent(a, b, c, d), d(d + 1), e(e), f(f)
{
}
};
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't look like these fields or inheritance hierarchy are being used in the tests - are they needed? Best to keep things as simple as possible if not necessary

{
const auto &type = expr->type;
if (type.IsRefTy()) {
expr = new Unop(Operator::MUL, expr, expr->loc);
Copy link
Member

Choose a reason for hiding this comment

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

If we convert the wrapped reference here into a pointer, then I think that will simplify later passes by not having to care about reference types any more. I'm hoping we can get away with zero changes to codegen and field_analyser. With luck, it might help with the crash I found too.

Here's an example to explain what I mean a bit better.
Original AST:

. :: [reference (struct Foo)]
  builtin: args :: [struct uprobeFunction1_args]
  foo

Current transformation:

dereference :: [struct Foo]
  . :: [reference (struct Foo)]
    builtin: args :: [struct uprobeFunction1_args]
    foo

AST after converting reference into pointer:

dereference :: [struct Foo]
  . :: [pointer (struct Foo)]
    builtin: args :: [struct uprobeFunction1_args]
    foo

@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?

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.

None yet

2 participants