From: Masami Hiramatsu <mhiramat@kernel.org>
To: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
Cc: acme@redhat.com, alexis.berlemont@gmail.com,
linux-kernel@vger.kernel.org, peterz@infradead.org,
mingo@redhat.com, alexander.shishkin@linux.intel.com,
mpe@ellerman.id.au, naveen.n.rao@linux.vnet.ibm.com,
mhiramat@kernel.org, maddy@linux.vnet.ibm.com
Subject: Re: [PATCH 3/5] perf/sdt/x86: Move OP parser to tools/perf/arch/x86/
Date: Tue, 7 Feb 2017 12:11:05 +0900 [thread overview]
Message-ID: <20170207121105.c16c1bded3f24c798eac318a@kernel.org> (raw)
In-Reply-To: <20170202111143.14319-4-ravi.bangoria@linux.vnet.ibm.com>
On Thu, 2 Feb 2017 16:41:41 +0530
Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
> SDT marker argument is in N@OP format. N is the size of argument and
> OP is the actual assembly operand. OP is arch dependent component and
> hence it's parsing logic also should be placed under tools/perf/arch/.
>
Ok, I have one question. Is there any possibility that N is different
size of OP? e.g. 8@dil, in this case we will record whole rdi.
is that OK?
Thanks,
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> ---
> tools/perf/arch/x86/util/perf_regs.c | 93 ++++++++++++++++++++++++-
> tools/perf/util/perf_regs.c | 9 ++-
> tools/perf/util/perf_regs.h | 7 +-
> tools/perf/util/probe-file.c | 127 +++++++++--------------------------
> 4 files changed, 134 insertions(+), 102 deletions(-)
>
> diff --git a/tools/perf/arch/x86/util/perf_regs.c b/tools/perf/arch/x86/util/perf_regs.c
> index d8a8dcf..34fcb0d 100644
> --- a/tools/perf/arch/x86/util/perf_regs.c
> +++ b/tools/perf/arch/x86/util/perf_regs.c
> @@ -3,6 +3,7 @@
> #include "../../perf.h"
> #include "../../util/util.h"
> #include "../../util/perf_regs.h"
> +#include "../../util/debug.h"
>
> const struct sample_reg sample_reg_masks[] = {
> SMPL_REG(AX, PERF_REG_X86_AX),
> @@ -87,7 +88,16 @@ static const struct sdt_name_reg sdt_reg_renamings[] = {
> SDT_NAME_REG_END,
> };
>
> -int sdt_rename_register(char **pdesc, char *old_name)
> +bool arch_sdt_probe_arg_supp(void)
> +{
> + return true;
> +}
> +
> +/*
> + * The table sdt_reg_renamings is used for adjusting gcc/gas-generated
> + * registers before filling the uprobe tracer interface.
> + */
> +static int sdt_rename_register(char **pdesc, char *old_name)
> {
> const struct sdt_name_reg *rnames = sdt_reg_renamings;
> char *new_desc, *old_desc = *pdesc;
> @@ -129,3 +139,84 @@ int sdt_rename_register(char **pdesc, char *old_name)
>
> return 0;
> }
> +
> +/*
> + * x86 specific implementation
> + * return value:
> + * <0 : error
> + * 0 : success
> + * >0 : skip
> + */
> +int arch_sdt_probe_parse_op(char **desc, const char **prefix)
> +{
> + char *tmp;
> + int ret = 0;
> +
> + /*
> + * 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.
> + *
> + * Also it does not support constants; if we find one in the
> + * current argument, let's skip the argument.
> + */
> + if (strchr(*desc, ',') || strchr(*desc, '$')) {
> + pr_debug4("Skipping unsupported SDT argument; %s\n", *desc);
> + return 1;
> + }
> +
> + /*
> + * 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 = ((*desc)[0] == '(') ? "+0" : "+";
> +
> + /*
> + * ...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);
> + return 1;
> + }
> + }
> + }
> +
> + /*
> + * 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)
> + return ret;
> +
> + /*
> + * The *desc pointer might have changed; so, let's not
> + * try to reuse tmp for next lookup
> + */
> + tmp = strchr(*desc + offset + 1, '%');
> + }
> + return 0;
> +}
> diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c
> index a37e593..f2b3d0d 100644
> --- a/tools/perf/util/perf_regs.c
> +++ b/tools/perf/util/perf_regs.c
> @@ -6,8 +6,13 @@ 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)
> +bool __weak arch_sdt_probe_arg_supp(void)
> +{
> + return false;
> +}
> +
> +int __weak arch_sdt_probe_parse_op(char **op_ptr __maybe_unused,
> + const char **prefix __maybe_unused)
> {
> return 0;
> }
> diff --git a/tools/perf/util/perf_regs.h b/tools/perf/util/perf_regs.h
> index 7544a15..86a2961 100644
> --- a/tools/perf/util/perf_regs.h
> +++ b/tools/perf/util/perf_regs.h
> @@ -15,11 +15,8 @@ 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);
> +bool arch_sdt_probe_arg_supp(void);
> +int arch_sdt_probe_parse_op(char **op_ptr, const char **prefix);
>
> #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 d8a169e..38eca3c 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -693,6 +693,25 @@ static const char * const type_to_suffix[] = {
> "", ":u8", ":u16", "", ":u32", "", "", "", ":u64"
> };
>
> +/*
> + * Isolate the string number and convert it into a decimal value;
> + * this will be an index to get suffix of the uprobe name (defining
> + * the type)
> + */
> +static int sdt_probe_parse_n(char *n_ptr, const char **suffix)
> +{
> + long type_idx;
> +
> + type_idx = strtol(n_ptr, NULL, 10);
> + if (type_idx < -8 || type_idx > 8) {
> + pr_debug4("Failed to get a valid sdt type\n");
> + return -1;
> + }
> +
> + *suffix = type_to_suffix[type_idx + 8];
> + return 0;
> +}
> +
> static int synthesize_sdt_probe_arg(struct strbuf *buf, int i, const char *arg)
> {
> char *tmp, *desc = strdup(arg);
> @@ -704,109 +723,29 @@ static int synthesize_sdt_probe_arg(struct strbuf *buf, int i, const char *arg)
> return ret;
> }
>
> + /*
> + * Argument is in N@OP format. N is size of the argument and OP is
> + * the actual assembly operand. N can be omitted; in that case
> + * argument is just OP(without @).
> + */
> 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)
> + if (sdt_probe_parse_n(desc, &suffix))
> goto error;
> -
> - /*
> - * The desc pointer might have changed; so, let's not
> - * try to reuse tmp for next lookup
> - */
> - tmp = strchr(desc + offset + 1, '%');
> + memmove(desc, tmp, strlen(tmp) + 1);
> }
>
> - if (strbuf_addf(buf, " arg%d=%s%s%s", i + 1, prefix, desc, suffix) < 0)
> + ret = arch_sdt_probe_parse_op(&desc, &prefix);
> + if (ret < 0)
> + goto error;
> +
> + if (ret == 0 &&
> + strbuf_addf(buf, " arg%d=%s%s%s", i + 1, prefix, desc, suffix) < 0)
> goto error;
>
> -out:
> ret = 0;
> error:
> free(desc);
> @@ -829,7 +768,7 @@ static char *synthesize_sdt_probe_command(struct sdt_note *note,
> sdt_note__get_addr(note)) < 0)
> goto error;
>
> - if (!note->args)
> + if (!note->args || !arch_sdt_probe_arg_supp())
> goto out;
>
> if (note->args) {
> --
> 2.9.3
>
--
Masami Hiramatsu <mhiramat@kernel.org>
next prev parent reply other threads:[~2017-02-07 3:11 UTC|newest]
Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-16 23:55 [PATCH 0/2] perf: add support of SDT probes arguments Alexis Berlemont
2016-11-16 23:56 ` [PATCH 1/2] perf sdt: add scanning of sdt probles arguments Alexis Berlemont
2016-11-16 23:56 ` [PATCH 2/2] perf probe: add sdt probes arguments into the uprobe cmd string Alexis Berlemont
2016-11-17 9:04 ` Hemant Kumar
2016-11-18 23:56 ` [PATCH v2 0/2] " Alexis Berlemont
2016-11-18 23:56 ` [PATCH v2 1/2] perf sdt: add scanning of sdt probles arguments Alexis Berlemont
2016-11-25 14:40 ` Arnaldo Carvalho de Melo
2016-11-26 0:58 ` [PATCH v4 0/2] perf probe: add sdt probes arguments into the uprobe cmd string Alexis Berlemont
2016-12-05 23:42 ` Alexis Berlemont
2016-12-06 14:45 ` Arnaldo Carvalho de Melo
2016-11-26 0:58 ` [PATCH v4 1/2] perf sdt: add scanning of sdt probles arguments Alexis Berlemont
2016-12-07 2:44 ` Masami Hiramatsu
2016-11-26 0:58 ` [PATCH v4 2/2] perf probe: add sdt probes arguments into the uprobe cmd string Alexis Berlemont
2016-12-07 3:26 ` Masami Hiramatsu
2016-12-09 15:14 ` Arnaldo Carvalho de Melo
2016-12-10 10:00 ` Masami Hiramatsu
2016-12-14 0:07 ` [PATCH v5 0/2] " Alexis Berlemont
2016-12-14 7:36 ` Ingo Molnar
2017-01-23 11:23 ` Ravi Bangoria
2017-02-22 22:41 ` Alexis Berlemont
2017-01-24 6:58 ` Ravi Bangoria
2017-01-24 8:22 ` Ingo Molnar
2017-01-24 8:36 ` Ravi Bangoria
2017-02-02 11:11 ` [PATCH 0/5] perf/sdt: Argument support for x86 and powepc Ravi Bangoria
2017-02-02 11:11 ` [PATCH 1/5] perf/sdt: Show proper hint Ravi Bangoria
2017-02-02 13:40 ` Ingo Molnar
2017-02-02 16:20 ` Arnaldo Carvalho de Melo
2017-02-03 10:26 ` [PATCH v2] " Ravi Bangoria
2017-02-03 15:18 ` Arnaldo Carvalho de Melo
2017-02-07 7:53 ` Ingo Molnar
2017-02-07 15:50 ` Arnaldo Carvalho de Melo
2017-02-07 8:00 ` Ingo Molnar
2017-02-16 10:16 ` [RFC] perf/sdt: Directly record SDT event with 'perf record' Ravi Bangoria
2017-02-20 7:08 ` Ingo Molnar
2017-02-20 8:21 ` Ravi Bangoria
2017-02-20 8:42 ` Ingo Molnar
2017-02-20 11:01 ` Ravi Bangoria
2017-02-20 14:11 ` Arnaldo Carvalho de Melo
2017-02-23 8:13 ` Ravi Bangoria
2017-02-23 12:48 ` Arnaldo Carvalho de Melo
2017-02-07 1:13 ` [PATCH v2] perf/sdt: Show proper hint Masami Hiramatsu
2017-02-10 7:44 ` [tip:perf/core] perf sdt: Show proper hint when event not yet in place via 'perf probe' tip-bot for Ravi Bangoria
2017-02-02 11:11 ` [PATCH 2/5] perf/sdt/x86: Add renaming logic for rNN and other registers Ravi Bangoria
2017-02-07 3:11 ` Masami Hiramatsu
2017-03-21 14:08 ` Arnaldo Carvalho de Melo
2017-03-24 18:45 ` [tip:perf/core] perf sdt x86: " tip-bot for Ravi Bangoria
2017-02-02 11:11 ` [PATCH 3/5] perf/sdt/x86: Move OP parser to tools/perf/arch/x86/ Ravi Bangoria
2017-02-07 3:11 ` Masami Hiramatsu [this message]
2017-02-07 5:22 ` Ravi Bangoria
2017-03-21 14:10 ` Arnaldo Carvalho de Melo
2017-03-21 23:00 ` Masami Hiramatsu
2017-03-22 11:22 ` Arnaldo Carvalho de Melo
2017-03-21 14:55 ` Masami Hiramatsu
2017-02-02 11:11 ` [PATCH 4/5] perf/sdt/powerpc: Add argument support Ravi Bangoria
2017-02-02 11:11 ` [PATCH 5/5] perf/probe: Change MAX_CMDLEN Ravi Bangoria
2017-02-07 1:40 ` Masami Hiramatsu
2017-02-07 5:45 ` [PATCH v2] " Ravi Bangoria
2017-03-21 5:19 ` Masami Hiramatsu
2017-03-21 13:37 ` Arnaldo Carvalho de Melo
2017-03-24 18:43 ` [tip:perf/core] perf probe: " tip-bot for Ravi Bangoria
2017-02-07 2:55 ` [PATCH 0/5] perf/sdt: Argument support for x86 and powepc Masami Hiramatsu
2017-03-06 7:53 ` Ravi Bangoria
2017-03-06 13:42 ` Masami Hiramatsu
2017-03-21 5:08 ` [PATCH v5 0/2] perf probe: add sdt probes arguments into the uprobe cmd string Masami Hiramatsu
2016-12-14 0:07 ` [PATCH v5 1/2] perf sdt: add scanning of sdt probles arguments Alexis Berlemont
2017-03-06 13:39 ` Masami Hiramatsu
2017-03-21 13:52 ` Arnaldo Carvalho de Melo
2017-03-24 18:44 ` [tip:perf/core] perf sdt: Add scanning of sdt probes arguments tip-bot for Alexis Berlemont
2016-12-14 0:07 ` [PATCH v5 2/2] perf probe: add sdt probes arguments into the uprobe cmd string Alexis Berlemont
2017-01-24 8:50 ` Ravi Bangoria
2017-03-06 17:23 ` Masami Hiramatsu
2017-03-24 18:44 ` [tip:perf/core] perf probe: Add " tip-bot for Alexis Berlemont
2016-11-18 23:56 ` [PATCH v2 2/2] perf probe: add " Alexis Berlemont
2016-11-21 10:25 ` Hemant Kumar
2016-11-24 23:13 ` [PATCH v3 0/2] " Alexis Berlemont
2016-11-24 23:13 ` [PATCH v3 1/2] perf sdt: add scanning of sdt probles arguments Alexis Berlemont
2016-11-24 23:13 ` [PATCH v3 2/2] perf probe: add sdt probes arguments into the uprobe cmd string Alexis Berlemont
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170207121105.c16c1bded3f24c798eac318a@kernel.org \
--to=mhiramat@kernel.org \
--cc=acme@redhat.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=alexis.berlemont@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=maddy@linux.vnet.ibm.com \
--cc=mingo@redhat.com \
--cc=mpe@ellerman.id.au \
--cc=naveen.n.rao@linux.vnet.ibm.com \
--cc=peterz@infradead.org \
--cc=ravi.bangoria@linux.vnet.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).