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

Add tuples to language #1326

Merged
merged 14 commits into from
May 26, 2020
Merged

Add tuples to language #1326

merged 14 commits into from
May 26, 2020

Conversation

danobi
Copy link
Member

@danobi danobi commented May 11, 2020

This PR adds tuple support to the bpftrace language.

Tuples are useful for a couple of reasons:

  • There is already a way to associate multiple values to a map key
    (@[1,2] = 3) which is useful. Likewise, it would be very useful to
    associate multiple values with a map value (ie @[1] = (2,3)).
    Scripts currently work around this by using multiple maps. That's
    fine but it'd be nicer to have the language support common use
    cases.
  • Tuples provide a way to structure data together in output.
    Especially for distributed tracing, one common feature request is
    to have multiple pieces of data appear as one "row" in wherever
    the data is collected. You could work around this by using printf
    and requiring a certain format (like CSV), but that's not very
    reliable and can be easily broken by typos.

This PR allows stuff like the following:

$t = (1, 2, "string");
printf("%d %d %s\n", $t.0, $t.1, $t.2);

@m = ("str", (1, 2));
printf("%s %d %d\n", @m.0, @m.1.0, @m.1.1);
print(@m);

I'd rather get this proof of concept out to get feedback first, so
still on the todo list:

  • print()ing tuples
    • so stuff like @ = (1,2); print(@); comes out correctly
  • modifying a tuple after creation (probably for another PR)
    • is this even desired? for example python tuples are immutable
    • would look something like: $t = (0,1); $t.0 = 5;
Checklist
  • Language changes are updated in docs/reference_guide.md
  • User-visible and non-trivial changes updated in CHANGELOG.md

@@ -314,10 +314,17 @@ expr : int { $$ = $1; }
| MINUS expr { $$ = new ast::Unop(token::MINUS, $2, @1); }
| MUL expr %prec DEREF { $$ = new ast::Unop(token::MUL, $2, @1); }
| expr DOT ident { $$ = new ast::FieldAccess($1, $3, @2); }
| expr DOT INT { $$ = new ast::FieldAccess($1, $3, @3); }
Copy link
Contributor

Choose a reason for hiding this comment

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

For the parser I think map_or_var DOT INT makes more sense here. The downside is the error it generates

stdin:1:9-16: ERROR: syntax error, unexpected integer
i:s:1 { (1+2).2 }
        ~~~~~~~

Although I don't like the current one that much either:

stdin:1:9-16: ERROR: Can not access field '' on expression of type 'int64'
i:s:1 { (1+2).2 }
        ~~~~~~~

Copy link
Member Author

@danobi danobi May 20, 2020

Choose a reason for hiding this comment

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

I think this would break nested tuple indexing:

[  FAILED  ] basic.basic tuple
        Command: ../src/bpftrace -e 'BEGIN { $v = 99; $t = (0, 1, "str", (5, 6), $v); printf("%d %d %s %d %d %d\n", $t.0, $t.1, $t.2, $t.3.0, $t.3.1, $t.4); exit(); }'
        Expected: 0 1 str 5 6 99
        Found: stdin:1:98-104: ERROR: syntax error, unexpected integer
BEGIN { $v = 99; $t = (0, 1, "str", (5, 6), $v); printf("%d %d %s %d %d %d\n", $t.0, $t.1, $t.2, $t.3.0, $t.3.1, $t.4); exit(); }
                                                                                                 ~~~~~~

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I can make the error message better too

src/ast/codegen_llvm.cpp Outdated Show resolved Hide resolved
src/output.cpp Outdated Show resolved Hide resolved
@mmisono
Copy link
Collaborator

mmisono commented May 17, 2020

Can we have a more user-friendly error message?

% ./src/bpftrace -e 'kprobe:vfs_read { @bytes[comm] = (avg(arg2), sum(arg2)); }'
stdin:1:34-44: ERROR: avg() should be assigned to a map
kprobe:vfs_read { @bytes[comm] = (avg(arg2), sum(arg2)); }
                                 ~~~~~~~~~~
stdin:1:46-55: ERROR: sum() should be assigned to a map
kprobe:vfs_read { @bytes[comm] = (avg(arg2), sum(arg2)); }
                                             ~~~~~~~~~

This commit sets up all the scaffolding for actual implementation.
This helper is used to determine if a memcpy is necessary or if a simple
load will work.
Before:
```
stdin:1:9-16: ERROR: Can not access field '' on expression of type 'int64'
BEGIN { (1+2).2 }
        ~~~~~~~
```

After:
```
stdin:1:9-16: ERROR: Can not access index 2 on expression of type 'int64'
BEGIN { (1+2).2 }
        ~~~~~~~
```
@danobi
Copy link
Member Author

danobi commented May 21, 2020

Can we have a more user-friendly error message?

I added the word "directly". Now it's:

# bpftrace -e 'kprobe:vfs_read { @bytes[comm] = (avg(arg2), sum(arg2)); }'
stdin:1:34-44: ERROR: avg() should be directly assigned to a map
kprobe:vfs_read { @bytes[comm] = (avg(arg2), sum(arg2)); }
                                 ~~~~~~~~~~
stdin:1:46-55: ERROR: sum() should be directly assigned to a map
kprobe:vfs_read { @bytes[comm] = (avg(arg2), sum(arg2)); }

I thought about adding a hint like "Is the call value being used in a tuple?" but I worry about making the common case more confusing. I feel like more people will do things like $v = avg($asdf) than $v = (avg($asdf), "string").

This commit implements codegen for tuple creation and tuple indexing.

Note that tuples are allocated on the stack because we can't really
contain aggregate types in registers.
Have `map_value_to_str` take a SizedType instead of an `IMap` reference
so we can later reuse this method to print tuple elements.
Support for both text and json output
@danobi danobi merged commit 3f9939e into bpftrace:master May 26, 2020
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