Allow working with all probe params (kfunc, uprobe) #2477
Conversation
viktormalik
requested review from
ajor,
danobi,
fbs and
BurntBrunch
as code owners
|
looks good! |
viktormalik
force-pushed
the
print-args
branch
from
17065de
to
324fe35
Compare
|
Rebased on master, ready for merge. |
There was a problem hiding this comment.
First of all I just want to say that I really like the idea behind this change - the examples look great!
Some of the commits in this PR are simple and unobjectionable and could be merged independently - that would make the core change here a lot less intimidating to go through :)
Language Design
I know that in the current implementation, args is treated as a pointer, but does it need to be?
I don't remember, but I'm assuming I made it like that just because it was easiest - just pass the context pointer directly around and call it "args".
Ideally the bpftrace language won't become full of cruft because of lazy decisions made in the past. It seems to me that a pointer doesn't actually make sense, since we don't allow using it without first dereferencing.
What do you think about allowing use of args without dereferencing? e.g.:
kfunc:vfs_read { @a = args }
kfunc:vfs_read { @b = args.file }
The old syntax of "args->file" could be kept for backwards compatibility (at least for a while).
Bugs
I came across a couple of bugs when playing around with this new builtin:
Error dereferencing args in tracepoint
# ./build/src/bpftrace -e 'tracepoint:syscalls:sys_enter_read { @ = *args }'
Attaching 1 probe...
ERROR: failed to create map: '@': Invalid argument
Creation of the required BPF maps has failed.
Make sure you have all the required permissions and are not confined (e.g. like
snapcraft does). `dmesg` will likely have useful output for further troubleshooting
Segfault dereferencing args when function is not defined
void foo(int a, int b);
int main() { }
# ./build/src/bpftrace -e 'uprobe:/tmp/a.out:foo { @ = *args }'
Segmentation fault
src/output.cpp
Outdated
| std::ostringstream res; | ||
| res << "0x" << std::hex << read_data<uint64_t>(value.data()); | ||
| return res.str(); |
There was a problem hiding this comment.
I see this is a common pattern in output.cpp, but for future reference I think we'll need to change this at some point (not now), as output is one place where we actually need to care about the performance of bpftrace. Taking longer here can mean we don't keep up with events from the kernel.
Maybe we should just drop the |
|
Interesting idea. I'll need to think about it some more but I like simplifying the language. My main concern is unintentionally breaking some part of the C language, like happened when we didn't require the "struct" prefix when casting: #682 |
Well, we'd have to support What would change is that users wouldn't be able to rely on the type system to warn them if they use |
|
I'm having second thoughts about the move from "->" to "." now. What would we do about dereferencing pointers when not accessing fields? C++ and Rust are two languages that mainly use the "." syntax to access struct fields, but that's because they mainly use references instead of pointers. Reading a reference gives the value being pointed to rather than the address it holds (it's implicitly dereferenced). We wouldn't want this behaviour for pointers in bpftrace though, since users will often want and expect an address if they're reading a pointer. Maybe we should keep pointers and references as separate ideas - we can introduce reference types for higher level language support later, but it feels like this change would be creating something half way between a reference and a pointer. Opinions? I'm also not sure that it will help with this PR for printing args - if args remains as a pointer type, then printing it will still require |
|
I didn't take a very close look at the PR, but the runtime test examples look great.
I think simplicity is a good north star for the bpftrace language. While I like the idea of a unifying
I think reference types would require some more discussion, perhaps at an office hours. I have lots of thoughts on that :).
I don't think we can get rid of it easily. For example rust even supports |
I agree, deprecating
Yes, let's discuss on the next office hours :-)
But do we need to keep Anyways, allowing |
|
After giving it more thought, I realized that we're talking about two independent problems here:
While I'm not sure if the former is a good idea (as it may introduce confusion), I like the latter as |
viktormalik
force-pushed
the
print-args
branch
2 times, most recently
from
b507cc3
to
be7487f
Compare
|
Rebased onto master which now includes #2578 which started treating |
There was a problem hiding this comment.
I have a couple of questions to help me understand this area better:
Q1: Will we able to do something similar to this for tracepoint args in the future or is there some fundamental limitation for them?
Q2: Why are uprobe args handled differently from kfunc args? (put on the stack)
I also found a few more problems which we'll need to sort out before merging:
uprobe arg collisions
#include <stdio.h>
void __attribute__((noinline)) bar(int a, int b, int c, int d, int e, int f) {
printf("hi\n");
}
void __attribute__((noinline)) baz(char g, char h, char i) {
printf("hi\n");
}
int main() {
bar(11,12,13,14,15,16);
baz(21, 22, 23);
}
gcc test.c -g -O0
Works fine with just one uprobe:
sudo ./src/bpftrace -e 'uprobe:/tmp/a.out:baz { print(args); }'
Attaching 1 probe...
{ .g = 21, .h = 22, .i = 23 }
^C
Wrong results with multiple uprobes:
sudo ./src/bpftrace -e 'uprobe:/tmp/a.out:bar { print(args); } uprobe:/tmp/a.out:baz { print(args); }'
Attaching 2 probes...
{ .a = 11, .b = 12, .c = 13, .d = 14, .e = 15, .f = 16 }
{ .g = 21, .h = 0, .i = 0 }
^C
Crash when uprobe order is swapped:
sudo ./src/bpftrace -e 'uprobe:/tmp/a.out:baz { print(args); } uprobe:/tmp/a.out:bar { print(args); }'
bpftrace: /usr/include/llvm/IR/Instructions.h:922: llvm::Type* llvm::checkGEPType(llvm::Type*): Assertion `Ty && "Invalid GetElementPtrInst indices for type!"' failed.
Aborted
Error message
The error message for uprobes with lots of arguments isn't very nice:
sudo ./src/bpftrace -e 'uprobe:/tmp/a.out:foo { print(args); }'
ERROR: Failed to compile: array::at: __n (which is 6) >= _Nm (which is 6)
Repro:
void __attribute__((noinline)) foo(int a, int b, int c, int d, int e, int f, int g) {
}
int main() {
foo(1,2,3,4,5,6,7);
}
gcc test.c -g -O0
|
Thanks for the review! I added fixup commits to simplify another reviewing and will squash them before merge.
We should be. Tracepoint args are passed in a similar way to kfuncs, but they have some specialties, like extra fields at the beginning of the struct which we will want to omit or
That's b/c for kfuncs, the BPF program gets passed a pointer to a struct containing all the arguments (pretty much exactly what we want). OTOH, uprobe args are passed via registers (the passed context is pointer to |
src/ast/irbuilderbpf.cpp
Outdated
|
|
||
| llvm::Type *IRBuilderBPF::UprobeArgsType(const SizedType &args_type) | ||
| { | ||
| auto type_name = args_type.GetName().substr(strlen("struct ")); |
There was a problem hiding this comment.
| auto type_name = args_type.GetName().substr(strlen("struct ")); | |
| auto type_name = args_type.GetName(); | |
| type_name.erase(0, strlen("struct ")); |
Removes the prefix in-place to avoid creating a temporary std::string with substr(), eliminating an unnecessary malloc/memcpy/free
There was a problem hiding this comment.
Good idea, fixed.
When trying to access the 'args' builtin for a probe in which the arguments cannot be parsed (e.g. are not present in DWARF/BTF), throw an error in semantic analyser (instead of segfaulting in codegen).
This was disabled by commit 722c5c7 ("Error if trying to assign args to a variable in kfunc") since it was not supported at the time. Now, FieldAnalyser supports this so it can be enabled.
Since args is now representing a record containing all function args, it may be used as a map key/value since the it will be proberead and copied into the map. For now, this only works for k(ret)func probes.
Allows to pull all arguments of a uprobe from DWARF into a single
structure (record) using "args". The record can be used as any other
record - printed, stored inside maps/variables, used as a map key, etc.
This enables constructions like:
print(args);
@[args] = count();
@[tid] = args;
The implementation builds on the fact that probe args are now stored
inside struct manager under a special "struct <probename>_args" type.
When accessing the "args" builtin in a uprobe, this builds a new record
(having the above type) on stack and fills it with probe args (from
registers).
One effect of this is that all uprobe args are always read onto stack,
even when a single arg is necessary (e.g. "args.a" is used). This is a
bit inefficient but much better fits the current implementation. Since
there are at most 6 args, the slowdown should be small.
Also a difference from other records in bpftrace is that we don't use
byte arrays to represent the new record with an LLVM type. The reason is
that it is easier to use GEPs with indices to access individual args,
instead of using byte offsets inside the new record.
Adds runtime tests for the above constructions.
Add entry to the reference guide and manpage about the `args` builtin.
|
Squashed the fixup commits, ready for merge. |
Allows to use
*argsin multiple constructions such as:The main change is that probe args are now represented using a special record with
struct <probename>_argstype and hence can be treated as any other record type. Construction of the type is different for kfuncs and uprobes:ctxpointer points to the struct of args which has exactly the same layout as our representation, so the implementation is straightforward.*argsoccurs in the code, the args record is constructed on BPF stack and filled from registers. Note: this is done every time, even when onlyargs->xis used (and it would be sufficient to read one register). The reason for this is that creating entire*argselegantly fits the current implementation and since there are at most 6 args supported, this should be quite a cheap operation.This required some minor changes, especially to codegen tests, see commit messages for more details.
Checklist
man/adoc/bpftrace.adocand if needed indocs/reference_guide.mdCHANGELOG.md