Report helper function errors #1276
Conversation
| BasicBlock *helper_merge_block = BasicBlock::Create(module_.getContext(), | ||
| "helper_merge", | ||
| parent); | ||
| auto *ret = CreateIntCast(return_value, getInt32Ty(), true); |
There was a problem hiding this comment.
I found that a return type of most helper functions was Int64Ty but they actually returned int and we needed to use Int32Ty value to check if a return value was negative. I cast the value here to keep code changes of this PR as small as possible.
There was a problem hiding this comment.
Can you leave that as a comment in the code? Will help next person who wonders why there's a cast.
|
I think this solves #464. I forgot to mention but this uses perf_event_output() for error reporting, so an error of perf_event_output() is not reported. An error message is printed using |
|
This is the comparison of processed insn sizes (not the actual code size. Since the verifier explorers all possible code path, processed insn size becomes larger than the code size). Multiple values mean that the scripts have multiple functions. So, basically error checking increases the number of processed insn by tens or hundreds, but some scripts (especially which have many map updates) increase code size by thousands (e.g., capable.bt). I also measured the compilation times but I didn't see noticeable differences with and without error checking. I didn't measure runtime speed. I'm still wondering what is the best default behavior, and I'd appreciate any feedback.
|
docs/reference_guide.md
Outdated
| @@ -141,6 +141,8 @@ OPTIONS: | |||
| -p PID enable USDT probes on PID | |||
| -c 'CMD' run CMD and enable USDT probes on resulting process | |||
| -v verbose messages | |||
| --check-all-helper check return codes of all helper functions | |||
There was a problem hiding this comment.
| --check-all-helper check return codes of all helper functions | |
| --check-helper-errors emit a warning when a bpf helper returns an error |
I think this is a bit more descriptive.
Also alignment :)
src/main.cpp
Outdated
| @@ -57,14 +57,16 @@ void usage() | |||
| std::cerr << " --unsafe allow unsafe builtin functions" << std::endl; | |||
| std::cerr << " -v verbose messages" << std::endl; | |||
| std::cerr << " --info Print information about kernel BPF support" << std::endl; | |||
| std::cerr << " --check-all-helper check return codes of all helper functions" << std::endl; | |||
There was a problem hiding this comment.
same comment and alignment thing here
|
Nice, this is useful :) Seems like the processed instruction count is actually reduced in the |
|
Thanks for the review, will fix. "default" in the above table means the default behavior when this patch applied (so it checks errors other than bpf_probe_read_xxx / bpf_map_lookup_elem). |
|
I examined the generated code. Basically, this error check creates one branch and adds 10 instructions per helper function call. A problem is a branch creates another state and the verifier needs to explore paths both that take a branch and not. This is the reason the processed instructions sharply increase in scripts that call many helper functions. The verifier prunes a search if two states are equivalent, but unfortunately not. I'm wondering if we can implement this functionality using bpf-to-bpf call. I guess it reduces states. I'll give it a try later. (there is little information about this..) |
mmisono
force-pushed
the
report_helper_errors
branch
2 times, most recently
from
a0c1e8d to
027c544
Compare
|
I fixed the reviewed points and changed the option since the initial version was a bit confusing:
Therefore, if these options are not given, program behavior is the same as the current. What do you think about this? @fbs (As I wrote, I'm seeing if bpf-to-bpf call is usable, but I don't plan to include it in this PR even if it works.) |
|
Nice, this looks really useful. Code looks good to me. Can you add some codegen tests with the error checks and also a runtime test or two? Just so we don't accidentally break this later. Sorry about the delay, didn't see this PR. In the future feel free to ping me if PR get delayed too much. |
| BasicBlock *helper_merge_block = BasicBlock::Create(module_.getContext(), | ||
| "helper_merge", | ||
| parent); | ||
| auto *ret = CreateIntCast(return_value, getInt32Ty(), true); |
There was a problem hiding this comment.
Can you leave that as a comment in the code? Will help next person who wonders why there's a cast.
mmisono
force-pushed
the
report_helper_errors
branch
from
027c544 to
5461b5d
Compare
|
Updated:
Please take a look when you have time @danobi |
Some helper functions return error codes when failing, but currently,
bpftrace does not check and discards it. Several times I had to use bcc
to check error codes for debugging.
This patch adds an error checking mechanism, and helper function errors
are reported in runtime using '-k' option. For example:
```
% sudo BPFTRACE_MAP_KEYS_MAX=2 ./src/bpftrace -e \
'BEGIN { @[0] = 0; @[1] = 1; @[2] = 2;} -k'
Attaching 1 probe...
stdin:1:35-36: WARNING: Failed to map_update_elem: Argument list too long (-7)
BEGIN { @[0] = 0; @[1] = 1; @[2] = 2;}
~
^C
@[0]: 0
@[1]: 1
```
This mechanism is implemented as an async action. By default, errors of
`bpf_probe_read()`-related functions and `bpf_map_lookup_elem()` are not
checked. This is because when these functions fail, bpftrace returns
zero as a result, and some scripts already check it, for example, by
using predicate `/ @[tid] /`. `-kk` option forces to check all errors
including these functions.
Note that the error check increases code size. All codegen tests are
performed with the error check disabled.
Helper error checking increases code size and some scripts requires more bigger log size.
mmisono
force-pushed
the
report_helper_errors
branch
2 times, most recently
from
811059e to
181c0cb
Compare
Some helper functions return error codes when failing, but currently,
bpftrace does not check and discards it. Several times I had to use bcc
to check error codes for debugging.
This patch adds an error checking mechanism, and helper function errors
are reported in runtime using '-k' option. For example:
This mechanism is implemented as an async action. By default, errors of
bpf_probe_read()-related functions andbpf_map_lookup_elem()are notchecked. This is because when these functions fail, bpftrace returns
zero as a result, and some scripts already check it, for example, by
using predicate
/ @[tid] /.-kkoption forces to check all errorsincluding these functions.
Note that the error check increases code size. All codegen tests are
performed with the error check disabled.