Re: [PATCH perf/core 07/10] perf probe: Support @BUILDID or @FILE suffix for SDT events

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Wed, Jul 13, 2016 at 04:50:19PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Jul 12, 2016 at 07:05:37PM +0900, Masami Hiramatsu escreveu:
> > Support @BUILDID or @FILE suffix for SDT events. This allows
> > perf to add probes on SDTs/pre-cached events on given FILE
> > or the file which has given BUILDID (also, this complements
> > BUILDID.)
> > For example, both gcc and libstdc++ has same SDTs as below.
> > If you would like to add a probe on sdt_libstdcxx:catch on gcc,
> > you can do as below.
> 
> Ok, how to add those entries?
> 
> [root@jouet hello_world]# perf probe -x /usr/bin/gcc -a %sdt_gcc:\*
>   Error: Failed to add events.
> [root@jouet hello_world]# perf probe --verbose -x /usr/bin/gcc -a %sdt_gcc:\*
> probe-definition(0): %sdt_gcc:*
> 0 arguments
> symbol:throw file:(null) line:0 offset:0 return:0 lazy:(null)
> symbol:rethrow file:(null) line:0 offset:0 return:0 lazy:(null)
> symbol:catch file:(null) line:0 offset:0 return:0 lazy:(null)
> symbol:unwind file:(null) line:0 offset:0 return:0 lazy:(null)
>   Error: Failed to add events. Reason: No such file or directory (Code: -2)
> [root@jouet hello_world]# 

So, ok, I managed with:

[root@jouet ~]# perf probe -x /usr/bin/gcc %sdt_libstdcxx:\*
Added new events:
  sdt_libstdcxx:throw  (on %* in /usr/bin/gcc)
  sdt_libstdcxx:rethrow (on %* in /usr/bin/gcc)
  sdt_libstdcxx:catch  (on %* in /usr/bin/gcc)

You can now use it in all perf tools, such as:

	perf record -e sdt_libstdcxx:catch -aR sleep 1

[root@jouet ~]# 

I then tried to remove the events just created:

# perf probe -d  %sdt_libstdcxx:\*
#

It tells me nothing, cool, probably it removed it all, then I go and
try re-adding them:

[root@jouet ~]# perf probe -x /usr/bin/gcc %sdt_libstdcxx:\*
Error: event "throw" already exists.
 Hint: Remove existing event by 'perf probe -d'
       or force duplicates by 'perf probe -f'
       or set 'force=yes' in BPF source.
  Error: Failed to add events.
[root@jouet ~]#

Huh?

Then I tried like you did above, using 'perf list sdt'

[root@jouet ~]# perf list sdt

List of pre-defined events (to be used in -e):

[root@jouet ~]#

But they appear via 'perf probe -l'

[root@jouet ~]# perf probe -l sdt_libs*
  sdt_libstdcxx:catch  (on execute_cfa_program+1551@../../../libgcc/unwind-dw2.c in /usr/bin/gcc)
  sdt_libstdcxx:rethrow (on d_print_subexpr+280@libsupc++/cp-demangle.c in /usr/bin/gcc)
  sdt_libstdcxx:throw  (on d_print_subexpr+93@libsupc++/cp-demangle.c in /usr/bin/gcc)
[root@jouet ~]#

There is no event, then, guessing, I tried removing that %, since I don't see
it in the 'perf probe -l' output, so it must've been just some distracting
artifact at the time of adding the SDT prove:

[root@jouet ~]# perf probe -d  sdt_libstdcxx:\*
Removed event: sdt_libstdcxx:catch
Removed event: sdt_libstdcxx:rethrow
Removed event: sdt_libstdcxx:throw
[root@jouet ~]# perf probe -l sdt_libs*
[root@jouet ~]#

Ok, now it works, but then if I try having both the gcc and libstdc++ ones, I can't:

[root@jouet ~]# perf probe -x /usr/lib64/libstdc++.so.6 %sdt_libstdcxx:\*
Error: event "catch" already exists.
 Hint: Remove existing event by 'perf probe -d'
       or force duplicates by 'perf probe -f'
       or set 'force=yes' in BPF source.
  Error: Failed to add events.

So I forced it:

[root@jouet ~]# perf probe -f -x /usr/lib64/libstdc++.so.6 %sdt_libstdcxx:\*
Added new events:
  sdt_libstdcxx:catch_1 (on %* in /usr/lib64/libstdc++.so.6.0.22)
  sdt_libstdcxx:throw_1 (on %* in /usr/lib64/libstdc++.so.6.0.22)
  sdt_libstdcxx:rethrow_1 (on %* in /usr/lib64/libstdc++.so.6.0.22)

You can now use it in all perf tools, such as:

	perf record -e sdt_libstdcxx:rethrow_1 -aR sleep 1

[root@jouet ~]# 

If it will rename it, why require -f? Where is the duplicity?

If I go and re-read all the discussion on the thread that lead to these
patches, I may figure out all those questions, but then, that would be a lot
of info to have in mind :-\

I.e. I would suggest that no -f should be needed, that that "Error: event
"catch" already exists" shouldn't be even emitted, since those will be
different events, i.e. one is named "sdt_libstdcxx:catch" and the other
"sdt_libstdcxx:catch_1"...

But that becomes an usability problem, couldn't it have the basename in it?
I.e.:

sdt_gcc_libstdcxx:catch and sdt_libstdc++:catch?

