Add wildcard support for kfunc probe types #1410

olsajiri · 2020-07-07T14:51:42Z

Adding wildcard support for kfunc probe types. It's now possible to use following probes:

# bpftrace -e 'kfunc:ksys_* { @[comm, probe] = kstack; }'

mmisono · 2020-07-08T00:12:16Z

@olsajiri
What do you think about this comment?

It seems this check is too restrictive. For example, vfs_open(const struct path *path, struct file *file) and vfs_write(struct file *file, ...) have the same arg struct file *file, but cannot create the program to access the arg.
% sudo ./src/bpftrace -e 'f:vfs_open, f:vfs_write { @ = args->file->f_count; }'
stdin:1:12-24: ERROR: Failed: probe has attach points with mixed arguments
f:vfs_open, f:vfs_write { @ = args->file->f_count; }
           ~~~~~~~~~~~~
This works on the current master.
#1347 (comment)

Well, I think it is OK to cope with it later in another PR. Other than that looks good to me.

olsajiri · 2020-07-08T09:28:55Z

@olsajiri
What do you think about this comment?

oops, did I overlook this comment somewhere? sry

It seems this check is too restrictive. For example, vfs_open(const struct path *path, struct file *file) and vfs_write(struct file *file, ...) have the same arg struct file *file, but cannot create the program to access the arg.
% sudo ./src/bpftrace -e 'f:vfs_open, f:vfs_write { @ = args->file->f_count; }'
stdin:1:12-24: ERROR: Failed: probe has attach points with mixed arguments
f:vfs_open, f:vfs_write { @ = args->file->f_count; }
           ~~~~~~~~~~~~
This works on the current master.
#1347 (comment)
Well, I think it is OK to cope with it later in another PR. Other than that looks good to me.

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:

vfs_open(const struct path *path, struct file *file)
vfs_write(struct file *file, ...)

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

olsajiri · 2020-07-13T21:12:38Z

rebased, @danobi @fbs , could you guys please check on it?

thanks

danobi

Few small comments but LGTM overall

danobi · 2020-07-15T00:07:22Z

src/ast/field_analyser.cpp

@@ -5,6 +5,16 @@
 namespace bpftrace {
 namespace ast {

+void FieldAnalyser::error(const std::string &msg, const location &loc)
+{
+  bpftrace_.error(err_, loc, msg);


Why store in err_ and not use out_ like warning()?

yep, should be out_, thanks

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

danobi · 2020-07-15T00:20:29Z

src/ast/field_analyser.cpp

+bool FieldAnalyser::compare_args(std::map<std::string, SizedType> args1,
+                                 std::map<std::string, SizedType> args2)


I think we can save a copy here

Suggested change

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)

true, will change

danobi · 2020-07-15T00:23:55Z

src/ast/field_analyser.cpp

+{
+  if (has_kfunc_probe_ && has_mixed_args_)
+  {
+    error("Failed: probe has attach points with mixed arguments",


Having Failed: seems unnecessary since there should already be a big ERROR: printed. My 2c

ok, will change, thanks

fbs · 2020-07-15T19:10:56Z

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>

olsajiri mentioned this pull request Jul 7, 2020

Add lsm probe support #1347

Closed

olsajiri force-pushed the kfunc/wildcards branch from 12c075e to 7c2f4d2 Compare July 13, 2020 21:11

danobi approved these changes Jul 15, 2020

View reviewed changes

olsajiri added 6 commits July 18, 2020 23:27

Rename FieldAnalyser::builtin_args_ to FieldAnalyser::has_builtin_args_

96c23fe

More bools are comming so it's convenient they follow same name pattern. Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Add error display support to FieldAnalyser class

a62b778

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>

Add test for multiple kfunc attachpoints

3dee211

Adding automated tests for multiple kfunc attachpoints. Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Factor BTF::get_kfuncs from BTF::display_funcs

f3dc921

New get_funcs function will be used in wildcard matching later in following changes. Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Add wildcard support for kfunc probe types

48d88da

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>

olsajiri force-pushed the kfunc/wildcards branch from 7c2f4d2 to 48d88da Compare July 18, 2020 21:27

fbs merged commit f070111 into bpftrace:master Jul 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add wildcard support for kfunc probe types #1410

Add wildcard support for kfunc probe types #1410

olsajiri commented Jul 7, 2020

mmisono commented Jul 8, 2020

olsajiri commented Jul 8, 2020

olsajiri commented Jul 13, 2020

danobi left a comment

danobi Jul 15, 2020

olsajiri Jul 18, 2020

mmisono Jul 20, 2020 •

edited

Loading

danobi Jul 15, 2020

olsajiri Jul 18, 2020

danobi Jul 15, 2020

olsajiri Jul 18, 2020

fbs commented Jul 15, 2020

		bool FieldAnalyser::compare_args(std::map<std::string, SizedType> args1,
		std::map<std::string, SizedType> args2)

Add wildcard support for kfunc probe types #1410

Add wildcard support for kfunc probe types #1410

Conversation

olsajiri commented Jul 7, 2020

mmisono commented Jul 8, 2020

olsajiri commented Jul 8, 2020

olsajiri commented Jul 13, 2020

danobi left a comment

Choose a reason for hiding this comment

danobi Jul 15, 2020

Choose a reason for hiding this comment

olsajiri Jul 18, 2020

Choose a reason for hiding this comment

mmisono Jul 20, 2020 • edited Loading

Choose a reason for hiding this comment

danobi Jul 15, 2020

Choose a reason for hiding this comment

olsajiri Jul 18, 2020

Choose a reason for hiding this comment

danobi Jul 15, 2020

Choose a reason for hiding this comment

olsajiri Jul 18, 2020

Choose a reason for hiding this comment

fbs commented Jul 15, 2020

mmisono Jul 20, 2020 •

edited

Loading