Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1530578 > unrolled thread

[PATCH v4 2/2] perf probe: add sdt probes arguments into the uprobe cmd string

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v4 2/2] perf probe: add sdt probes arguments into the uprobe cmd string Alexis Berlemont <alexis.berlemont@gmail.com> - 2016-11-26 02:10 +0100
    Re: [PATCH v4 2/2] perf probe: add sdt probes arguments into the  uprobe cmd string Masami Hiramatsu <mhiramat@kernel.org> - 2016-12-07 04:30 +0100
      Re: [PATCH v4 2/2] perf probe: add sdt probes arguments into the  uprobe cmd string Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-12-09 16:20 +0100
        Re: [PATCH v4 2/2] perf probe: add sdt probes arguments into the  uprobe cmd string Masami Hiramatsu <mhiramat@kernel.org> - 2016-12-10 11:10 +0100

#1530578 — [PATCH v4 2/2] perf probe: add sdt probes arguments into the uprobe cmd string

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.

As the uprobe parser does not support scaled address mode, perf will
skip arguments which cannot be adapted to the uprobe format.

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

Signed-off-by: Alexis Berlemont <alexis.berlemont@gmail.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,
+};
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;
+		}
+		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");
+		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;
+
+		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

[toc] | [next] | [standalone]

#1537442 — Re: [PATCH v4 2/2] perf probe: add sdt probes arguments into the uprobe cmd string

Hello Alexis,

On Sat, 26 Nov 2016 01:58:03 +0100
Alexis Berlemont <alexis.berlemont@gmail.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!

> 
> 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@gmail.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@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1539433 — Re: [PATCH v4 2/2] perf probe: add sdt probes arguments into the uprobe cmd string

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@gmail.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?

- 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@gmail.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@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1539806 — 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@kernel.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@gmail.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@gmail.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@kernel.org>


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web