From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753731AbdCFRcX (ORCPT ); Mon, 6 Mar 2017 12:32:23 -0500 Received: from mail.kernel.org ([198.145.29.136]:57144 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752673AbdCFRcK (ORCPT ); Mon, 6 Mar 2017 12:32:10 -0500 Date: Mon, 6 Mar 2017 18:23:00 +0100 From: Masami Hiramatsu To: Alexis Berlemont Cc: linux-kernel@vger.kernel.org, peterz@infradead.org, mingo@redhat.com, acme@kernel.org, alexander.shishkin@linux.intel.com Subject: Re: [PATCH v5 2/2] perf probe: add sdt probes arguments into the uprobe cmd string Message-Id: <20170306182300.d66f137aa17f98a100dbbcd9@kernel.org> In-Reply-To: <20161214000732.1710-3-alexis.berlemont@gmail.com> References: <20161214000732.1710-1-alexis.berlemont@gmail.com> <20161214000732.1710-3-alexis.berlemont@gmail.com> X-Mailer: Sylpheed 3.5.0 (GTK+ 2.24.30; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 14 Dec 2016 01:07:32 +0100 Alexis Berlemont 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. > > 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 > OK, this looks good to me, good job! Acked-by: Masami Hiramatsu Thanks! > Signed-off-by: Alexis Berlemont > --- > tools/perf/arch/x86/util/perf_regs.c | 83 +++++++++++++++++ > tools/perf/util/perf_regs.c | 6 ++ > tools/perf/util/perf_regs.h | 6 ++ > tools/perf/util/probe-file.c | 170 ++++++++++++++++++++++++++++++++++- > 4 files changed, 261 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..09a7f55 100644 > --- a/tools/perf/arch/x86/util/perf_regs.c > +++ b/tools/perf/arch/x86/util/perf_regs.c > @@ -1,4 +1,7 @@ > +#include > + > #include "../../perf.h" > +#include "../../util/util.h" > #include "../../util/perf_regs.h" > > const struct sample_reg sample_reg_masks[] = { > @@ -26,3 +29,83 @@ const struct sample_reg sample_reg_masks[] = { > #endif > SMPL_REG_END > }; > + > +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} > + > +static 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, > +}; > + > +int sdt_rename_register(char **pdesc, char *old_name) > +{ > + const struct sdt_name_reg *rnames = sdt_reg_renamings; > + char *new_desc, *old_desc = *pdesc; > + size_t prefix_len, sdt_len, uprobe_len, old_desc_len, offset; > + int ret = -1; > + > + while (ret != 0 && rnames->sdt_name != NULL) { > + sdt_len = strlen(rnames->sdt_name); > + ret = strncmp(old_name, rnames->sdt_name, sdt_len); > + rnames += !!ret; > + } > + > + if (rnames->sdt_name == NULL) > + return 0; > + > + sdt_len = strlen(rnames->sdt_name); > + uprobe_len = strlen(rnames->uprobe_name); > + old_desc_len = strlen(old_desc) + 1; > + > + new_desc = zalloc(old_desc_len + uprobe_len - sdt_len); > + if (new_desc == NULL) > + return -1; > + > + /* Copy the chars before the register name (at least '%') */ > + prefix_len = old_name - old_desc; > + memcpy(new_desc, old_desc, prefix_len); > + > + /* Copy the new register name */ > + memcpy(new_desc + prefix_len, rnames->uprobe_name, uprobe_len); > + > + /* Copy the chars after the register name (if need be) */ > + offset = prefix_len + sdt_len; > + if (offset < old_desc_len) { > + /* > + * The orginal register name can be suffixed by 'b', > + * 'w' or 'd' to indicate its size; so, we need to > + * skip this char if we met one. > + */ > + char sfx = old_desc[offset]; > + > + if (sfx == 'b' || sfx == 'w' || sfx == 'd') > + offset++; > + } > + > + if (offset < old_desc_len) > + memcpy(new_desc + prefix_len + uprobe_len, > + old_desc + offset, old_desc_len - offset); > + > + free(old_desc); > + *pdesc = new_desc; > + > + return 0; > +} > diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c > index c4023f2..a37e593 100644 > --- a/tools/perf/util/perf_regs.c > +++ b/tools/perf/util/perf_regs.c > @@ -6,6 +6,12 @@ const struct sample_reg __weak sample_reg_masks[] = { > SMPL_REG_END > }; > > +int __weak sdt_rename_register(char **pdesc __maybe_unused, > + char *old_name __maybe_unused) > +{ > + return 0; > +} > + > #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..7544a15 100644 > --- a/tools/perf/util/perf_regs.h > +++ b/tools/perf/util/perf_regs.h > @@ -15,6 +15,12 @@ struct sample_reg { > > extern const struct sample_reg sample_reg_masks[]; > > +/* > + * The table sdt_reg_renamings is used for adjusting gcc/gas-generated > + * registers before filling the uprobe tracer interface. > + */ > +int sdt_rename_register(char **pdesc, char *old_name); > + > #ifdef HAVE_PERF_REGS_SUPPORT > #include > > diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c > index 436b647..d8a169e 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,166 @@ 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) > +{ > + 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); > + /* Check that the conversion went OK */ > + if (type_idx == LONG_MIN || type_idx == LONG_MAX) { > + pr_debug4("Failed to parse sdt type\n"); > + goto error; > + } > + /* Check that the converted value is OK */ > + if (type_idx < -8 || type_idx > 8) { > + pr_debug4("Failed to get a valid 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("Skipping unsupported SDT argument; %s\n", desc); > + 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])) { > + pr_debug4("Skipping unsupported SDT argument; " > + "%s\n", desc); > + goto out; > + } > + } > + } > + > + /* > + * The uprobe tracer format does not support constants; if we > + * find one in the current argument, let's skip the argument. > + */ > + if (strchr(desc, '$')) { > + pr_debug4("Skipping unsupported SDT argument; %s\n", desc); > + 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 looks for the register names (starting with > + * a '%' and tries to perform the needed renamings. > + */ > + tmp = strchr(desc, '%'); > + while (tmp) { > + size_t offset = tmp - desc; > + > + ret = sdt_rename_register(&desc, desc + offset); > + if (ret < 0) > + goto error; > + > + /* > + * The desc pointer might have changed; so, let's not > + * try to reuse tmp for next lookup > + */ > + tmp = strchr(desc + offset + 1, '%'); > + } > + > + if (strbuf_addf(buf, " arg%d=%s%s%s", i + 1, 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 +884,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