-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 wildcard support for kfunc probe types #1410
Conversation
@olsajiri
Well, I think it is OK to cope with it later in another PR. Other than that looks good to me. |
oops, did I overlook this comment somewhere? sry
the issue here is that the argument you want to use need to have the same position, because that's how we access arguments in kfunc program - through its index into the argument array so when you have single code for multiple probes, that's accessing an argument, that argument must be in the same place in arguments array for each probe, so the example above would not work anyway:
because 'file' has different argument index in each function we could make the check more permissive to allow this, I thought this to be a corner case which might not be worth to add so far.. I'll check on it however I'm planning to add 'outside' wildcard matching that should solve all these cases.. meaning that bpftrace will create extra separate probe code for every matched probe, which can't be shared with others |
12c075e
to
7c2f4d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few small comments but LGTM overall
src/ast/field_analyser.cpp
Outdated
@@ -5,6 +5,16 @@ | |||
namespace bpftrace { | |||
namespace ast { | |||
|
|||
void FieldAnalyser::error(const std::string &msg, const location &loc) | |||
{ | |||
bpftrace_.error(err_, loc, msg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why store in err_
and not use out_
like warning()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, should be out_, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olsajiri
I believe err_
should be used here since FieldAnalyzer later checks if an error occurs by consulting err_
length:
https://github.com/iovisor/bpftrace/blob/f0701114e22709ad4828c8c83f84f467e558e670/src/ast/field_analyser.cpp#L369-L374
Now semantic_analyser_btf.kfunc
test fails because field analyzer always returns zero.
src/ast/field_analyser.cpp
Outdated
bool FieldAnalyser::compare_args(std::map<std::string, SizedType> args1, | ||
std::map<std::string, SizedType> args2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can save a copy here
bool FieldAnalyser::compare_args(std::map<std::string, SizedType> args1, | |
std::map<std::string, SizedType> args2) | |
bool FieldAnalyser::compare_args(const std::map<std::string, SizedType>& args1, | |
const std::map<std::string, SizedType>& args2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, will change
src/ast/field_analyser.cpp
Outdated
{ | ||
if (has_kfunc_probe_ && has_mixed_args_) | ||
{ | ||
error("Failed: probe has attach points with mixed arguments", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having Failed:
seems unnecessary since there should already be a big ERROR:
printed. My 2c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, will change, thanks
can you add a changelog entry for this? |
More bools are comming so it's convenient they follow same name pattern. Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Adding error and warning functions to emit error with location and allow to fail from FieldAnalyser class with some nice output. Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Currently we allow to attach multiple attach points for a single probe code which uses args->X. If arguments of all the functions are not the same, bad things will happen ;-) Adding the arguments check for multiple attach points and failing only if arguments are used in the code. Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Adding automated tests for multiple kfunc attachpoints. Signed-off-by: Jiri Olsa <jolsa@kernel.org>
New get_funcs function will be used in wildcard matching later in following changes. Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Adding wildcard support for kfunc probe types. It's now possible to use following probes: # bpftrace -e 'kfunc:ksys_* { @[comm, probe] = kstack; }' Signed-off-by: Jiri Olsa <jolsa@kernel.org>
7c2f4d2
to
48d88da
Compare
Adding wildcard support for kfunc probe types. It's now possible to use following probes:
# bpftrace -e 'kfunc:ksys_* { @[comm, probe] = kstack; }'