Date:   Sat, 10 Dec 2016 19:00:26 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     Alexis Berlemont <alexis.berlemont@...il.com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        linux-kernel@...r.kernel.org, peterz@...radead.org,
        mingo@...hat.com, alexander.shishkin@...ux.intel.com,
        hemant@...ux.vnet.ibm.com
Subject: Re: [PATCH v4 2/2] perf probe: add sdt probes arguments into the
 uprobe cmd string

On Fri, 9 Dec 2016 12:14:30 -0300
Arnaldo Carvalho de Melo <acme@...nel.org> wrote:

> Em Wed, Dec 07, 2016 at 12:26:10PM +0900, Masami Hiramatsu escreveu:
> > Hello Alexis,
> > 
> > On Sat, 26 Nov 2016 01:58:03 +0100
> > Alexis Berlemont <alexis.berlemont@...il.com> wrote:
> > 
> > > An sdt probe can be associated with arguments but they were not passed
> > > to the user probe tracing interface (uprobe_events); this patch adapts
> > > the sdt argument descriptors according to the uprobe input format.
> > 
> > Great!
> 
> Yeah, good to see work in this area!
> 
> I applied the first patch, with Masami's ack, waiting for his concerns
> on this one to be addressed, ok?

Yes, I'm OK. Alexis, I'm happy to review/ack your next version! :)

Thank you,