I'm going to look my inbox for patches I overlook that added 'perf list sdt'...

- Arnaldo

 
> Can those messages be improved? What am I missing?
>  
> >   ----
> >   # perf list sdt | tail -n 6
> >     sdt_libstdcxx:catch@/usr/bin/gcc(0cc207fc4b27)     [SDT event]
> >     sdt_libstdcxx:catch@/usr/lib64/libstdc++.so.6.0.20(91c7a88fdf49)
> >     sdt_libstdcxx:rethrow@/usr/bin/gcc(0cc207fc4b27)   [SDT event]
> >     sdt_libstdcxx:rethrow@/usr/lib64/libstdc++.so.6.0.20(91c7a88fdf49)
> >     sdt_libstdcxx:throw@/usr/bin/gcc(0cc207fc4b27)     [SDT event]
> >     sdt_libstdcxx:throw@/usr/lib64/libstdc++.so.6.0.20(91c7a88fdf49)
> >   # perf probe -a %sdt_libstdcxx:catch@0cc
> >   Added new event:
> >     sdt_libstdcxx:catch  (on %catch in /usr/bin/gcc)
> > 
> >   You can now use it in all perf tools, such as:
> > 
> >   	perf record -e sdt_libstdcxx:catch -aR sleep 1
> >   ----
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  Changes in v12:
> >   - Rename strlist__for_each to strlist__for_each_entry.
> > ---
> >  tools/perf/util/build-id.c    |   43 +++++++++++++++++++++++++++++++++++++++++
> >  tools/perf/util/build-id.h    |    1 +
> >  tools/perf/util/probe-event.c |   17 ++++++++++++++--
> >  3 files changed, 59 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> > index 8e86a83..e59003e 100644
> > --- a/tools/perf/util/build-id.c
> > +++ b/tools/perf/util/build-id.c
> > @@ -523,6 +523,49 @@ err_out:
> >  	goto out_free;
> >  }
> >  
> > +static bool str_is_build_id(const char *maybe_sbuild_id, size_t len)
> > +{
> > +	size_t i;
> > +
> > +	for (i = 0; i < len; i++) {
> > +		if (!isxdigit(maybe_sbuild_id[i]))
> > +			return false;
> > +	}
> > +	return true;
> > +}
> > +
> > +/* Return the valid complete build-id */
> > +char *build_id_cache__complement(const char *incomplete_sbuild_id)
> > +{
> > +	struct strlist *bidlist;
> > +	struct str_node *nd, *cand = NULL;
> > +	char *sbuild_id = NULL;
> > +	size_t len = strlen(incomplete_sbuild_id);
> > +
> > +	if (len >= SBUILD_ID_SIZE ||
> > +	    !str_is_build_id(incomplete_sbuild_id, len))
> > +		return NULL;
> > +
> > +	bidlist = build_id_cache__list_all(true);
> > +	if (!bidlist)
> > +		return NULL;
> > +
> > +	strlist__for_each_entry(nd, bidlist) {
> > +		if (strncmp(nd->s, incomplete_sbuild_id, len) != 0)
> > +			continue;
> > +		if (cand) {	/* Error: There are more than 2 candidates. */
> > +			cand = NULL;
> > +			break;
> > +		}
> > +		cand = nd;
> > +	}
> > +	if (cand)
> > +		sbuild_id = strdup(cand->s);
> > +	strlist__delete(bidlist);
> > +
> > +	return sbuild_id;
> > +}
> > +
> >  char *build_id_cache__cachedir(const char *sbuild_id, const char *name,
> >  			       bool is_kallsyms, bool is_vdso)
> >  {
> > diff --git a/tools/perf/util/build-id.h b/tools/perf/util/build-id.h
> > index 64e740f..d279906 100644
> > --- a/tools/perf/util/build-id.h
> > +++ b/tools/perf/util/build-id.h
> > @@ -35,6 +35,7 @@ char *build_id_cache__linkname(const char *sbuild_id, char *bf, size_t size);
> >  char *build_id_cache__cachedir(const char *sbuild_id, const char *name,
> >  			       bool is_kallsyms, bool is_vdso);
> >  struct strlist *build_id_cache__list_all(bool validonly);
> > +char *build_id_cache__complement(const char *incomplete_sbuild_id);
> >  int build_id_cache__list_build_ids(const char *pathname,
> >  				   struct strlist **result);
> >  bool build_id_cache__cached(const char *sbuild_id);
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index 679aa6b..0dadaa3 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -1251,8 +1251,21 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
> >  	ptr = strpbrk(arg, ";=@+%");
> >  	if (pev->sdt) {
> >  		if (ptr) {
> > -			semantic_error("%s must contain only an SDT event name.\n", arg);
> > -			return -EINVAL;
> > +			if (*ptr != '@') {
> > +				semantic_error("%s must be an SDT name.\n",
> > +					       arg);
> > +				return -EINVAL;
> > +			}
> > +			/* This must be a target file name or build id */
> > +			tmp = build_id_cache__complement(ptr + 1);
> > +			if (tmp) {
> > +				pev->target = build_id_cache__origname(tmp);
> > +				free(tmp);
> > +			} else
> > +				pev->target = strdup(ptr + 1);
> > +			if (!pev->target)
> > +				return -ENOMEM;
> > +			*ptr = '\0';
> >  		}
> >  		ret = parse_perf_probe_event_name(&arg, pev);
> >  		if (ret == 0) {