| Message ID | 20200707211449.3868944-1-irogers@google.com |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | perf parse-events: report bpf errors | expand |
Em Tue, Jul 07, 2020 at 02:14:49PM -0700, Ian Rogers escreveu: > Setting the parse_events_error directly doesn't increment num_errors > causing the error message not to be displayed. Use the > parse_events__handle_error function that sets num_errors and handle > multiple errors. What was the command line you used to exercise the error and then the fix? - Arnaldo > Signed-off-by: Ian Rogers <irogers@google.com> > --- > tools/perf/util/parse-events.c | 38 ++++++++++++++++++---------------- > 1 file changed, 20 insertions(+), 18 deletions(-) > > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c > index c4906a6a9f1a..e88e4c7a2a9a 100644 > --- a/tools/perf/util/parse-events.c > +++ b/tools/perf/util/parse-events.c > @@ -767,8 +767,8 @@ int parse_events_load_bpf_obj(struct parse_events_state *parse_state, > > return 0; > errout: > - parse_state->error->help = strdup("(add -v to see detail)"); > - parse_state->error->str = strdup(errbuf); > + parse_events__handle_error(parse_state->error, 0, > + strdup(errbuf), strdup("(add -v to see detail)")); > return err; > } > > @@ -784,36 +784,38 @@ parse_events_config_bpf(struct parse_events_state *parse_state, > return 0; > > list_for_each_entry(term, head_config, list) { > - char errbuf[BUFSIZ]; > int err; > > if (term->type_term != PARSE_EVENTS__TERM_TYPE_USER) { > - snprintf(errbuf, sizeof(errbuf), > - "Invalid config term for BPF object"); > - errbuf[BUFSIZ - 1] = '\0'; > - > - parse_state->error->idx = term->err_term; > - parse_state->error->str = strdup(errbuf); > + parse_events__handle_error(parse_state->error, term->err_term, > + strdup("Invalid config term for BPF object"), > + NULL); > return -EINVAL; > } > > err = bpf__config_obj(obj, term, parse_state->evlist, &error_pos); > if (err) { > + char errbuf[BUFSIZ]; > + int idx; > + > bpf__strerror_config_obj(obj, term, parse_state->evlist, > &error_pos, err, errbuf, > sizeof(errbuf)); > - parse_state->error->help = strdup( > + > + if (err == -BPF_LOADER_ERRNO__OBJCONF_MAP_VALUE) > + idx = term->err_val; > + else > + idx = term->err_term + error_pos; > + > + parse_events__handle_error(parse_state->error, idx, > + strdup(errbuf), > + strdup( > "Hint:\tValid config terms:\n" > " \tmap:[<arraymap>].value<indices>=[value]\n" > " \tmap:[<eventmap>].event<indices>=[event]\n" > "\n" > " \twhere <indices> is something like [0,3...5] or [all]\n" > -" \t(add -v to see detail)"); > - parse_state->error->str = strdup(errbuf); > - if (err == -BPF_LOADER_ERRNO__OBJCONF_MAP_VALUE) > - parse_state->error->idx = term->err_val; > - else > - parse_state->error->idx = term->err_term + error_pos; > +" \t(add -v to see detail)")); > return err; > } > } > @@ -877,8 +879,8 @@ int parse_events_load_bpf(struct parse_events_state *parse_state, > -err, errbuf, > sizeof(errbuf)); > > - parse_state->error->help = strdup("(add -v to see detail)"); > - parse_state->error->str = strdup(errbuf); > + parse_events__handle_error(parse_state->error, 0, > + strdup(errbuf), strdup("(add -v to see detail)")); > return err; > } > > -- > 2.27.0.383.g050319c2ae-goog >
On Wed, Jul 8, 2020 at 4:19 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > Em Tue, Jul 07, 2020 at 02:14:49PM -0700, Ian Rogers escreveu: > > Setting the parse_events_error directly doesn't increment num_errors > > causing the error message not to be displayed. Use the > > parse_events__handle_error function that sets num_errors and handle > > multiple errors. > > What was the command line you used to exercise the error and then the > fix? You need something to stand in for the BPF event so: Before: ``` $ /tmp/perf/perf record -e /tmp/perf/util/parse-events.o Run 'perf list' for a list of valid events Usage: perf record [<options>] [<command>] or: perf record [<options>] -- <command> [<options>] -e, --event <event> event selector. use 'perf list' to list available event ``` After: ``` $ /tmp/perf/perf record -e /tmp/perf/util/parse-events.o event syntax error: '/tmp/perf/util/parse-events.o' \___ Failed to load /tmp/perf/util/parse-events.o: BPF object format invalid (add -v to see detail) Run 'perf list' for a list of valid events Usage: perf record [<options>] [<command>] or: perf record [<options>] -- <command> [<options>] -e, --event <event> event selector. use 'perf list' to list available events ``` Thanks, Ian > - Arnaldo > > > Signed-off-by: Ian Rogers <irogers@google.com> > > --- > > tools/perf/util/parse-events.c | 38 ++++++++++++++++++---------------- > > 1 file changed, 20 insertions(+), 18 deletions(-) > > > > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c > > index c4906a6a9f1a..e88e4c7a2a9a 100644 > > --- a/tools/perf/util/parse-events.c > > +++ b/tools/perf/util/parse-events.c > > @@ -767,8 +767,8 @@ int parse_events_load_bpf_obj(struct parse_events_state *parse_state, > > > > return 0; > > errout: > > - parse_state->error->help = strdup("(add -v to see detail)"); > > - parse_state->error->str = strdup(errbuf); > > + parse_events__handle_error(parse_state->error, 0, > > + strdup(errbuf), strdup("(add -v to see detail)")); > > return err; > > } > > > > @@ -784,36 +784,38 @@ parse_events_config_bpf(struct parse_events_state *parse_state, > > return 0; > > > > list_for_each_entry(term, head_config, list) { > > - char errbuf[BUFSIZ]; > > int err; > > > > if (term->type_term != PARSE_EVENTS__TERM_TYPE_USER) { > > - snprintf(errbuf, sizeof(errbuf), > > - "Invalid config term for BPF object"); > > - errbuf[BUFSIZ - 1] = '\0'; > > - > > - parse_state->error->idx = term->err_term; > > - parse_state->error->str = strdup(errbuf); > > + parse_events__handle_error(parse_state->error, term->err_term, > > + strdup("Invalid config term for BPF object"), > > + NULL); > > return -EINVAL; > > } > > > > err = bpf__config_obj(obj, term, parse_state->evlist, &error_pos); > > if (err) { > > + char errbuf[BUFSIZ]; > > + int idx; > > + > > bpf__strerror_config_obj(obj, term, parse_state->evlist, > > &error_pos, err, errbuf, > > sizeof(errbuf)); > > - parse_state->error->help = strdup( > > + > > + if (err == -BPF_LOADER_ERRNO__OBJCONF_MAP_VALUE) > > + idx = term->err_val; > > + else > > + idx = term->err_term + error_pos; > > + > > + parse_events__handle_error(parse_state->error, idx, > > + strdup(errbuf), > > + strdup( > > "Hint:\tValid config terms:\n" > > " \tmap:[<arraymap>].value<indices>=[value]\n" > > " \tmap:[<eventmap>].event<indices>=[event]\n" > > "\n" > > " \twhere <indices> is something like [0,3...5] or [all]\n" > > -" \t(add -v to see detail)"); > > - parse_state->error->str = strdup(errbuf); > > - if (err == -BPF_LOADER_ERRNO__OBJCONF_MAP_VALUE) > > - parse_state->error->idx = term->err_val; > > - else > > - parse_state->error->idx = term->err_term + error_pos; > > +" \t(add -v to see detail)")); > > return err; > > } > > } > > @@ -877,8 +879,8 @@ int parse_events_load_bpf(struct parse_events_state *parse_state, > > -err, errbuf, > > sizeof(errbuf)); > > > > - parse_state->error->help = strdup("(add -v to see detail)"); > > - parse_state->error->str = strdup(errbuf); > > + parse_events__handle_error(parse_state->error, 0, > > + strdup(errbuf), strdup("(add -v to see detail)")); > > return err; > > } > > > > -- > > 2.27.0.383.g050319c2ae-goog > > > > -- > > - Arnaldo
Em Wed, Jul 08, 2020 at 08:15:24AM -0700, Ian Rogers escreveu: > On Wed, Jul 8, 2020 at 4:19 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > Em Tue, Jul 07, 2020 at 02:14:49PM -0700, Ian Rogers escreveu: > > > Setting the parse_events_error directly doesn't increment num_errors > > > causing the error message not to be displayed. Use the > > > parse_events__handle_error function that sets num_errors and handle > > > multiple errors. > > What was the command line you used to exercise the error and then the > > fix? > You need something to stand in for the BPF event so: > Before: > ``` > $ /tmp/perf/perf record -e /tmp/perf/util/parse-events.o > Run 'perf list' for a list of valid events > Usage: perf record [<options>] [<command>] > or: perf record [<options>] -- <command> [<options>] > -e, --event <event> event selector. use 'perf list' to list available event > ``` > After: > ``` > $ /tmp/perf/perf record -e /tmp/perf/util/parse-events.o > event syntax error: '/tmp/perf/util/parse-events.o' > \___ Failed to load /tmp/perf/util/parse-events.o: > BPF object format invalid > > (add -v to see detail) > Run 'perf list' for a list of valid events > Usage: perf record [<options>] [<command>] > or: perf record [<options>] -- <command> [<options>] > -e, --event <event> event selector. use 'perf list' to list > available events > ``` Cool, I'll add that to the cset comment log. If you need programs to test the Ok path, consider: # perf trace -e tools/perf/examples/bpf/5sec.c sleep 5.1 0.000 perf_bpf_probe:hrtimer_nanosleep(__probe_ip: -1508461648, rqtp: 5100000000) # After applying tge patch below, that I'll have soon in my repo, I guess this comes from those header includes path fixes from you :) # cat tools/perf/examples/bpf/5sec.c #include <bpf.h> #define NSEC_PER_SEC 1000000000L int probe(hrtimer_nanosleep, rqtp)(void *ctx, int err, long long sec) { return sec / NSEC_PER_SEC == 5ULL; } license(GPL); # Backtraces works as well and you can ask for the .o file to be retained so that you then skip the compilation phase and use the .o file directly: # perf config llvm.dump-obj=true # perf config llvm.dump-obj llvm.dump-obj=true # perf trace -e tools/perf/examples/bpf/5sec.c/max-stack=99/ sleep 5.1 0.000 perf_bpf_probe:hrtimer_nanosleep(__probe_ip: -1508461648, rqtp: 5100000000) hrtimer_nanosleep ([kernel.kallsyms]) __x64_sys_nanosleep ([kernel.kallsyms]) do_syscall_64 ([kernel.kallsyms]) entry_SYSCALL_64 ([kernel.kallsyms]) __GI___nanosleep (/usr/lib64/libc-2.29.so) # # file tools/perf/examples/bpf/5sec.o tools/perf/examples/bpf/5sec.o: ELF 64-bit LSB relocatable, eBPF, version 1 (SYSV), with debug_info, not stripped # # perf trace -e tools/perf/examples/bpf/5sec.o/max-stack=3/ sleep 5.1 0.000 perf_bpf_probe:hrtimer_nanosleep(__probe_ip: -1508461648, rqtp: 5100000000) hrtimer_nanosleep ([kernel.kallsyms]) __x64_sys_nanosleep ([kernel.kallsyms]) do_syscall_64 ([kernel.kallsyms]) # - Arnaldo [root@quaco perf]# git diff diff --git a/tools/perf/examples/bpf/5sec.c b/tools/perf/examples/bpf/5sec.c index 65c4ff6892d9..e6b6181c6dc6 100644 --- a/tools/perf/examples/bpf/5sec.c +++ b/tools/perf/examples/bpf/5sec.c @@ -39,7 +39,7 @@ Copyright (C) 2018 Red Hat, Inc., Arnaldo Carvalho de Melo <acme@redhat.com> */ -#include <bpf/bpf.h> +#include <bpf.h> #define NSEC_PER_SEC 1000000000L [root@quaco perf]# - Arnaldo > Thanks, > Ian > > > - Arnaldo > > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > > --- > > > tools/perf/util/parse-events.c | 38 ++++++++++++++++++---------------- > > > 1 file changed, 20 insertions(+), 18 deletions(-) > > > > > > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c > > > index c4906a6a9f1a..e88e4c7a2a9a 100644 > > > --- a/tools/perf/util/parse-events.c > > > +++ b/tools/perf/util/parse-events.c > > > @@ -767,8 +767,8 @@ int parse_events_load_bpf_obj(struct parse_events_state *parse_state, > > > > > > return 0; > > > errout: > > > - parse_state->error->help = strdup("(add -v to see detail)"); > > > - parse_state->error->str = strdup(errbuf); > > > + parse_events__handle_error(parse_state->error, 0, > > > + strdup(errbuf), strdup("(add -v to see detail)")); > > > return err; > > > } > > > > > > @@ -784,36 +784,38 @@ parse_events_config_bpf(struct parse_events_state *parse_state, > > > return 0; > > > > > > list_for_each_entry(term, head_config, list) { > > > - char errbuf[BUFSIZ]; > > > int err; > > > > > > if (term->type_term != PARSE_EVENTS__TERM_TYPE_USER) { > > > - snprintf(errbuf, sizeof(errbuf), > > > - "Invalid config term for BPF object"); > > > - errbuf[BUFSIZ - 1] = '\0'; > > > - > > > - parse_state->error->idx = term->err_term; > > > - parse_state->error->str = strdup(errbuf); > > > + parse_events__handle_error(parse_state->error, term->err_term, > > > + strdup("Invalid config term for BPF object"), > > > + NULL); > > > return -EINVAL; > > > } > > > > > > err = bpf__config_obj(obj, term, parse_state->evlist, &error_pos); > > > if (err) { > > > + char errbuf[BUFSIZ]; > > > + int idx; > > > + > > > bpf__strerror_config_obj(obj, term, parse_state->evlist, > > > &error_pos, err, errbuf, > > > sizeof(errbuf)); > > > - parse_state->error->help = strdup( > > > + > > > + if (err == -BPF_LOADER_ERRNO__OBJCONF_MAP_VALUE) > > > + idx = term->err_val; > > > + else > > > + idx = term->err_term + error_pos; > > > + > > > + parse_events__handle_error(parse_state->error, idx, > > > + strdup(errbuf), > > > + strdup( > > > "Hint:\tValid config terms:\n" > > > " \tmap:[<arraymap>].value<indices>=[value]\n" > > > " \tmap:[<eventmap>].event<indices>=[event]\n" > > > "\n" > > > " \twhere <indices> is something like [0,3...5] or [all]\n" > > > -" \t(add -v to see detail)"); > > > - parse_state->error->str = strdup(errbuf); > > > - if (err == -BPF_LOADER_ERRNO__OBJCONF_MAP_VALUE) > > > - parse_state->error->idx = term->err_val; > > > - else > > > - parse_state->error->idx = term->err_term + error_pos; > > > +" \t(add -v to see detail)")); > > > return err; > > > } > > > } > > > @@ -877,8 +879,8 @@ int parse_events_load_bpf(struct parse_events_state *parse_state, > > > -err, errbuf, > > > sizeof(errbuf)); > > > > > > - parse_state->error->help = strdup("(add -v to see detail)"); > > > - parse_state->error->str = strdup(errbuf); > > > + parse_events__handle_error(parse_state->error, 0, > > > + strdup(errbuf), strdup("(add -v to see detail)")); > > > return err; > > > } > > > > > > -- > > > 2.27.0.383.g050319c2ae-goog > > > > > > > -- > > > > - Arnaldo
On Tue, Jul 07, 2020 at 02:14:49PM -0700, Ian Rogers wrote: > Setting the parse_events_error directly doesn't increment num_errors > causing the error message not to be displayed. Use the > parse_events__handle_error function that sets num_errors and handle > multiple errors. > > Signed-off-by: Ian Rogers <irogers@google.com> looks good Acked-by: Jiri Olsa <jolsa@redhat.com> thanks, jirka
Em Wed, Jul 08, 2020 at 08:47:32PM +0200, Jiri Olsa escreveu: > On Tue, Jul 07, 2020 at 02:14:49PM -0700, Ian Rogers wrote: > > Setting the parse_events_error directly doesn't increment num_errors > > causing the error message not to be displayed. Use the > > parse_events__handle_error function that sets num_errors and handle > > multiple errors. > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > looks good > > Acked-by: Jiri Olsa <jolsa@redhat.com> Thanks, applied. - Arnaldo
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c index c4906a6a9f1a..e88e4c7a2a9a 100644 --- a/tools/perf/util/parse-events.c +++ b/tools/perf/util/parse-events.c @@ -767,8 +767,8 @@ int parse_events_load_bpf_obj(struct parse_events_state *parse_state, return 0; errout: - parse_state->error->help = strdup("(add -v to see detail)"); - parse_state->error->str = strdup(errbuf); + parse_events__handle_error(parse_state->error, 0, + strdup(errbuf), strdup("(add -v to see detail)")); return err; } @@ -784,36 +784,38 @@ parse_events_config_bpf(struct parse_events_state *parse_state, return 0; list_for_each_entry(term, head_config, list) { - char errbuf[BUFSIZ]; int err; if (term->type_term != PARSE_EVENTS__TERM_TYPE_USER) { - snprintf(errbuf, sizeof(errbuf), - "Invalid config term for BPF object"); - errbuf[BUFSIZ - 1] = '\0'; - - parse_state->error->idx = term->err_term; - parse_state->error->str = strdup(errbuf); + parse_events__handle_error(parse_state->error, term->err_term, + strdup("Invalid config term for BPF object"), + NULL); return -EINVAL; } err = bpf__config_obj(obj, term, parse_state->evlist, &error_pos); if (err) { + char errbuf[BUFSIZ]; + int idx; + bpf__strerror_config_obj(obj, term, parse_state->evlist, &error_pos, err, errbuf, sizeof(errbuf)); - parse_state->error->help = strdup( + + if (err == -BPF_LOADER_ERRNO__OBJCONF_MAP_VALUE) + idx = term->err_val; + else + idx = term->err_term + error_pos; + + parse_events__handle_error(parse_state->error, idx, + strdup(errbuf), + strdup( "Hint:\tValid config terms:\n" " \tmap:[<arraymap>].value<indices>=[value]\n" " \tmap:[<eventmap>].event<indices>=[event]\n" "\n" " \twhere <indices> is something like [0,3...5] or [all]\n" -" \t(add -v to see detail)"); - parse_state->error->str = strdup(errbuf); - if (err == -BPF_LOADER_ERRNO__OBJCONF_MAP_VALUE) - parse_state->error->idx = term->err_val; - else - parse_state->error->idx = term->err_term + error_pos; +" \t(add -v to see detail)")); return err; } } @@ -877,8 +879,8 @@ int parse_events_load_bpf(struct parse_events_state *parse_state, -err, errbuf, sizeof(errbuf)); - parse_state->error->help = strdup("(add -v to see detail)"); - parse_state->error->str = strdup(errbuf); + parse_events__handle_error(parse_state->error, 0, + strdup(errbuf), strdup("(add -v to see detail)")); return err; }
Setting the parse_events_error directly doesn't increment num_errors causing the error message not to be displayed. Use the parse_events__handle_error function that sets num_errors and handle multiple errors. Signed-off-by: Ian Rogers <irogers@google.com> --- tools/perf/util/parse-events.c | 38 ++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 18 deletions(-)