> 
> - Arnaldo
>  
> > > 
> > > As the uprobe parser does not support scaled address mode, perf will
> > > skip arguments which cannot be adapted to the uprobe format.
> > 
> > OK, it seems that skipping argument is a good idea :)
> > I just tried to support fixed-number arguments in probe events,
> > but skipping it is better with older kernel.
> > 
> > I have some comments.
> > 
> > > Here are the results:
> > > 
> > > $ perf buildid-cache -v --add test_sdt
> > > $ perf probe -x test_sdt sdt_libfoo:table_frob
> > > $ perf probe -x test_sdt sdt_libfoo:table_diddle
> > > $ perf record -e sdt_libfoo:table_frob -e sdt_libfoo:table_diddle test_sdt
> > > $ perf script
> > > test_sdt  ...   666.255678:   sdt_libfoo:table_frob: (4004d7) arg0=0 arg1=0
> > > test_sdt  ...   666.255683: sdt_libfoo:table_diddle: (40051a) arg0=0 arg1=0
> > > test_sdt  ...   666.255686:   sdt_libfoo:table_frob: (4004d7) arg0=1 arg1=2
> > > test_sdt  ...   666.255689: sdt_libfoo:table_diddle: (40051a) arg0=3 arg1=4
> > > test_sdt  ...   666.255692:   sdt_libfoo:table_frob: (4004d7) arg0=2 arg1=4
> > > test_sdt  ...   666.255694: sdt_libfoo:table_diddle: (40051a) arg0=6 arg1=8
> > 
> > We'd better start with arg1, since sdt.h and original Dtrace SDT starts
> > arguments from arg1 (I'm not sure why) and dtrace/systemtap scripts
> > call it "arg1".
> > 
> > > 
> > > Signed-off-by: Alexis Berlemont <alexis.berlemont@...il.com>
> > > ---
> > >  tools/perf/arch/x86/util/perf_regs.c |  18 ++++
> > >  tools/perf/util/perf_regs.c          |   4 +
> > >  tools/perf/util/perf_regs.h          |  13 +++
> > >  tools/perf/util/probe-file.c         | 169 ++++++++++++++++++++++++++++++++++-
> > >  4 files changed, 200 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/tools/perf/arch/x86/util/perf_regs.c b/tools/perf/arch/x86/util/perf_regs.c
> > > index c5db14f..52a1e65 100644
> > > --- a/tools/perf/arch/x86/util/perf_regs.c
> > > +++ b/tools/perf/arch/x86/util/perf_regs.c
> > > @@ -26,3 +26,21 @@ const struct sample_reg sample_reg_masks[] = {
> > >  #endif
> > >  	SMPL_REG_END
> > >  };
> > > +
> > > +const struct sdt_name_reg sdt_reg_renamings[] = {
> > > +	SDT_NAME_REG(eax, ax),
> > > +	SDT_NAME_REG(rax, ax),
> > > +	SDT_NAME_REG(ebx, bx),
> > > +	SDT_NAME_REG(rbx, bx),
> > > +	SDT_NAME_REG(ecx, cx),
> > > +	SDT_NAME_REG(rcx, cx),
> > > +	SDT_NAME_REG(edx, dx),
> > > +	SDT_NAME_REG(rdx, dx),
> > > +	SDT_NAME_REG(esi, si),
> > > +	SDT_NAME_REG(rsi, si),
> > > +	SDT_NAME_REG(edi, di),
> > > +	SDT_NAME_REG(rdi, di),
> > > +	SDT_NAME_REG(ebp, bp),
> > > +	SDT_NAME_REG(rbp, bp),
> > > +	SDT_NAME_REG_END,
> > > +};
> > 
> > It is not enough, rNN registers also have to take care, since
> > gcc adds 'd', 'w' or 'b'suffixes for those registers to indicate
> > its size. e.g. r15d means r15 register with 32 lower bits.
> > What we need is just cut them off, since probe event uses
> > length modifiers (like :u32)
> > 
> > > diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c
> > > index c4023f2..1c21150 100644
> > > --- a/tools/perf/util/perf_regs.c
> > > +++ b/tools/perf/util/perf_regs.c
> > > @@ -6,6 +6,10 @@ const struct sample_reg __weak sample_reg_masks[] = {
> > >  	SMPL_REG_END
> > >  };
> > >  
> > > +const struct sdt_name_reg __weak sdt_reg_renamings[] = {
> > > +	SDT_NAME_REG_END,
> > > +};
> > > +
> > >  #ifdef HAVE_PERF_REGS_SUPPORT
> > >  int perf_reg_value(u64 *valp, struct regs_dump *regs, int id)
> > >  {
> > > diff --git a/tools/perf/util/perf_regs.h b/tools/perf/util/perf_regs.h
> > > index 679d6e4..41815ca 100644
> > > --- a/tools/perf/util/perf_regs.h
> > > +++ b/tools/perf/util/perf_regs.h
> > > @@ -15,6 +15,19 @@ struct sample_reg {
> > >  
> > >  extern const struct sample_reg sample_reg_masks[];
> > >  
> > > +struct sdt_name_reg {
> > > +	const char *sdt_name;
> > > +	const char *uprobe_name;
> > > +};
> > > +#define SDT_NAME_REG(n, m) {.sdt_name = "%" #n, .uprobe_name = "%" #m}
> > > +#define SDT_NAME_REG_END {.sdt_name = NULL, .uprobe_name = NULL}
> > > +
> > > +/*
> > > + * The table sdt_reg_renamings is used for adjusting gcc/gas-generated
> > > + * registers before filling the uprobe tracer interface.
> > > + */
> > > +extern const struct sdt_name_reg sdt_reg_renamings[];
> > > +
> > >  #ifdef HAVE_PERF_REGS_SUPPORT
> > >  #include <perf_regs.h>
> > >  
> > > diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> > > index 436b647..75033c7 100644
> > > --- a/tools/perf/util/probe-file.c
> > > +++ b/tools/perf/util/probe-file.c
> > > @@ -27,6 +27,7 @@
> > >  #include "probe-event.h"
> > >  #include "probe-file.h"
> > >  #include "session.h"
> > > +#include "perf_regs.h"
> > >  
> > >  #define MAX_CMDLEN 256
> > >  
> > > @@ -687,6 +688,165 @@ static unsigned long long sdt_note__get_addr(struct sdt_note *note)
> > >  		 : (unsigned long long)note->addr.a64[0];
> > >  }
> > >  
> > > +static const char * const type_to_suffix[] = {
> > > +	":s64", "", "", "", ":s32", "", ":s16", ":s8",
> > > +	"", ":u8", ":u16", "", ":u32", "", "", "", ":u64"
> > > +};
> > > +
> > > +static int synthesize_sdt_probe_arg(struct strbuf *buf, int i, const char *arg)
> > > +{
> > > +	const struct sdt_name_reg *rnames;
> > > +	char *tmp, *desc = strdup(arg);
> > > +	const char *prefix = "", *suffix = "";
> > > +	int ret = -1;
> > > +
> > > +	if (desc == NULL) {
> > > +		pr_debug4("Allocation error\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	tmp = strchr(desc, '@');
> > > +	if (tmp) {
> > > +		long type_idx;
> > > +		/*
> > > +		 * Isolate the string number and convert it into a
> > > +		 * binary value; this will be an index to get suffix
> > > +		 * of the uprobe name (defining the type)
> > > +		 */
> > > +		tmp[0] = '\0';
> > > +		type_idx = strtol(desc, NULL, 10);
> > > +		if (type_idx == LONG_MIN ||
> > > +			type_idx == LONG_MAX) {
> > > +			pr_debug4("Failed to get sdt type\n");
> > > +			goto error;
> > > +		}
> > 
> > 	You must ensure 0 <= type_idx + 8 <= 16 here.
> > 
> > > +		suffix = type_to_suffix[type_idx + 8];
> > > +		/* Get rid of the sdt prefix which is now useless */
> > > +		tmp++;
> > > +		memmove(desc, tmp, strlen(tmp) + 1);
> > > +	}
> > > +
> > > +	/*
> > > +	 * The uprobe tracer format does not support all the
> > > +	 * addressing modes (notably: in x86 the scaled mode); so, we
> > > +	 * detect ',' characters, if there is just one, there is no
> > > +	 * use converting the sdt arg into a uprobe one.
> > > +	 */
> > > +	if (strchr(desc, ',')) {
> > > +		pr_debug4("SDT argument format not supported\n");
> > 
> > 	Please print 'desc' by %s too. 
> > 
> > > +		goto out;
> > > +	}
> > > +
> > > +	/*
> > > +	 * If the argument addressing mode is indirect, we must check
> > > +	 * a few things...
> > > +	 */
> > > +	tmp = strchr(desc, '(');
> > > +	if (tmp) {
> > > +		int j;
> > > +
> > > +		/*
> > > +		 * ...if the addressing mode is indirect with a
> > > +		 * positive offset (ex.: "1608(%ax)"), we need to add
> > > +		 * a '+' prefix so as to be compliant with uprobe
> > > +		 * format.
> > > +		 */
> > > +		if (desc[0] != '+' && desc[0] != '-')
> > > +			prefix = "+";
> > > +
> > > +		/*
> > > +		 * ...or if the addressing mode is indirect with a symbol
> > > +		 * as offset, the argument will not be supported by
> > > +		 * the uprobe tracer format; so, let's skip this one.
> > > +		 */
> > > +		for (j = 0; j < tmp - desc; j++) {
> > > +			if (desc[j] != '+' && desc[j] != '-' &&
> > > +				!isdigit(desc[j]))
> > > +				goto out;
> > > +		}
> > > +	}
> > > +
> > > +	/*
> > > +	 * The uprobe parser does not support all gas register names;
> > > +	 * so, we have to replace them (ex. for x86_64: %rax -> %ax);
> > > +	 * the loop below performs all the needed renamings if needed.
> > > +	 */
> > > +	for (rnames = sdt_reg_renamings; rnames->sdt_name != NULL; rnames++) {
> > > +		char *new_desc, *sdt_name;
> > > +		size_t prefix_len, uprobe_len, mid_ofs, desc_len;
> > > +
> > > +		sdt_name = strstr(desc, rnames->sdt_name);
> > > +		if (sdt_name == NULL)
> > > +			continue;
> > 
> > It is better to search '%' from the desc and parse it.
> > And here, we also find fixed numbers which starts with '$',
> > since that is not supported yet.
> > 
> > For example, with your patch, I still see some entries which have fixed num.
> > 
> > $ perf buildid-cache --add /usr/lib64/libglib-2.0.so
> > $ grep \$[0-9] ~/.debug/usr/lib64/libglib-2.0.so.0.5000.2/fda1ca4181ba7135d41bf3cfadc813a432f31066/probes | tail -n 2
> > p:sdt_glib/mem__realloc /usr/lib64/libglib-2.0.so.0.5000.2:0x4f670 arg0=%ax:u64 arg1=%bx:u64 arg2=%bp:u32 arg3=$0:s32
> > p:sdt_glib/mem__realloc /usr/lib64/libglib-2.0.so.0.5000.2:0x4f75d arg0=%ax:u64 arg1=%bp:u64 arg2=%bx:u32 arg3=$1:s32
> > 
> > These arguments should be skipped.
> > 
> > Thank you,
> > 
> > > +
> > > +		new_desc = zalloc(strlen(desc) + 1 +
> > > +				strlen(rnames->uprobe_name) -
> > > +				strlen(rnames->sdt_name));
> > > +		if (new_desc == NULL)
> > > +			goto error;
> > > +
> > > +		prefix_len = sdt_name - desc;
> > > +		if (prefix_len != 0)
> > > +			memcpy(new_desc, desc, prefix_len);
> > > +
> > > +		uprobe_len = strlen(rnames->uprobe_name);
> > > +		memcpy(new_desc + prefix_len, rnames->uprobe_name, uprobe_len);
> > > +
> > > +		mid_ofs = prefix_len + strlen(rnames->sdt_name);
> > > +		desc_len = strlen(desc);
> > > +		if (mid_ofs < desc_len)
> > > +			memcpy(new_desc + prefix_len + uprobe_len,
> > > +				desc + mid_ofs, desc_len - mid_ofs);
> > > +
> > > +		free(desc);
> > > +		desc = new_desc;
> > > +	}
> > > +
> > > +	if (strbuf_addf(buf, " arg%d=%s%s%s", i, prefix, desc, suffix) < 0)
> > > +		goto error;
> > > +
> > > +out:
> > > +	ret = 0;
> > > +error:
> > > +	free(desc);
> > > +	return ret;
> > > +}
> > > +
> > > +static char *synthesize_sdt_probe_command(struct sdt_note *note,
> > > +					const char *pathname,
> > > +					const char *sdtgrp)
> > > +{
> > > +	struct strbuf buf;
> > > +	char *ret = NULL, **args;
> > > +	int i, args_count;
> > > +
> > > +	if (strbuf_init(&buf, 32) < 0)
> > > +		return NULL;
> > > +
> > > +	if (strbuf_addf(&buf, "p:%s/%s %s:0x%llx",
> > > +				sdtgrp, note->name, pathname,
> > > +				sdt_note__get_addr(note)) < 0)
> > > +		goto error;
> > > +
> > > +	if (!note->args)
> > > +		goto out;
> > > +
> > > +	if (note->args) {
> > > +		args = argv_split(note->args, &args_count);
> > > +
> > > +		for (i = 0; i < args_count; ++i) {
> > > +			if (synthesize_sdt_probe_arg(&buf, i, args[i]) < 0)
> > > +				goto error;
> > > +		}
> > > +	}
> > > +
> > > +out:
> > > +	ret = strbuf_detach(&buf, NULL);
> > > +error:
> > > +	strbuf_release(&buf);
> > > +	return ret;
> > > +}
> > > +
> > >  int probe_cache__scan_sdt(struct probe_cache *pcache, const char *pathname)
> > >  {
> > >  	struct probe_cache_entry *entry = NULL;
> > > @@ -723,11 +883,12 @@ int probe_cache__scan_sdt(struct probe_cache *pcache, const char *pathname)
> > >  			entry->pev.group = strdup(sdtgrp);
> > >  			list_add_tail(&entry->node, &pcache->entries);
> > >  		}
> > > -		ret = asprintf(&buf, "p:%s/%s %s:0x%llx",
> > > -				sdtgrp, note->name, pathname,
> > > -				sdt_note__get_addr(note));
> > > -		if (ret < 0)
> > > +		buf = synthesize_sdt_probe_command(note, pathname, sdtgrp);
> > > +		if (!buf) {
> > > +			ret = -ENOMEM;
> > >  			break;
> > > +		}
> > > +
> > >  		strlist__add(entry->tevlist, buf);
> > >  		free(buf);
> > >  		entry = NULL;
> > > -- 
> > > 2.10.2
> > > 
> > 
> > 
> > -- 
> > Masami Hiramatsu <mhiramat@...nel.org>


-- 
Masami Hiramatsu <mhiramat@...nel.org>

Powered by blists - more mailing lists