perf parse-events: report bpf errors
diff mbox series

Message ID 20200707211449.3868944-1-irogers@google.com
State In Next
Commit 5f634c8e401df25c479c8d07ed575804059c3826
Headers show
Series
  • perf parse-events: report bpf errors
Related show

Commit Message

Ian Rogers July 7, 2020, 9:14 p.m. UTC
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(-)

Comments

Arnaldo Carvalho de Melo July 8, 2020, 11:19 a.m. UTC | #1
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
>
Ian Rogers July 8, 2020, 3:15 p.m. UTC | #2
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
Arnaldo Carvalho de Melo July 8, 2020, 3:45 p.m. UTC | #3
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
Jiri Olsa July 8, 2020, 6:47 p.m. UTC | #4
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
Arnaldo Carvalho de Melo July 10, 2020, 12:37 p.m. UTC | #5
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

Patch
diff mbox series

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;
 	}